Details

Reviewers

Commits

rBe12ad2bce0cc: Geometry Nodes: support Vector Rotate node

Summary

This patch makes the Vector Rotate node from the shader compatible with Geometry Nodes.

Use Case
This node allows for easy rotation of direction based attribute falloffs like linear or radial gradients either directly with the node or controlled with an empty.

Example
Linear Attribute gradient rotated by an empty:

geometry-nodes-vector-rotate-node-test.blend907 KBDownload

Diff Detail

Repository

rB Blender

Branch

geometry-nodes-port-vector-rotate-node (branched from master)

Build Status

Buildable 12866
Build 12866: arc lint + arc unit

Event Timeline

Leon Schittek (lone_noel) requested review of this revision.Feb 12 2021, 9:04 PM

Leon Schittek (lone_noel) created this revision.

Harbormaster completed remote builds in B12866: Diff 33946.Feb 12 2021, 9:04 PM

Leon Schittek (lone_noel) edited the summary of this revision. (Show Details)Feb 12 2021, 9:17 PM

Victor-Louis De Gusseme (victorlouis) awarded a token.Feb 12 2021, 10:41 PM

Florian Kamenz (TimBrown) awarded a token.Feb 13 2021, 10:51 AM

Charlie Jolly (charlie) added projects: Geometry Nodes, BF Blender.Feb 17 2021, 1:25 PM

Leon Schittek (lone_noel) mentioned this in D10355: Geometry Nodes: Add Invert option to the Transform node.Feb 17 2021, 6:13 PM

Expose Input sliders for the Vector input.

Harbormaster completed remote builds in B13024: Diff 34207.Feb 20 2021, 10:21 AM

Leon Schittek (lone_noel) edited the summary of this revision. (Show Details)Feb 20 2021, 11:05 AM

Leon Schittek (lone_noel) retitled this revision from Geometry Nodes: Implement Vector Rotate node (WIP) to Geometry Nodes: Implement Vector Rotate node.Feb 21 2021, 9:12 PM

Leon Schittek (lone_noel) edited the summary of this revision. (Show Details)

higgsas (higgsas) awarded a token.Feb 25 2021, 6:10 PM

Jacques Lucke (JacquesLucke) added a reviewer: Jacques Lucke (JacquesLucke).Mar 2 2021, 11:47 AM

Works great for me with current master 👍

Jacques Lucke (JacquesLucke) requested changes to this revision.Mar 2 2021, 1:01 PM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/nodes/shader/nodes/node_shader_vector_rotate.cc
48	This is not valid c++ (it might compile on linux): https://godbolt.org/z/1bdxYG Better we convert the file to c++ in a separate patch, before adding features to it.
86	Obviously, in common cases where the rotation is the same for all vectors, this is quite inefficient. This could be improved by implementing a proper multi-function (instead of using `CustomMF_SI_SI_SI_SI_SO`, but both approaches are fine for me for now.

This revision now requires changes to proceed.Mar 2 2021, 1:01 PM

Rahul Chaudhary (RC12) added a subscriber: Rahul Chaudhary (RC12).Mar 3 2021, 6:07 AM

Fix invalid C++

Harbormaster completed remote builds in B13220: Diff 34701.Mar 3 2021, 12:04 PM

Great that it’s working for you, @Simon Thommes (simonthommes) !
What do you think about the exposed Vector input? You’re pretty much using the node how I imagined it would be and in those cases I figured it’d be nice not having to add a vector input node.
Just wanna make sure everybody is on board with this.

@Jacques Lucke (JacquesLucke) I’ve only updated the patch to fix my c++ blunder. I’ll have time to more thoroughly address your feedback this weekend.

Does exposing the Vector socket inputs require versioning? Currently it won’t expose the inputs for the Vector socket on existing Vector Rotate nodes in shaders. This doesn’t break anything but it might be weird.
Would it be better to make a separate patch for that, as well, to keep things more clean?

In D10410#267828, @Leon Schittek (lone_noel) wrote:

Great that it’s working for you, @Simon Thommes (simonthommes) !
What do you think about the exposed Vector input? You’re pretty much using the node how I imagined it would be and in those cases I figured it’d be nice not having to add a vector input node.
Just wanna make sure everybody is on board with this.

Thanks for the update!
I thought about this question before and I'm not exactly sure. I do think that the node feels quite bloated with 3 exposed input vectors. The shader node also had this initially, I can't find the decision to change this. But I do think it makes sense to hide it. Partly to emphasize that this is the main input that is being altered and to encourage the user to deliberately create an input node for use-cases like this. That helps with readibility, it makes sense to separate the operation from the input (the other inputs are part of the operation).

So I'm leaning towards following the shader node with this decision, even though I can see why you made yours differently and I can see the benefit of this approach.
If we go with a hidden input, at least Geometry Nodes do have a dedicated Vector Input node and don't have to go the way of Combine XYZ :D

In D10410#267913, @Simon Thommes (simonthommes) wrote:
But I do think it makes sense to hide it. Partly to emphasize that this is the main input that is being altered and to encourage the user to deliberately create an input node for use-cases like this. That helps with readibility, it makes sense to separate the operation from the input (the other inputs are part of the operation).

This is pretty much what made me unsure about the change and the way you put it makes a very clear case fo hiding the input. I'll go back to hiding the input with the next update.
Thanks for the feedback! :)

