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

VisualiserTool Primitive Variable Menu #6181

Merged

Conversation

ericmehl
Copy link
Collaborator

This adds a handy sub-menu to the context menu for the VisualisationTool.dataName plug listing all the primitive variables that can be visualised by the tool for the current selection.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

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 Eric - helping folks find the things they want is definitely a good thing. I've highlighted a couple of bugs inline that should be pretty straightforward to take care of.

Having this as a submenu of the right-click menu still feels a bit out of the way to me in practice, and I'm not sure how we'll fit the "Vertex ID" mode in with it. I think it might be nice to write a custom PlugValueWidget hosting a MenuButton with your menu (and only your menu) on it - what do you think?

python/GafferSceneUI/VisualiserToolUI.py Outdated Show resolved Hide resolved
python/GafferSceneUI/VisualiserToolUI.py Outdated Show resolved Hide resolved
python/GafferSceneUI/VisualiserToolUI.py Outdated Show resolved Hide resolved
python/GafferSceneUI/VisualiserToolUI.py Outdated Show resolved Hide resolved
python/GafferSceneUI/VisualiserToolUI.py Outdated Show resolved Hide resolved
@ericmehl ericmehl force-pushed the visualiserToolPrimVarMenu branch from 3d656e1 to 53aa8ed Compare December 17, 2024 16:48
@ericmehl ericmehl mentioned this pull request Dec 17, 2024
4 tasks
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 for the updates Eric - they look good. Could you quickly take care of the two additional comments I've just made please, and then we can get this merged and into the next release.

def __setDataName( self, value, *unused ) :

self.getPlug().setValue( value )
self.__menuButton.setText( value )
Copy link
Member

Choose a reason for hiding this comment

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

This should really be done in an _updateFromValues() override, so that the button is updated no matter how the plug's value is changed.

Copy link
Collaborator Author

@ericmehl ericmehl Jan 7, 2025

Choose a reason for hiding this comment

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

Fixed and squashed into c78be9e. I guess using the --- string in case someone tries to assign multiple different values to the plug is appropriate? I'm not sure it entirely makes sense, maybe we should error instead?

Copy link
Member

Choose a reason for hiding this comment

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

You'll only get multiple different values when the widget has been configured to edit multiple plugs at once, which won't happen for this widget. But I think an empty string will now be replaced with ---, which probably doesn't make sense? Maybe None would be better?

Copy link
Member

Choose a reason for hiding this comment

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

While I'm nitpicking, let's also move _updateFromValues() above the private methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds good. Changed to None and moved the method above the private methods in dfe1c9a

Changes.md Outdated
@@ -16,7 +16,7 @@ Features
- Inference : Loads ONNX models and performs inference using an array of input tensors.
- ImageToTensor : Converts images to tensors for use with the Inference node.
- TensorToImage : Converts tensors back to images following inference.
- VisualiserTool : Added tool to 3D viewer for visualising primitive variables on meshes.
- VisualiserTool : Added tool to 3D viewer for visualising primitive variables on meshes. A list of available variable names for the current selection is available from the `dataName` context menu.
Copy link
Member

Choose a reason for hiding this comment

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

This will need moving to the 1.5.x.x section since we've released 1.5.2.0 now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed and squashed into c78be9e

@ericmehl ericmehl force-pushed the visualiserToolPrimVarMenu branch from 53aa8ed to c78be9e Compare January 7, 2025 17:39
@ericmehl ericmehl force-pushed the visualiserToolPrimVarMenu branch from c78be9e to dfe1c9a Compare January 7, 2025 18:05
@johnhaddon johnhaddon merged commit bbd7752 into GafferHQ:1.5_maintenance Jan 8, 2025
5 checks passed
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