[Bf-codereview] Cycles shading nodes soc-2013 (issue 12177043)

[brechtvanlommel at gmail.com]

Overall looks good to me, see inline comments for things to fix.

The only thing that I don't think is ready to merge after fixes is the
non-progressive mode for CUDA. That needs to become a separate kernel
first.


https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_emission.h
File intern/cycles/kernel/kernel_emission.h (left):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_emission.h#oldcode45
intern/cycles/kernel/kernel_emission.h:45: eval =
shader_eval_background(kg, &sd, 0, SHADER_CONTEXT_EMISSION);
This should be bounce+1, same for the two calls to
shader_setup_from_sample later in this function.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_emission.h
File intern/cycles/kernel/kernel_emission.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_emission.h#newcode243
intern/cycles/kernel/kernel_emission.h:243:
shader_setup_from_background(kg, &sd, ray, bounce);
Make this bounce+1.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_path.h
File intern/cycles/kernel/kernel_path.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_path.h#newcode218
intern/cycles/kernel/kernel_path.h:218: shader_setup_from_ray(kg, &sd,
&isect, ray, state->bounce);
This should be state->bounce+1, as you're doing shading at the next
bounce, not the current shading point.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_shader.h
File intern/cycles/kernel/kernel_shader.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_shader.h#newcode959
intern/cycles/kernel/kernel_shader.h:959: #ifdef __KERNEL_GPU__
Remove this #ifdef and just use this loop for both CPU and GPU.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_types.h
File intern/cycles/kernel/kernel_types.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_types.h#newcode51
intern/cycles/kernel/kernel_types.h:51: #define BB_TABLE_SPACING 2.0
Align the values with spaces like for BSSRDF.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_types.h#newcode558
intern/cycles/kernel/kernel_types.h:558: float ray_depth;
Can you keep this as an int and do the cast to float when needed? Is a
tiny bit faster but also just makes more sense to have it as in I think,
only reason it needs to be cast to float is because svm doesn't support
integer sockets.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_types.h#newcode831
intern/cycles/kernel/kernel_types.h:831: typedef struct KernelBLACKBODY
{
Rename to KernelBlackbody without the all caps.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/osl/osl_services.cpp
File intern/cycles/kernel/osl/osl_services.cpp (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/osl/osl_services.cpp#newcode667
intern/cycles/kernel/osl/osl_services.cpp:667: return
set_attribute_float(f, type, derivatives, val);
This should be set_attribute_int.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/osl/osl_services.cpp#newcode928
intern/cycles/kernel/osl/osl_services.cpp:928: shader_setup_from_ray(kg,
sd, &tracedata->isect, &tracedata->ray, -1);
To get the bounce value here, you could do:

ShaderData *original_sd = (ShaderData *)(sg->renderstate);
bounce = original_sd->ray_depth + 1;

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/node_blackbody.osl
File intern/cycles/kernel/shaders/node_blackbody.osl (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/node_blackbody.osl#newcode29
intern/cycles/kernel/shaders/node_blackbody.osl:29: Color = rgb /= l;
Check for division by zero here:

float l = luminance(rgb);
if (l != 0.0)
     rgb /= l;
Color = rgb;

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/node_light_path.osl
File intern/cycles/kernel/shaders/node_light_path.osl (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/node_light_path.osl#newcode41
intern/cycles/kernel/shaders/node_light_path.osl:41:
getattribute("path:ray_depth", RayDepth);
Make this:

int ray_depth;
getattribute("path:ray_depth", ray_depth);
RayDepth = (float)ray_depth;

Kind of stupid but we must keep the sockets float for now, while at the
same time it's best to have integer values as integers.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/node_vector_transform.osl
File intern/cycles/kernel/shaders/node_vector_transform.osl (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/node_vector_transform.osl#newcode24
intern/cycles/kernel/shaders/node_vector_transform.osl:24: string
convert_to = "Object",
Can you make these lowercase "world" and "object". Not that they are
actually uses in practice but is nice if they match valid values.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/node_vector_transform.osl#newcode29
intern/cycles/kernel/shaders/node_vector_transform.osl:29: VectorOut =
transform(convert_from, convert_to, VectorIn);
Thinking about this there is a 3rd type that makes sense, "Normal",
which automatically normalizes after the transform. The code would be:

if (type == "Vector" || type == "Normal) {
     VectorOut = transform(convert_from, convert_to, VectorIn);
     if (type == "Normal")
         VectorOut = normalize(VectorOut);
}

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/node_vector_transform.osl#newcode32
intern/cycles/kernel/shaders/node_vector_transform.osl:32: point Point =
point(VectorIn[0], VectorIn[1], VectorIn[2]);
I think you can just cast like this?
point Point = (point)VectorIn;

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/svm/svm_blackbody.h
File intern/cycles/kernel/svm/svm_blackbody.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/svm/svm_blackbody.h#newcode52
intern/cycles/kernel/svm/svm_blackbody.h:52: const float
lookuptablesizef = 956.0f;
You can replace this with

const float lookuptablenormalize = 1.0f/956.0f;

and then do:

float lutval = t*lookuptablenormalize;

The compiler in general can't replace a division by a multiplication for
you because then it doesn't follow the IEEE specification, but we can do
it manually like this.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/svm/svm_blackbody.h#newcode56
intern/cycles/kernel/svm/svm_blackbody.h:56: float t = powf
((temperature - BB_DRAPPER) / BB_TABLE_SPACING, 1.0f/BB_TABLE_XPOWER);
Similar optimization here, you can make this:
float t = powf((temperature - BB_DRAPPER) * (1.0f / BB_TABLE_SPACING),
1.0f/BB_TABLE_XPOWER));

It can do the division compile time then and only do the multiplication
at runtime.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/svm/svm_blackbody.h#newcode77
intern/cycles/kernel/svm/svm_blackbody.h:77: color_rgb /= l;
Check for division by zero here like in OSL shader.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/svm/svm_vector_transform.h
File intern/cycles/kernel/svm/svm_vector_transform.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/svm/svm_vector_transform.h#newcode38
intern/cycles/kernel/svm/svm_vector_transform.h:38: int is_object =
(sd->object != ~0);
Can make this "bool is_object".

https://codereview.appspot.com/12177043/diff/1/intern/cycles/render/nodes.h
File intern/cycles/render/nodes.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/render/nodes.h#newcode517
intern/cycles/render/nodes.h:517: static ShaderEnum convert_to_enum;
convert_from_enum and convert_to_enum can be a single
convert_space_enum. They have the same values so there's no need to
define them twice.

https://codereview.appspot.com/12177043/diff/1/intern/cycles/render/svm.cpp
File intern/cycles/render/svm.cpp (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/render/svm.cpp#newcode395
intern/cycles/render/svm.cpp:395:
current_shader->has_converter_blackbody = true;
Can you move this check inside   if(inputs_done) {
Right before   node->compile(*this);

Doesn't matter much but seems more logical there.

https://codereview.appspot.com/12177043/diff/1/source/blender/blenkernel/BKE_node.h
File source/blender/blenkernel/BKE_node.h (right):

https://codereview.appspot.com/12177043/diff/1/source/blender/blenkernel/BKE_node.h#newcode811
source/blender/blenkernel/BKE_node.h:811: #define
RRES_OUT_SUBS_COLOR			30
Can you use SUBSURFACE instead of SUBS as abbreviation? SUBS is not so
obvious to me.

https://codereview.appspot.com/12177043/diff/1/source/blender/makesdna/DNA_node_types.h
File source/blender/makesdna/DNA_node_types.h (right):

https://codereview.appspot.com/12177043/diff/1/source/blender/makesdna/DNA_node_types.h#newcode882
source/blender/makesdna/DNA_node_types.h:882: #define
SHD_VECT_TRANSFORM_FROM_WORLD	0
Same comment here as in nodes.cpp, this can actually be a single enum
instead of having separate ones for to/from.

https://codereview.appspot.com/12177043/diff/1/source/blender/makesrna/intern/rna_nodetree.c
File source/blender/makesrna/intern/rna_nodetree.c (right):

https://codereview.appspot.com/12177043/diff/1/source/blender/makesrna/intern/rna_nodetree.c#newcode3527
source/blender/makesrna/intern/rna_nodetree.c:3527: static
EnumPropertyItem prop_vect_to_items[] = {
Same comment as before, this can be a single enum.

https://codereview.appspot.com/12177043/diff/1/source/blender/render/intern/source/render_result.c
File source/blender/render/intern/source/render_result.c (right):

https://codereview.appspot.com/12177043/diff/1/source/blender/render/intern/source/render_result.c#newcode283
source/blender/render/intern/source/render_result.c:283: if (channel ==
-1) return "SubsDir";
Can you use SubsurfaceDir here? There used to be a limit on the name
length but that is actually gone now.

https://codereview.appspot.com/12177043/