This adds a new node editor overlay that helps users to see where named attributes are used. This is important, because named attributes can have name collisions between independent node groups which can lead to hard to find issues.
Details
Details
- Reviewers
Hans Goudey (HooglyBoogly) - Maniphest Tasks
- T96275: Named attribute node group visualization
- Commits
- rBc71013082d09: Geometry Nodes: show used named attributes in nodes
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
- I originally imagined that the information would be in the node header rather than an overlay. The overlay is a bit more in-your-face, but it's more consistent with the general idea for having inspection info separate from nodes in the overlays. So it's more extensible, and the looks can be tuned in the future I suppose.
- I'm not convinced it's necessary to display this overlay directly above the named attribute nodes themselves, I would suggest only adding it over node groups that use them. Otherwise it looks quite noisy, and it's redundant with the information in the node.
- I understand this is a little more complex, but I think it would be better if the overlay didn't display attributes that were created inside and removed inside of node groups (also described in T96275). This overlay displays hidden dependencies, and those aren't really hidden dependencies. I expect that might be common when using higher level node groups in the future.
- For this to work, it would probably have to check if the attribute existed on any of the node group's geometry inputs or outputs before displaying it.
- The tooltip text is a bit prescriptive/commanding. That sort of "best practices" guidance probably belongs in the manual.
- My suggestion: "Attributes with these names used within the group may conflict with existing attributes"
| source/blender/editors/space_node/node_draw.cc | ||
|---|---|---|
| 1739 | TIP_ | |
| 1886–1892 | I would expect this string to be built by a callback rather than all the time. I think a pointer to the node (or its name or index) would be all that's necessary to pass to a callback. | |
| source/blender/nodes/intern/geometry_nodes_eval_log.cc | ||
| 500 | It might be better for this to return early if the name is empty, rather than doing it in the callers. | |
Comment Actions
I originally imagined that the information would be in the node header rather than an overlay. The overlay is a bit more in-your-face, but it's more consistent with the general idea for having inspection info separate from nodes in the overlays.
To me there isn't really a fundamental difference between this kind of information and other stuff we can display in overlays. Might even be good to move warnings into the overlays as well.
I'm not convinced it's necessary to display this overlay directly above the named attribute nodes themselves, I would suggest only adding it over node groups that use them.
I found that to be useful when the name is passed in from somewhere else. Maybe the overlay should only show when the name socket is linked?
I understand this is a little more complex, but I think it would be better if the overlay didn't display attributes that were created inside and removed inside of node groups (also described in T96275). This overlay displays hidden dependencies, and those aren't really hidden dependencies.
I see where this is coming from, but personally I think those are still hidden dependencies. For it work the user of the node group has to know not to use these attribute names for different purposes, because it might conflict with what happens within the node group. There could be a separate filter, but I don't think that is worth it now. I'd rather be more explicit with where named attributes are used for now, maybe we can relax this a bit in the future. If a node group creates and removes the same named attribute inside a node group, chances are, it shouldn't be a named attribute in the first place.
I expect that might be common when using higher level node groups in the future.
Seems likely, but I'd still prefer to be explicit about it. If I'm trying to figure out why a node tree doesn't work how I thought, I'd rather have the overlay show me all the named attributes that are used, instead of just a subset based on some heuristic.
The tooltip text is a bit prescriptive/commanding. That sort of "best practices" guidance probably belongs in the manual.
That's fine as well.
Comment Actions
Thanks for the reasoning. It's helpful, and I do find the argument about simplicity and avoiding unpredictable "heuristics" convincing.
Maybe the overlay should only show when the name socket is linked?
Yeah, I like that idea a lot!
Comment Actions
- Merge branch 'master' into named-attribute-usage-visualize
- fix
- move dynamic tooltip to callback
| source/blender/editors/space_node/node_draw.cc | ||
|---|---|---|
| 1886–1892 | I moved some of the stuff in the callback, but it doesn't work super nicely yet, because the bulk of the work (finding the used attributes) still has to be all the time. Think that can be improved with a more general logger refactor. | |
| source/blender/nodes/intern/geometry_nodes_eval_log.cc | ||
| 500 | Hm, to me it seems like something that the caller is responsible for. If the log function is called, it should also log something. | |
| source/blender/editors/space_node/node_draw.cc | ||
|---|---|---|
| 1886–1892 | Hmm, right, because finding whether there are any named attribute usages in a node group is basically the same work as gathering them for a tooltip. The iteration over all nodes in a group is ugly, but I agree that it's a more general problem that's out of scope here. | |
| source/blender/nodes/intern/geometry_nodes_eval_log.cc | ||
| 500 | That's also fine with me. In that case I think the input and store nodes need the empty() check. | |