Miscellaneous ContextTracker-related improvements #6058
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
murraystevenson
requested changes
Sep 26, 2024
There was a problem hiding this comment.
Improvements look good to me. As we discussed offline, I had some initial reservations about the brightness of "context yellow" when used in the Render Pass Editor, but after living with it for a day I've come around to its charms and it does fit well with the Spreadsheet active row indication, etc.
One minor thing that we'll need to update as part of this change is a reference to the Active column indicator as a "green dot" in the renderPassEditorArnold example.
Errors are caused by evaluating nodes with bad input, either in the form of plug values or of context variables. We have always cleared error badges when plugs are dirtied, which takes care of the situation where the user has fixed the input in the form of plug values. But until now we have stuck our head in the sand completely when it comes to errors due to bad contexts. But now we have the ContextTracker, we can associate errors with tracked contexts, and when the tracked context changes, clear any errors from previous contexts. Fixes GafferHQ#3820.
While also taking some small steps to making the window scene-agnostic. We no longer manage `scene:path` explicitly in the window, with one immediate benefit being that it doesn't leak a nonsense value into the context given to RenderPassEditor inspectors. This also fixes updates in the RenderPassEditor for animated properties, which weren't working in 1.4, even before the introduction of ContextTracker. I'm not entirely happy about passing an `inspectionPath` though. We do need to pass something to provide an inspection context, but it seems weird to pass a path, especially when the majority of the Context now comes from the ContextTracker. I'm wondering if we should instead pass a small dictionary of context overrides (`scene:path` in LightEditor and `renderPass` in RenderPassEditor). But I'm not sure how we'd obtain them. Regardless, this still represents progress.
johnhaddon
force-pushed
the
contextTrackerBits
branch
from
a3281ec
to
7a88c2d
Compare
Thanks for spotting that - squashed the change into dced4d4. Merging... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.