Details
Diff Detail
- Repository
- rB Blender
- Branch
- master
- Build Status
Buildable 14153 Build 14153: arc lint + arc unit
Event Timeline
Can all of the inputs be as attributes? That's always really handy. Is there also Euler rotation instead of Axis angle as an option?
Yes, the same options as the other Vector Rotate node.
Do you mean attribute inputs by default?
Not necessarily by default but just as an option. You can't bend things properly without having them as attributes is all.
They're all there though 👌
Thanks for adding this!
This node unfortunately really highlights the issue that we still have in the attribute workflow with switching between attributes and regular values. The inputs here should really all be possible to switch, so for now I don't really see a better option.
In the future I can see this node getting irrelevant once we have something like the planned attribute processor, but we also need to look into making the interface for changing the input type better again.
To this node itself:
- functionality works great as far as I can tell
- the sheer size of the node when adding it in is quite worrying to me tbh
- Looking at it again, I think the Vector type enum can be removed. The result is always an attribute and so can be the input. There is the fill node if a single input vector should be used
- It generally makes sense to have the default for at least center and axis not be attributes, but for space reasons I am leaning towards changing all defaults to attribute. Attribute nodes are generally leaning towards having mostly attribute inputs and using the type enum as an opt-out. Once we do change to how input types are selected in a way that expands the node interface less we could review the default choices in general. But I would be interested what others think about this.
Yes the node can be quite tall because of the type selector enums.
I've changed the default vector input to use an attribute but left the option to change to vector.
I don't think useful defaults and functionality should be removed because of the UI issues.
Like you said, that can be addressed with a better UI design.
- moving the type enum selector next to the input or as part of right click menu.
- having options to minimise vector inputs
Other than a few minor comments, I think this looks good.
Also, I think the "Invert" input should be second to last, right before "Result". Before it was at the top because it was a property rather than a socket.
| source/blender/nodes/geometry/nodes/node_geo_attribute_vector_rotate.cc | ||
|---|---|---|
| 71 | You shouldn't need this. | |
| 100–140 | Put these at the top, so only the layout function and the inputs are above them. | |
| 209–210 | You can use attribute_get_meta_data here, which was recently added. | |
The code looks quite good, nice job. Also, with just of testing I can tell this node is really powerful! Just some final minor tweaks here.
Some warnings:
Warning: Call save() to make sure that changes persist in all cases.
| source/blender/nodes/geometry/nodes/node_geo_attribute_vector_rotate.cc | ||
|---|---|---|
| 53 | uiLayoutSetPropSep(layout, true); uiLayoutSetPropDecorate(layout, false); uiLayout *column = uiLayoutColumn(layout, false); Then, if you put everything in the column, the node will get a little bit shorter : ) | |
| 125 | Move this one too, so it's right below geo_node_attribute_vector_rotate_update (out of the way of all of the file's interesting logic). | |
| 209 | IMO there's no need for has_value, the std::optional right above makes the overridden bool conversion quite clear | |
| 305–306 | This looks a bit fishy here ("default" right above another "case"). Maybe just remove one of them? | |
| 346 | node_type_size(&ntype, 165, 100, 600); This makes the name fit by default and gives some more room for the type enums at the top. | |
| source/blender/nodes/geometry/nodes/node_geo_attribute_vector_rotate.cc | ||
|---|---|---|
| 305–306 | This is handled prior to the other modes to avoid processing the unavailable angle socket used by the other modes. | |
Address comments and fix warning. I was passing VMutableArray and saving instead of passing MutableSpan.
Apart from the related UI issue of type changing that we have to address separately this looks look to go for me now, will be nice to have this in master!
The code looks great!
| source/blender/nodes/geometry/nodes/node_geo_attribute_vector_rotate.cc | ||
|---|---|---|
| 325–327 | Maybe add a curve component check here? Just nice to have it so it's not forgotten, though it will only do anything later this week. | |
Pushed rB93933ee8bbed but accidentally removed the line referencing the diff.
Differential Revision: https://developer.blender.org/D11042