VisualiserTool : Vertex Labels #6212
Conversation
ericmehl
force-pushed
the
visualiseInteger
branch
from
97f6cf9 to
d853320
Compare
| menuDefinition.prepend( | ||
| "/Vertex Index", | ||
| { | ||
| "command" : functools.partial( Gaffer.WeakMethod( self.__setDataName ), self.__vertexIndexDataName ), | ||
| "checkBox" : self.getPlug().getValue() == self.__vertexIndexDataName, | ||
| } | ||
| ) | ||
| menuDefinition.prepend( "/Implicit", { "divider" : True, "label" : "Implicit" } ) |
There was a problem hiding this comment.
My guess is that Vertex Index will be used less commonly than the various primitive variables, and as the odd one out I think it would make more sense at the bottom of the menu. I'd also suggest using "Other" as the title rather than "Implicit" - it seems a bit more straightforward.
One other note on the menu : it won't show primitive variables from PointsPrimitives, even though we can now draw them. Would be good to get that fixed.
| if( layer == Gadget::Layer::MidFront ) | ||
| { | ||
| renderColorVisualiser( viewportGadget ); | ||
| renderColorVisualiser( viewportGadget, mode ); |
There was a problem hiding this comment.
Passing the mode to the individual render methods was a surprise to me at first. I was expecting us to resolve "Auto" mode upfront and then only call the single appropriate render method.
I think the reason we're doing it like this instead is so that Auto mode can choose different modes for each object in the selection, when the primvar on those objects is of a different type. Is that right? If so, might be worth a comment.
|
|
||
| if( mode == VisualiserTool::Mode::Auto && vData->typeId() == IntVectorDataTypeId ) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
Worth a comment here I think - "Will be handled by renderVertexLabelValue() instead".
| @@ -164,18 +166,19 @@ | |||
|
|
|||
| class _DataNameChooser( GafferUI.PlugValueWidget ) : | |||
|
|
|||
| __primVarPrefix = "primVar:" | |||
There was a problem hiding this comment.
May I suggest "primitiveVariable:" - we don't abbreviate in the other spots we refer to primitive variables.
|
|
||
| def __primVarFromDataName( self, name ) : | ||
|
|
||
| return name[8:] if name.startswith( self.__primVarPrefix ) else "" |
There was a problem hiding this comment.
Seems odd to go to the trouble of having __primVarPrefix and then hardcoding the 8.
src/GafferSceneUI/VisualiserTool.cpp
Outdated
| case FloatVectorDataTypeId : | ||
| vData = primitive->expandedVariableData<FloatVectorData>( | ||
| primVarName, | ||
| IECoreScene::PrimitiveVariable::Vertex, | ||
| false /* throwIfInvalid */ | ||
| ); | ||
| break; | ||
| case V2fVectorDataTypeId : | ||
| vData = primitive->expandedVariableData<V2fVectorData>( | ||
| primVarName, | ||
| IECoreScene::PrimitiveVariable::Vertex, | ||
| false /* throwIfInvalid */ | ||
| ); | ||
| break; | ||
| case V3fVectorDataTypeId : | ||
| vData = primitive->expandedVariableData<V3fVectorData>( | ||
| primVarName, | ||
| IECoreScene::PrimitiveVariable::Vertex, | ||
| false /* throwIfInvalid */ | ||
| ); | ||
| break; | ||
| case Color3fVectorDataTypeId : | ||
| vData = primitive->expandedVariableData<Color3fVectorData>( | ||
| primVarName, | ||
| IECoreScene::PrimitiveVariable::Vertex, | ||
| false /* throwIfInvalid */ | ||
| ); | ||
| break; |
There was a problem hiding this comment.
This seems rather longwinded when you consider that vData is just a ConstDataPtr, so we've thrown away all the type information we got from the templated calls to expandedVariableData(). Can we just use one expandedVariableData<Data>() call after checking the types more concisely?
| if( auto fData = runTimeCast<const FloatVectorData>( vData.get() ) ) | ||
| { | ||
| auto data = runTimeCast<FloatData>( vertexValue ); | ||
| if( !data ) | ||
| { | ||
| data.reset( new FloatData() ); | ||
| } | ||
| data->writable() = fData->readable()[i]; | ||
| vertexValue = data; | ||
| } | ||
| if( auto v2fData = runTimeCast<const V2fVectorData>( vData.get() ) ) | ||
| { | ||
| auto data = runTimeCast<V2fData>( vertexValue ); | ||
| if( !data ) | ||
| { | ||
| data.reset( new V2fData() ); | ||
| } | ||
| data->writable() = v2fData->readable()[i]; | ||
| vertexValue = data; | ||
| } | ||
| if( auto v3fData = runTimeCast<const V3fVectorData>( vData.get() ) ) | ||
| { | ||
| auto data = runTimeCast<V3fData>( vertexValue ); | ||
| if( !data ) | ||
| { | ||
| data.reset( new V3fData() ); | ||
| } | ||
| data->writable() = v3fData->readable()[i]; | ||
| vertexValue = data; | ||
| } | ||
| if( auto c3fData = runTimeCast<const Color3fVectorData>( vData.get() ) ) | ||
| { | ||
| auto data = runTimeCast<Color3fData>( vertexValue ); | ||
| if( !data ) | ||
| { | ||
| data.reset( new Color3fData() ); | ||
| } | ||
| data->writable() = c3fData->readable()[i]; | ||
| vertexValue = data; | ||
| } |
There was a problem hiding this comment.
Why are we going to all this trouble of making an intermediate per-vertex Data value? Why don't we just format to a string directly here?
| "preset:Color (Type Range)", GafferSceneUI.VisualiserTool.Mode.ColorTypeRange, | ||
| "preset:Color (Manual Range)", GafferSceneUI.VisualiserTool.Mode.ColorManualRange, |
There was a problem hiding this comment.
I know you're trying to convey the underlying mechanism in the name here, but I do wonder if maybe "Type Range" is a bit unobvious? I think maybe "Auto Range" conveys more quickly that the range will be picked for you, and then the tooltip can tell you the details of how.
Maybe just "Color" and "Color (Auto Range)" for the names?
I'm bikeshedding here, and others may disagree, so feel free to canvas other opinions.
src/GafferSceneUI/VisualiserTool.cpp
Outdated
| drawStrokedText( | ||
| viewportGadget, | ||
| text, | ||
| m_tool->sizePlug()->getValue(), |
There was a problem hiding this comment.
I suspect it's worth lifting sizePlug()->getValue() out of the per-vertex loop.
src/GafferSceneUI/VisualiserTool.cpp
Outdated
| cursorVertexRasterPos.y | ||
| ), | ||
| style, | ||
| Style::State::HighlightedState |
There was a problem hiding this comment.
I'm finding the blue text for the hovered vertex value to be less readable than the white. I think perhaps the size change is enough on its own.
This is in preparation for visualising vertex indices. By namespacing primitive variable names, we can ensure there are no collisions between special data names (for things like vertex indices) and primitive variable names.
Timing the previous `stringstream` implementation vs `fmt::format` shows about a 70% speedup for formatting a sample mesh's vertex indices.
ericmehl
force-pushed
the
visualiseInteger
branch
from
70f87c9 to
398f9a3
Compare
|
I think I have the comments addressed except for the drag-value-shared-data part. Hold off just a bit on reviewing the fixups so I can take a fresh look on Monday and we can talk through the data handling part then too. |
This adds a new method for visualising object data, a vertex label per-vertex.
It uses this new method for vertex indices and integer primitive variables when in
Automode, leaving the other data types to be displayed as colors. And there's a dedicatedVertex Labelmode to see other data types as a label as well. There are some additions that could be made, such as visualising string variables, but I've drawn the line for the PR at the types we support.This also includes the change from #6190, which I'll close.
Checklist