I moved the node to c++ in master, so you don't have to do that as part of a separate patch anymore.

Rebase on current master
Hide Vector input socket again

Harbormaster completed remote builds in B13306: Diff 34855.Mar 7 2021, 9:14 PM

Leon Schittek (lone_noel) edited the summary of this revision. (Show Details)Mar 7 2021, 9:20 PM

@Jacques Lucke (JacquesLucke) Thanks a lot for doing the c++ conversion! :)

source/blender/nodes/shader/nodes/node_shader_vector_rotate.cc
86	I don't think there currently is a way to actually use this node for multiple vectors, right? So for now I've kept the implementation with `CustomMF_SI_SI_SI_SI_SO`.

I still think the code can be simplified a bit to make it less redundant. But overall it looks good now. Will merge it in a moment with minor changes to make clang-tidy happy (else-after-return).

This revision is now accepted and ready to land.Mar 8 2021, 11:36 AM

Closed by commit rBe12ad2bce0cc: Geometry Nodes: support Vector Rotate node (authored by Leon Leno <lone_noel>, committed by Jacques Lucke (JacquesLucke)). · Explain WhyMar 8 2021, 11:40 AM

This revision was automatically updated to reflect the committed changes.

Jacques Lucke (JacquesLucke) added a commit: rBe12ad2bce0cc: Geometry Nodes: support Vector Rotate node.

Damian (damian) awarded a token.Mar 8 2021, 2:53 PM

Charlie Jolly (charlie) mentioned this in T86749: Geometry Node Community Additions.Mar 19 2021, 10:40 PM

Geometry Nodes: Implement Vector Rotate node
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 33946

release/scripts/startup/nodeitems_builtins.py

source/blender/functions/FN_multi_function_builder.hh

source/blender/nodes/CMakeLists.txt

source/blender/nodes/shader/nodes/node_shader_vector_rotate.c

source/blender/nodes/shader/nodes/node_shader_vector_rotate.cc

Geometry Nodes: Implement Vector Rotate nodeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 33946

release/scripts/startup/nodeitems_builtins.py

source/blender/functions/FN_multi_function_builder.hh

source/blender/nodes/CMakeLists.txt

source/blender/nodes/shader/nodes/node_shader_vector_rotate.c

source/blender/nodes/shader/nodes/node_shader_vector_rotate.cc

Geometry Nodes: Implement Vector Rotate node
ClosedPublic
Actions

Revision Contents
Changeset List