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

ShaderTweakProxyUI : Fix when nodule menu appears #5873

Merged

Conversation

danieldresser-ie
Copy link
Contributor

Correcting the error John caught on #5842 after merging.

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Daniel. I've nitpicked the implementation in an inline comment, but this looks good enough to put in the release today if there's no time to fix it up. Should be a quick fix though...

Comment on lines 299 to 300
# but that seems like a pretty weird case, and we want to get to the early out without doing too much traversal
destPlug = Gaffer.PlugAlgo.findDestination( plug, lambda plug : plug if not plug.outputs() else None )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "too much traversal" is really a worry here is it? We use findDestination() in far more performance-critical places, like PlugValueWidget construction. The more canonical thing seems to be to use the Shader/ShaderTweaks check as the predicate, and then we don't need this NOTE at all.

@johnhaddon
Copy link
Member

johnhaddon commented May 29, 2024

Ugh, I've found another problem : the "Create proxy" button is showing up for popup editors in the LightEditor and the SceneViewInspector. We'll need to get it hidden for those...maybe the easiest fix is to only create it when the plug is directly parented to a ShaderTweaks node?

@johnhaddon johnhaddon force-pushed the shaderTweakProxyFix branch from 6125ae7 to 98c51a7 Compare May 29, 2024 14:12
@johnhaddon
Copy link
Member

In the interests of getting a release out today, I've pushed two more commits directly to this branch. The first is a fixup addressing my original review comment, and the second fixes the unwanted "Create proxy" button and another bug I found in the process of doing that.

@danieldresser-ie
Copy link
Contributor Author

LGTM. Sorry for the noise. Should I squash the fixup so someone can hit merge?

danieldresser-ie and others added 2 commits May 29, 2024 18:00
- Remove unused `__updateReadOnly()` method
- Disable button when plug is read-only.
- Hide button unless plug is directly on a ShaderTweaks node.
@johnhaddon johnhaddon force-pushed the shaderTweakProxyFix branch from 98c51a7 to bbd1add Compare May 29, 2024 17:00
@johnhaddon johnhaddon merged commit 7e03f5a into GafferHQ:1.4_maintenance May 29, 2024
5 checks passed
@johnhaddon
Copy link
Member

Should I squash the fixup so someone can hit merge?

All good - I've just done it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants