VisualiserTool : Vertex Labels #6212

ericmehl · 2025-01-14T20:45:13Z

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 Auto mode, leaving the other data types to be displayed as colors. And there's a dedicated Vertex Label mode 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

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.

johnhaddon

Thanks Eric - this seems to be working nicely. I've made a few comments about the implementation inline - feel free to ping me when you get in if it's easier to chat about them in person.
Cheers...
John

python/GafferSceneUI/VisualiserToolUI.py

src/GafferSceneUI/VisualiserTool.cpp

python/GafferSceneUI/VisualiserToolUI.py

src/GafferSceneUI/VisualiserTool.cpp

johnhaddon · 2025-01-21T12:09:54Z

python/GafferSceneUI/VisualiserToolUI.py

+			"preset:Color (Type Range)", GafferSceneUI.VisualiserTool.Mode.ColorTypeRange,
+			"preset:Color (Manual Range)", GafferSceneUI.VisualiserTool.Mode.ColorManualRange,


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.

That makes sense to me, "Type Range" would probably be overly specific for most users. Changed in be2fd8f, e2be4c6 and 2d9da70

Could we put "Color (Auto Range)" after "Color" please? To me it is a variant of the first, so makes sense to come second.

Right, yes, fixed in a97c8f1

src/GafferSceneUI/VisualiserTool.cpp

ericmehl · 2025-01-23T23:30:17Z

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.

ericmehl · 2025-01-27T23:40:35Z

All the comment should be addressed now, including an improvement to the data handling that uses std::variant. There's one fixup, 33621af, that might have a more elegant solution, but I haven't had a chance to dig into the data template code to figure out what's going wrong with the original approach there.

With that noted, ready for a new look...

python/GafferSceneUI/VisualiserToolUI.py

johnhaddon · 2025-01-28T10:25:40Z

include/GafferSceneUI/Private/VisualiserTool.h

 		Imath::V2f cursorPos() const;
+		bool cursorPosValid() const;


I looked a little at getting rid of the separate m_cursorPosValid variable and just rely on the state of m_cursorPos, but I think they are logically different enough that they are both needed.

Can you explain? My comment was based on the assumption that the two variables had combined semantics that were equivalent to optional. When do we use m_cursorPos while m_cursorPosValid == false, and why?

src/GafferSceneUI/VisualiserTool.cpp

johnhaddon · 2025-01-28T10:48:54Z

python/GafferSceneUI/VisualiserToolUI.py

+			"preset:Color (Type Range)", GafferSceneUI.VisualiserTool.Mode.ColorTypeRange,
+			"preset:Color (Manual Range)", GafferSceneUI.VisualiserTool.Mode.ColorManualRange,


Could we put "Color (Auto Range)" after "Color" please? To me it is a variant of the first, so makes sense to come second.

johnhaddon · 2025-01-28T10:55:29Z

Thanks Eric, I think we're close now! Sorry, I somehow submitted my inline comments without a covering letter, so am making one last comment here.

I wonder if it is possible to make Auto mode even smarter when rendering a points primitive? Currently, float, point and color data just isn't visualised at all in this mode, so could we show it as vertex labels, like we do for ints? Soon we'll be able to default to lines for vector and normal data, but I think labels will always be the best we can do for the other types?

ericmehl · 2025-01-28T23:27:19Z

I crossed off the comments above, and added some better handling of Points and Curves in 269d03e.

I also found we were getting possibly misleading drag and drop data in some cases. For example, we're not visualising data with FaceVarying interpolation in VertexLabel mode for now. But if you drag from an object in that case (for example, visualise uv in VertexLabel mode), you'll get the sampled value even though nothing is showing up in the viewport.

I think it's worth not dragging those values and letting it instead drag the scene path. I prevented against that in e078806.

One last thing to do is a pass at the node documentation to make sure it's correct through all the permutations the code goes through here.

But the rest is ready for another review.

johnhaddon · 2025-01-29T10:23:57Z

One last thing to do is a pass at the node documentation to make sure it's correct through all the permutations the code goes through here.

One small suggestion here : don't list of all the permutations of decisions that "Auto" mode makes. Just document it as "Chooses the most appropriate mode based on the data and primitive type". The permutations are only going to get more complex when we add vector and orientation modes, and the point of Auto is that folks don't have to think.

But the rest is ready for another review.

LGTM! Thanks for the fixups and for the additional extras you caught. Could you squash all this down and merge away please, after checking over the docs. Note that we've released 1.5.4.0 already, so the Changes.md stuff will need moving.

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 · 2025-01-29T16:29:17Z

LGTM! Thanks for the fixups and for the additional extras you caught. Could you squash all this down and merge away please, after checking over the docs. Note that we've released 1.5.4.0 already, so the Changes.md stuff will need moving.

Wonderful! I squashed it down and simplified the documentation as you recommended. That's much improved now. The changelog entries are in the right place as well.

Merging!

johnhaddon · 2025-01-29T16:43:57Z

Woopwoop! 🎉

ericmehl mentioned this pull request Jan 14, 2025

Visualiser mode plug #6190

Closed

4 tasks

ericmehl force-pushed the visualiseInteger branch from 97f6cf9 to d853320 Compare January 14, 2025 20:48

johnhaddon reviewed Jan 21, 2025

View reviewed changes

ericmehl force-pushed the visualiseInteger branch from 70f87c9 to 398f9a3 Compare January 23, 2025 21:16

ericmehl force-pushed the visualiseInteger branch from 072a654 to 33621af Compare January 27, 2025 23:21

johnhaddon reviewed Jan 28, 2025

View reviewed changes

ericmehl force-pushed the visualiseInteger branch 2 times, most recently from b5a929e to e078806 Compare January 28, 2025 23:13

ericmehl added 15 commits January 29, 2025 10:41

VisualiserTool : Add visualiser modes

48d5613

VisualiserTool : Separate color visualiser

252a5ff

VisualiserTool : No color for Auto mode + integers

7e542db

VisualiserTool : Add per-vertex label shaders

a9c5433

VisualiserTool : primitiveVariable: prefix

f28ef41

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.

VisualiserTool : Add vertex index label

cf3a2f6

VisualiserTool : Closest vertex index

f38f961

VisualiserTool : Vertex IDs with stroked text

fb4873c

VisualiserTool : Use fmt::format for values

3787886

Timing the previous `stringstream` implementation vs `fmt::format` shows about a 70% speedup for formatting a sample mesh's vertex indices.

VisualiserTool : Show int data in Auto mode

666412d

VisualiserTool : VertexLabel mode

b0d5aa7

VisualiserTool : Remove unclear comments

d9fd9d3

VisualiserTool : Remove misleading ref assignment

1dabc52

VisualiserTool : Use std::variant

32390bb

VisualiserTool : Support points and curves

a404dfd

ericmehl force-pushed the visualiseInteger branch from e078806 to a404dfd Compare January 29, 2025 15:48

ericmehl merged commit cf64191 into GafferHQ:1.5_maintenance Jan 29, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VisualiserTool : Vertex Labels #6212

VisualiserTool : Vertex Labels #6212

ericmehl commented Jan 14, 2025

johnhaddon left a comment

johnhaddon Jan 21, 2025

ericmehl Jan 27, 2025 •

edited

Loading

johnhaddon Jan 28, 2025

ericmehl Jan 28, 2025

ericmehl commented Jan 23, 2025

ericmehl commented Jan 27, 2025

johnhaddon Jan 28, 2025

johnhaddon Jan 28, 2025

johnhaddon commented Jan 28, 2025

ericmehl commented Jan 28, 2025

johnhaddon commented Jan 29, 2025

ericmehl commented Jan 29, 2025

johnhaddon commented Jan 29, 2025

		"preset:Color (Type Range)", GafferSceneUI.VisualiserTool.Mode.ColorTypeRange,
		"preset:Color (Manual Range)", GafferSceneUI.VisualiserTool.Mode.ColorManualRange,

VisualiserTool : Vertex Labels #6212

VisualiserTool : Vertex Labels #6212

Conversation

ericmehl commented Jan 14, 2025

Checklist

johnhaddon left a comment

Choose a reason for hiding this comment

johnhaddon Jan 21, 2025

Choose a reason for hiding this comment

ericmehl Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

johnhaddon Jan 28, 2025

Choose a reason for hiding this comment

ericmehl Jan 28, 2025

Choose a reason for hiding this comment

ericmehl commented Jan 23, 2025

ericmehl commented Jan 27, 2025

johnhaddon Jan 28, 2025

Choose a reason for hiding this comment

johnhaddon Jan 28, 2025

Choose a reason for hiding this comment

johnhaddon commented Jan 28, 2025

ericmehl commented Jan 28, 2025

johnhaddon commented Jan 29, 2025

ericmehl commented Jan 29, 2025

johnhaddon commented Jan 29, 2025

ericmehl Jan 27, 2025 •

edited

Loading