Skip to content
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

MergeScenes bugfix #6174

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Conversation

johnhaddon
Copy link
Member

This fixes a fairly obscure bug in MergeScenes, and simplifies the code and removes some unnecessary contexts at the same time.

We're already in the right context to call `existsPlug()->getValue()` directly. Instead we were calling `exists()` which was constructing a new context identical to the one we already had.
Examples of such an input might be a promoted plug that hasn't yet been connected to anything. In this case we were treating that input as active for every single location declared by the other inputs, and if it was the first input it would take precedence over the other inputs when in Keep mode. This would mean all attributes being lost from the location.

I did look into an alternative fix : defaulting `ScenePlug.exists` to false. But that caused problems for the Parent node when omitting the primary input and parenting children to `/`. There just is no good default value for `exists`; it should be true for the root and false for all other locations.
@johnhaddon johnhaddon self-assigned this Dec 6, 2024
@danieldresser-ie
Copy link
Contributor

It's pretty unfortunate this is needed - the goal of existsPlug is to handle this stuff ... but this does appear to be a pragmatic solution.

The only alternative I could think of was building this logic into the exists() function so that it would be implemented more centrally, but you've correctly pointed out that this caller shouldn't need to use exists(). Hmmm.

The code LGTM.

@johnhaddon johnhaddon merged commit a6f4459 into GafferHQ:1.5_maintenance Dec 9, 2024
5 checks passed
@johnhaddon johnhaddon deleted the mergeScenesFixes branch December 10, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending release
Development

Successfully merging this pull request may close these issues.

2 participants