Ref T83668.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- temp-geometry-nodes-rotate-points (branched from master)
- Build Status
Buildable 11824 Build 11824: arc lint + arc unit
Event Timeline
Comment Actions
The code looks good, aside from the lack of IFACE and a suggestion.
I have a couple questions that head more in the design direction though:
- Should it be possible to use this node on other attributes? Maybe exposed only in the side bar? I could see people wanting to use it on other attributes like custom normals or velocities.
- It's feeling stranger that we can't do this kind of operation on the instances component. Maybe we should implement some limited form of the attribute access API for the InstancesComponent
- "Point Rotate" is more consistent with the existing naming pattern. Though if we switch to that it feels like we're prioritizing consistency a bit too much.
| source/blender/editors/space_node/drawnode.c | ||
|---|---|---|
| 3248 | Don't forget IFACE_( | |
| source/blender/nodes/geometry/node_geometry_util.cc | ||
| 25 | The name for this confused me for a moment because it implies some dependency on other things happening in this function, but it's really just a "yes or no" input. This function could probably just use some documentation, since the need for it is tied to the weird type toggles. How's this? /** * Update the availability of a group of input sockets with the same name, * used for switching between attribute inputs or single values. * * \param mode: Controls which socket of the group to make available. * \param can_be_available: If false, make all sockets with this name unavailable. */ | |
Comment Actions
Should it be possible to use this node on other attributes? Maybe exposed only in the side bar? I could see people wanting to use it on other attributes like custom normals or velocities.
Normals and velocities are usually not stored as eulers, which is what this node is operating on. Besides that, I agree. It's not too hard to imagine cases where it would be useful to change other attributes with this node. However, currently we need this for the rotation attribute and decided that we do not expose attribute inputs for the "builtin" attributes. This discussion applies to many other nodes as well. I don't think this should be discussed as part of this patch. Fortunately, it will be very easy to add these kinds of attribute inputs later on if we decide to do so.
It's feeling stranger that we can't do this kind of operation on the instances component. Maybe we should implement some limited form of the attribute access API for the InstancesComponent.
Adding an attribute api for the instances component would be quite easy, but it conflicts with the design that instancing should be an implementation detail. I don't necessarily agree with this design, but we should not make it harder/impossible to implement it. Again, this discussion is out of scope for this specific patch imo.
"Point Rotate" is more consistent with the existing naming pattern. Though if we switch to that it feels like we're prioritizing consistency a bit too much.
I'd probably use the name "Rotate Points" instead of "Point Rotate" . This kind of consistency might work well now, but I do think it will break down at some point, when more complex nodes don't fit into such categories anymore. To me it seems like nodes should have names that describe an action, because they are functions after all. So "Rotate Points", "Randomize Attribute", "Transform Geometry" or "Instance on Points" do make sense to me (note that they all start with a verb). That's just my opinion though, I'll probably leave the node naming to the ui dept. I realize that these names have little consistency, other than the fact that they sound more natural. Consistency mainly seems to be important to be able to find nodes, but it seems like a better menu and search system would solve this, even without making node names sound less natural.
| source/blender/nodes/geometry/node_geometry_util.cc | ||
|---|---|---|
| 25 | Sounds good. | |
Comment Actions
Thanks for the thoughts, they make sense to me, especially the point about the naming.
We probably should revisit the instances component design then, because if they were an implementation detail then users would probably expect to be able to rotate them, just like they can be transformed with the transform node. But yeah, out of scope of this patch.
Comment Actions
Missed this during the review, but we should consider renaming this node from Rotate Points to Point Rotate. Following the naming convention of other Point nodes as well as other Rotate nodes.
I guess it's already decided, just wanted to leave my point of view on this. If all nodes in Blender are already going in one direction, we should continue on that direction or rename all other nodes. Having some written in one way and some in another way (in Geometry Nodes and other node systems), will make it confusing.