-
-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CURA-10722] Add 'additional settings providers' (Engine Plugins) #890
[CURA-10722] Add 'additional settings providers' (Engine Plugins) #890
Conversation
Since they're loaded on top instead of going through the normal inheritance system, we need to merge/add them to each definition-container. This can't happen during load from file either, since most times, it's loaded from the cache-db instead. Instead, load the extra settings into the Definition ones each time one is loaded. If this turns out to be slow, we could pre-parse the json into (proto) SettingsDefinitions beforehand maybe, but it's not clear how much that is the actual bottleneck and how much code would need to be either duplicated or upended. part of CURA-10722
part of CURA-10722
Needed to use underscores instead of :: so that it would work as python variable names. part of CURA-10722
When saving to cfg (which is in the zipped 3mf file), the variables are forced to lower case. If a plugin-ID contains capital letters, it would not be able to match the settings it saved and the setting loaded otherwise. part of CURA-10722
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; As we discussed during the stand-up we will add functionality to the plugin system as a whole (both front-end and curaengine_plugins) which will allow to add settings by simply specify a path to a json
file. Which will have setting entries such as they are defined in fdmprinter.def.json
Also pinging @fieldOfView because I think he also could have some valuable insights on this PR and at the very least FYI |
done as part of CURA-10722 Co-authored-by: Jelle Spijker <[email protected]>
You could still override the getAdditionalSettingDefinitions method like before to get the same result as previous, but you can now also just put a path in the definition_file_paths variable to load from there instead. part of CURA-10722
Not all of them have to be for engine-plugins after all. part of CURA-10722
'sup? Hmm, I don't quite get why this would be necessary. Many of my plugins are adding settings that eventually also get sent to CuraEngine (which happily ignores them), then to be used in postprocessing. It is all a bit hacky (private method access), but has been working very well for years. |
@fieldOfView -- If you don't see the need to change for your own plugins, that's fine. We're basically using the same mechanism anyway, so we won't clash. Since (other) people are (we hope) going to be writing engine-plugins (also), there is additional need to make a proper (non-hacky) way to define additional settings for engine plugins. -- We may trust you with our internals, but that's not necessarily the case for every developer out there. Also to us these settings do not feel quite the same as plugin preferences, and we'd like the possibility of defining values for them in profiles (for ex.; a 'weird printer' plugin that comes with an engine part and a bunch of profiles for example). Additionally, different plugins might have the same setting-definition name added. We tag/'sort-of-namespace' the settings internally so that they at least don't destabilize the system. A similar story for different versions of the same plugin that might have tweaked their setting-definition meanings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from my other review comments; We need unit tests for this.
part of CURA-10722
The prepend method was only ever used by the Appender anyway. This is simpler. part of CURA-10722
part of CURA-10722
part of CURA-10722
Plugins should now be able to add settings. (But not append enum-values to existing ones, that's another ticket.)
See (now) also the accompanying front-end PR here: Ultimaker/Cura#16305