Added setting for disabling forcing context settings#193
Conversation
…e) upon opening an existing Script.
BigRoy
left a comment
There was a problem hiding this comment.
Very nice - disabling the integration to mess with your scenes on open I think is a good thing. My personal preference, I don't want to open your scene and have it altered without your knowledge. So this is a step the good way.
I'd love to still be notified and asked if I want to fix certain things though, but not super important.
@jakubjezek001 @LiborBatek thoughts?
… template builder back where they were.
|
But isnt that exactly what a pipeline should do set the correct frame range ? what is the use case ? under a task with a defined frame range you dont want to have that frame range ? Or maybe I am not getting what this is ment for... |
|
My particular use case is an artist working under a test shot doing various experiments. In my previous job we did consider it very important to ensure people are working with the correct context settings, however we also didn't like to impose too much in the way of changing someone's work file automatically without flagging the changes. We had a "set the correct context settings" menu item so an artist could ensure it's all correct, and also various moments where they'd be checked and if wrong a dialog would pop up telling them which were wrong and asking whether to set them correctly or not. Then obviously on creating a new work file, those settings would be set correctly. I guess another example might be the frame changing after a work file has been created. The artist does need to have their frame changed, but they need to be notified about it before it happens as they might want to sort a few things out before setting it. |
|
I agree that user notification is a good thing, a validator on publish does that... I still think that I lean more towards maybe an "opt out" button per script, rather than a studio wide setting ? Would be nice to have some metadata stored on root and if that says "dont update me" the callback just skips the update ? Just thinking out loud ... but I do get the usecase ... that if you are testing stuff too strict pipelines are not helpful... |
|
Yes, that makes sense. I could modify this to do that? Remove the new setting and add a knob to Root for opting out a particular script? |
|
Oh, actually I'm gonna predict that my artist would still want to be clearly notified / asked to change the settings even on a proper shot. If I did the above change, I could also roll in some kind of question dialog popping up (only if anything needs changing)? |
I really like the way you think! Yes this could be very good design from my perspective. Settings in studio/project overrides are also good, to be honest. Some productions might want to prefer to disable it but for about 5 years in productions no one was complaining. I am sharing openion with @BigRoy that someone might not like to have workfile changed at starting without notification about the changes. We already do this for example with colorspace config change. There is alrady mechanism in place that if OCIO config is not the same as in settings so it is popping up dialogue with notification. Trigger for the change is I believe hooked to onScriptsave callback. Not ideall but to be honest better then an interval check, since users tent to Ctrl+s quite often. |
|
Random question/suggestion: I've got into the habit over the last few years of running Black on Python code to 1) have a consistent formatting and 2) not have to really think about formatting. Reckon something like that might be a nice idea for these repos? |
…xt_settings_on_load_script
Pretty sure majority of our repos are set up for And just ruff fixing all files would make anyone with forks less happy to fix 'code conflicts' :) |
| # set apply all workfile settings on script load and save | ||
| nuke.addOnScriptLoad(WorkfileSettings().set_context_settings) | ||
| if nuke_settings["general"]["set_context_settings_on_script_open"]: | ||
| # set apply all workfile settings on script load and save |
There was a problem hiding this comment.
Pretty sure this comment above it was just wrong?
| # set apply all workfile settings on script load and save | |
| # set apply all workfile settings on script load |
| nuke.addOnScriptLoad(WorkfileSettings().set_context_settings) | ||
| if nuke_settings["general"]["set_context_settings_on_script_open"]: | ||
| # set apply all workfile settings on script load and save | ||
| nuke.addOnScriptLoad(WorkfileSettings().set_context_settings) |
There was a problem hiding this comment.
Pretty sure this is actually wrong - and apparently always has been?
Because this initializes WorkfileSettings DIRECTLY here - and not on the callback. As such, this is set to the context of AYON when Nuke is launched. If you open a workfile in a different context, it'd still be applying the workfile settings of the context you launched into.
| nuke.addOnScriptLoad(WorkfileSettings().set_context_settings) | |
| nuke.addOnScriptLoad(lambda: WorkfileSettings().set_context_settings) |
|
|
||
| # adding favorites to file browser | ||
| nuke.addOnCreate(workfile_settings.set_favorites, nodeClass="Root") | ||
| nuke.addOnCreate(WorkfileSettings().set_favorites, nodeClass="Root") |
There was a problem hiding this comment.
Pretty sure this is actually wrong - and apparently always has been?
Because this initializes WorkfileSettings DIRECTLY here - and not on the callback. As such, this is set to the context of AYON when Nuke is launched. If you open a workfile in a different context, it'd still be applying the workfile settings of the context you launched into.
| nuke.addOnCreate(WorkfileSettings().set_favorites, nodeClass="Root") | |
| nuke.addOnCreate(lambda: WorkfileSettings().set_favorites, nodeClass="Root") |
@jakubjezek001 right?
…tion before setting format or frame range. Can also set a specific script to check context settings on load or not, overriding the general setting if on.
|
I've made those tweaks you pointed out, and also put in asking for confirmation, changing the general setting to "check" rather than "set" and added support for a specific script to have checking turned off. |
… confirm set context setting message box
BigRoy
left a comment
There was a problem hiding this comment.
Thanks - I think code got worse now with latest changes - so more tweaks will be needed. I'll comment my first pass - @jakubjezek001 who's off this week may be better suited to comment from Nuke's perspective so my comments are mainly on the coded approach, not anything nuke-specific.
| if not ASSIST: | ||
| self._always_check_check_box = QtWidgets.QCheckBox( | ||
| "Always check this Script's context settings on load\n" | ||
| "(if switched off, can be switched back on in Project Settings User tab)" | ||
| ) | ||
| self._always_check_check_box.setChecked(True) | ||
| layout.addWidget( | ||
| self._always_check_check_box | ||
| ) | ||
| self._always_check_check_box.toggled.connect(self._always_check_check_box_toggled) |
There was a problem hiding this comment.
Please do not expose to the user to change the settings. Not every user has these permissions, or should even be allowed to care. That's for admin permissions, and even then you don't want anyone in production to accidentally misclick here either.
There was a problem hiding this comment.
This is just for excluding the current script from context setting checks on load, rather than the global setting.
| def set_context_settings(self, requires_confirmation=False): | ||
| self.reset_resolution(requires_confirmation) | ||
| self.reset_frame_range_handles(requires_confirmation) | ||
| # todo: No need to check confirmation for color? | ||
| self.set_colorspace() |
There was a problem hiding this comment.
This will start prompting once for every thing it needs to reset - we'll definitely want to avoid that, especially if we may add more over time. I'd rather present the user of all things invalid in current context, and then tell them about all. Not a massive issue, but may as well tackle now.
Also, when Nuke is running in headless mode - there's no such things as showing user a pop-up. In that case it should just log the fact that it's not the right values and allow it to continue without automatically fixing it (that's my personal preference though; @jakubjezek001 ?)
There was a problem hiding this comment.
Yes, good point about headless mode. I'll fix that.
With one pop-up showing all required changes, are their options "all or nothing" or should that pop-up let them control which to set, and if so how granular should it be?
There was a problem hiding this comment.
With one pop-up showing all required changes, are their options "all or nothing" or should that pop-up let them control which to set, and if so how granular should it be?
Probably a single prompt with checkboxes:
- Fix resolution
- Fix frame range
- Fix colorspace
With default having all checkboxes enabled? and the prompt can be confirmed or cancelled.
| if knob_name_current_new_value_triplets_by_node: | ||
| if not requires_confirmation or ConfirmSetContextSettingMessageBox.ask_for_confirmation( | ||
| "\n\n".join( | ||
| "{}:\n{}".format( | ||
| node.name(), | ||
| "\n".join( | ||
| f"{knob_name}: {current_value} -> {new_value}" for knob_name, current_value, new_value in knob_name_current_new_value_triplets | ||
| ) | ||
| ) for node, knob_name_current_new_value_triplets in knob_name_current_new_value_triplets_by_node.items() | ||
| ) | ||
| ): | ||
| was_node_graph_updated = False | ||
| for node, knob_name_current_new_value_triplets in knob_name_current_new_value_triplets_by_node.items(): | ||
| # Root node will be first. | ||
| # Based on how the code was when I found it, | ||
| # `update_node_graph` needs calling between | ||
| # root node and viewers - splidje | ||
| if not was_node_graph_updated and node != self._root_node: | ||
| # update node graph so knobs are updated | ||
| update_node_graph() | ||
| was_node_graph_updated = True | ||
| for knob_name, _, new_value in knob_name_current_new_value_triplets: | ||
| node[knob_name].setValue(new_value) | ||
|
|
||
| # in case no Viewers needed knob changes | ||
| if not was_node_graph_updated: | ||
| # update node graph so knobs are updated | ||
| update_node_graph() |
There was a problem hiding this comment.
Why does this suddenly need all this complexity? This is very hard to read and grasp.
Admittedly - all we're really changing is:
- Before
reset_frame_range_handlesgets called, ask for confirmation of changes. - Or better, before any of the
reset_..._gets called handle confirmation.
To know whether we need to ask the user we need two things:
-
- Are we checking?
-
- Is there anything to change?
The way I see it is what we should do is separate the logic of getting the current and expected state from the setting the state.
# overly simplified psuedocode
should_check = ...[settings][check][blabla]
if not should_check:
return
current, expected = get_current_state()
if current == expected:
return
if headless:
# log difference
log.warning("Values mismatch: {current} != {headless}")
return
accepted = prompt_user(current, expected):
if accepted:
set_state(expected)But in short, the setting of the values should not be changing or get more complicated at all?
We just end up having a library set_frame_range, set_colorspace and set_frame_range functions that will end up being called, right?
There was a problem hiding this comment.
Yes, makes sense to make this neater and more readable.
Above psuedocode makes sense to me.
When I find the time I'll do another sweep and make these changes.
Back to this. How do I use ruff? I can see a |
…xt_settings_on_load_script
…xt_settings_on_load_script
try |
Changelog Description
Added setting for disabling forcing context settings (e.g. frame range) upon opening an existing Script.
Additional review information
Added a new setting under general described as "Set Context Settings on Script Open".
Defaults to True, but if False opening a Nuke Script by any means - e.g. via Ayon -> Workfiles, selecting from the Launcher, using vanilla Nuke Open - skips calling
set_context_settings, so e.g. keeps Root node first/last frame as it was on save.Testing notes: