Normal Node for Cycles

Hi Brecht,

I wanted to tackle a more complicated node and gave Normal Node a try.
(and failed *cough* ;)

I created a new NodeNormal for storage of the Normal, and whatever I put as default in the node_shader_normal.c::node_shader_init_normal() is passed to the shader correctly.

The problem now is to hook up the interface normal values to the cycles storage node.

Maybe I don't need a storage node and could use the bNodeSocketValueVector directly in blender_shader.cpp. I don't know if it's possible (and the way to go).

Another idea was to create a custom get function in the RNA def_normal for the direction and point this to the bNodeSocketValueVector. Again, I don't know if is the correct way or even possible.

FInal Notes:
---------
My ultimate goal was to port the RGBCurves node to cycles, so I decided to start with a 'similar' but simpler one. I used the SkyTexture for some of the

Also, I was testing with a sample file created before I added the storage node in node_shader_init_normal. That was crashing on blender_shader.cpp (get_float3(b_normal_node.direction());) and I couldn't figure out why. Luckily I had the insight of remove the node and create it again :)

Thanks for any feedback and review.
OSL comes once those problems are sorted out.

#committed on 42669

[zanqdo_gmail.com, 2011-12-04 20:09:23]

go dalai go!

just saying :D

Daniel Salazar
3Developer.com



On Sun, Dec 4, 2011 at 3:01 AM,  <dfelinto@gmail.com> wrote:
> Reviewers: bf-codereview_blender.org, brechtvl,
>
> Description:
> Hi Brecht,
>
> I wanted to tackle a more complicated node and gave Normal Node a try.
> (and failed *cough* ;)
>
> I created a new NodeNormal for storage of the Normal, and whatever I put
> as default in the node_shader_normal.c::node_shader_init_normal() is
> passed to the shader correctly.
>
> The problem now is to hook up the interface normal values to the cycles
> storage node.
>
> Maybe I don't need a storage node and could use the
> bNodeSocketValueVector directly in blender_shader.cpp. I don't know if
> it's possible (and the way to go).
>
> Another idea was to create a custom get function in the RNA def_normal
> for the direction and point this to the bNodeSocketValueVector. Again, I
> don't know if is the correct way or even possible.
>
> FInal Notes:
> ---------
> My ultimate goal was to port the RGBCurves node to cycles, so I decided
> to start with a 'similar' but simpler one. I used the SkyTexture for
> some of the
>
> Also, I was testing with a sample file created before I added the
> storage node in node_shader_init_normal. That was crashing on
> blender_shader.cpp (get_float3(b_normal_node.direction());) and I
> couldn't figure out why. Luckily I had the insight of remove the node
> and create it again :)
>
> Thanks for any feedback and review.
> OSL comes once those problems are sorted out.
>
> Please review this at http://codereview.appspot.com/5440091/
>
> Affected files:
>   intern/cycles/app/cycles_xml.cpp
>   intern/cycles/blender/blender_shader.cpp
>   intern/cycles/kernel/CMakeLists.txt
>   intern/cycles/kernel/svm/svm.h
>   intern/cycles/kernel/svm/svm_normal.h
>   intern/cycles/kernel/svm/svm_types.h
>   intern/cycles/render/nodes.cpp
>   intern/cycles/render/nodes.h
>   source/blender/makesdna/DNA_node_types.h
>   source/blender/makesrna/intern/rna_nodetree.c
>   source/blender/makesrna/intern/rna_nodetree_types.h
>   source/blender/nodes/shader/nodes/node_shader_normal.c
>
>
> _______________________________________________
> Bf-codereview mailing list
> Bf-codereview@blender.org
> http://lists.blender.org/mailman/listinfo/bf-codereview

[dfelinto, 2011-12-16 00:49:14]

Final Patch, removing DNA and RNA as per Lukas orientation. Notes in code coming
next

[dfelinto, 2011-12-16 00:55:33]

the patch is trunk ready imho.

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/blender/blender...
File intern/cycles/blender/blender_shader.cpp (right):

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/blender/blender...
intern/cycles/blender/blender_shader.cpp:208: node = norm;
one option here is to create a get_node_output_normal() to move the code from
here (alas what we do for Value and RGB nodes). It works fine the way it's
though

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/osl/node...
File intern/cycles/kernel/osl/nodes/node_normal.osl (right):

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/osl/node...
intern/cycles/kernel/osl/nodes/node_normal.osl:24: output normal NormalOut =
normal(0.0, 0.0, 0.0),
small remark that this (as some of other nodes) need some auto renaming in order
to work.

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/osl/node...
intern/cycles/kernel/osl/nodes/node_normal.osl:30: Dot = -dot(Direction,
NormalIn);
following blender original code (returning - dot). results match.

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/svm/svm_...
File intern/cycles/kernel/svm/svm_normal.h (right):

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/svm/svm_...
intern/cycles/kernel/svm/svm_normal.h:27: float3 direction;
I couldn't find a way to pass the whole float3, so it came in parts (see
nodes.cpp)

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/svm/svm_...
intern/cycles/kernel/svm/svm_normal.h:38: stack_store_float(stack,
out_dot_offset, -dot(direction, normalize(normal)));
(same as OSL )following blender original code (returning - dot). results match.

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/render/nodes.cpp
File intern/cycles/render/nodes.cpp (right):

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/render/nodes.cp...
intern/cycles/render/nodes.cpp:743: }
Is there a way to pass the whole float3 as uint4?

[brechtvl, 2011-12-16 16:13:26]

Some comments, if those are fixed, LGTM.

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/osl/node...
File intern/cycles/kernel/osl/nodes/node_normal.osl (right):

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/osl/node...
intern/cycles/kernel/osl/nodes/node_normal.osl:27: direction =
normalize(Direction);
direction => Direction

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/osl/node...
intern/cycles/kernel/osl/nodes/node_normal.osl:30: Dot = -dot(Direction,
NormalIn);
The dot product should not be negated, normals do not point inside in cycles.

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/svm/svm_...
File intern/cycles/kernel/svm/svm_normal.h (right):

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/kernel/svm/svm_...
intern/cycles/kernel/svm/svm_normal.h:38: stack_store_float(stack,
out_dot_offset, -dot(direction, normalize(normal)));
Same here, normal should not be negated.

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/render/nodes.cpp
File intern/cycles/render/nodes.cpp (right):

http://codereview.appspot.com/5440091/diff/5001/intern/cycles/render/nodes.cp...
intern/cycles/render/nodes.cpp:743: }
No, the code used seems ok.