Page MenuHome

Cycles: Vector Rotate Node using Axis and Angle method
Needs ReviewPublic

Authored by Charlie Jolly (charlie) on Oct 13 2018, 6:37 PM.

Details

Summary

Change rotation method to axis angle

Previous method added as an option to new mapping node.

void mapping_rotate(vec3 vector, vec3 location, vec3 rotation, vec3 scale, out vec3 result)
{
  result = (euler_to_mat3(rotation) * (vector - location) + location) * scale;
}

--Old description--
This provides ability to rotate a vector around an origin point and also provide access to the XYZ values via sockets.
Together with D3786 Vector Multipy, this provides sockets and an alternative to the mapping node.
Like the mapping node it uses XYZ order and this could be expanded in future to support other rotation modes.

Diff Detail

Repository
rB Blender
Branch
master
Build Status
Buildable 4826
Build 4826: arc lint + arc unit

Event Timeline

Charlie Jolly (charlie) updated this revision to Diff 17102.

Updated patch so it builds with master

I think you should take care of those remarks first.

intern/cycles/kernel/shaders/node_vector_rotate.osl
51

Are you sure this works? I would expect this to fail due to bad parameters. Make sure they match the node definition.

intern/cycles/kernel/svm/svm_vector_rotate.h
50

The output is always stack assigned, so this is probably redundant.

intern/cycles/render/nodes.cpp
5570

Even if all inputs are constants, that doesn't mean that we can fold to (x, y, z). You would need to transform the inputs vectors just as you did in the SVM code.

5588

Those should be stack_assign_if_linked.

source/blender/makesdna/DNA_node_types.h
904

Why do you need a DNA struct for this node? The data is stored in sockets and the node have no properties, so you don't need custom storage.

@Omar Ahmad (OmarSquircleArt) Thanks. I'll address the comments. Some of that was probably due to copying code from another node.

Other minor remarks.

intern/cycles/kernel/shaders/node_vector_rotate.osl
21

Maybe it would be better if you just output the matrix directly.

25

Is there a reason why you opted to suffix the variables with ijh? Maybe cx, cy, cz is better?

47

Alternatively, you can construct the matrix using the constructor matrix(1.0). This makes an identity matrix, so you wouldn't need those two lines.

intern/cycles/kernel/svm/svm_vector_rotate.h
44

You don't need to do matrix multiplication here. Implement translation as addition directly.

source/blender/gpu/shaders/gpu_shader_material.glsl
2247 ↗(On Diff #17102)

No need to use mat4 here, mat3 would suffice. Also, you can just return the matrix directly, no need to use inout.

I wounder if we can implement this using graph expansion. It would expand to two nodes I think, a Vector Math node and a Mapping node. I used expansion before and it was very nice and easy to use. Maybe you should give it a try. Check the following commit for reference rB7a7eadaf.

This wouldn't work with the mapping node as this exposes rotation values as socket inputs.

This wouldn't work with the mapping node as this exposes rotation values as socket inputs.

I was talking about using the dynamic mapping node from my branch. It will probably land in master this week, so we can utilize it.

Charlie Jolly (charlie) marked 10 inline comments as done.Aug 14 2019, 6:35 PM
Charlie Jolly (charlie) added inline comments.
intern/cycles/kernel/shaders/node_vector_rotate.osl
21

Function copied from math_rotation.c.
Renamed to eul_to_mat4.

25

This is taken from eul_to_mat4 in math_rotation.c

51

It works. Matches the names in nodes.cpp.

intern/cycles/kernel/svm/svm_vector_rotate.h
44

this is different because transform_point doesn't work the same in cycles as it does in glsl

intern/cycles/render/nodes.cpp
5570

This was copied from the Combine nodes that use the same code. Not sure I understand this.

5588

Crashes if this is changed to stack_assign_if_linked

source/blender/gpu/shaders/gpu_shader_material.glsl
2247 ↗(On Diff #17102)

Function copied from math_rotation.c.
Renamed to eul_to_mat4.

Charlie Jolly (charlie) marked 7 inline comments as done.Aug 14 2019, 6:36 PM
Charlie Jolly (charlie) updated this revision to Diff 17126.

Address comments from @Omar Ahmad (OmarSquircleArt)

@Omar Ahmad (OmarSquircleArt) I've kept the eul_to_mat4 the same as in maths_rotation.c. OSL uses mat4 so I've tried to keep all the code as consistent as possible.

intern/cycles/kernel/shaders/node_vector_rotate.osl
21

C code is different from OSL and GLSL code, I think we should just return the matrix directly, no need to use output parameters.

25

I don't think there is a code convention here, so just do what you think is more readable. You don't have to stick to ijh.

intern/cycles/kernel/svm/svm_vector_rotate.h
44

Sure, but that still doesn't mean it should be done this way.

intern/cycles/render/nodes.cpp
5570

This line basically says that if all inputs are constants, that is, not linked to other sockets, then we can reduce the node to a constant value. You compute this value as make_float3(x, y, z), which is obviously not correct. To correct this, simply rotate the point just as you did in the SVM code.

5588

If you don't assign if linked then the second node will be useless since the inputs will always be stack assigned. Find out why Blender crashes and try to fix it.

source/blender/gpu/shaders/gpu_shader_material.glsl
2247 ↗(On Diff #17102)

Well there is also a eul_to_mat3 function in the same file, not sure what the reasoning here is.

source/blender/nodes/shader/nodes/node_shader_vectRotate.c
54 ↗(On Diff #17126)

node_type_storage is not needed in this case.

Charlie Jolly (charlie) marked 7 inline comments as done.Aug 15 2019, 2:24 PM
Charlie Jolly (charlie) added inline comments.
intern/cycles/kernel/shaders/node_vector_rotate.osl
21

I've made this a return function and changed name to eul_to_matrix

25

Because the code is copied I think it's better to keep the variable names as this helps if the codebase is searched.

intern/cycles/kernel/svm/svm_vector_rotate.h
44

Simplified to match OSL and GLSL code. Remove call to transform_translate.

intern/cycles/render/nodes.cpp
5570

I've removed the constant folding. I've noticed a lot of nodes do not have this.

5588

This is unclear to me why this crashes. Take a look at the CombineXYZNode and VectorTransformNode, they are coded the same way.

source/blender/gpu/shaders/gpu_shader_material.glsl
2247 ↗(On Diff #17102)

Simplified glsl to use mat3 and eul_to_mat3 which is identical to eul_to_mat4 but without the additional two lines at the end. Also used return function.

Charlie Jolly (charlie) marked 6 inline comments as done.Aug 15 2019, 2:25 PM
Charlie Jolly (charlie) updated this revision to Diff 17156.

Cleanup and address comments from @Omar Ahmad (OmarSquircleArt) - thanks

Charlie Jolly (charlie) updated this revision to Diff 17274.EditedMon, Aug 19, 4:57 PM

Change XYZ sockets to a single vector socket.

This comment has been deleted.
intern/cycles/kernel/svm/svm.h
504

I like passing the uints of the node directly like:
svm_node_vector_rotate(kg, sd, stack, node.y, node.z, &offset);
The reason I like this is because we can use explicit names in the function parameters like:

ccl_device void svm_node_vector_rotate(KernelGlobals *kg,
                                       ShaderData *sd,
                                       float *stack,
                                       uint inputs_stack_offsets,
                                       uint result_stack_offset,
                                       int *offset)
intern/cycles/kernel/svm/svm_vector_rotate.h
25

A naming convention I like to use is *_stack_offset, for instance, vector_stack_offset, origin_stack_offset, rotation_stack_offset, and result_stack_offset. This helps distinguishing sockets, stack offsets, and global offsets.

36

I think you should create a new variable instead of overwriting the input this many times:
float3 result = transform_point(&rmat, vector - origin) + origin;
The same goes for other backends.

intern/cycles/render/nodes.cpp
5798

No need to encode the output using compiler.encode_uchar4, just pass it directly.

source/blender/nodes/shader/nodes/node_shader_vectRotate.c
30 ↗(On Diff #17274)

I think it is better to call this Rotation, like in the rest of Blender.

Charlie Jolly (charlie) marked 5 inline comments as done.Mon, Aug 19, 6:42 PM

Rename and tidy up with comments from @Omar Ahmad (OmarSquircleArt)

Variable renaming for better readability

Missed one function name in last changes

Keep patch up to date

Change rotation method to axis angle

Add previous method as an option to new mapping node

Charlie Jolly (charlie) retitled this revision from Cycles & Eevee Vector Rotate Node: Standalone node with access to XYZ sockets. to Cycles: Vector Rotate Node using Axis and Angle method.Sat, Sep 7, 5:16 PM
Charlie Jolly (charlie) edited the summary of this revision. (Show Details)
Charlie Jolly (charlie) edited the summary of this revision. (Show Details)