Cycles shading nodes soc-2013

Cycles shading nodes soc-2013

[brechtvl, 2013-07-31 17:22:17]

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_em...
File intern/cycles/kernel/kernel_emission.h (left):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_em...
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_em...
File intern/cycles/kernel/kernel_emission.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_em...
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_pa...
File intern/cycles/kernel/kernel_path.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_pa...
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_sh...
File intern/cycles/kernel/kernel_shader.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_sh...
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_ty...
File intern/cycles/kernel/kernel_types.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/kernel_ty...
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_ty...
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_ty...
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_s...
File intern/cycles/kernel/osl/osl_services.cpp (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/osl/osl_s...
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_s...
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/n...
File intern/cycles/kernel/shaders/node_blackbody.osl (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/n...
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/n...
File intern/cycles/kernel/shaders/node_light_path.osl (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/n...
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/n...
File intern/cycles/kernel/shaders/node_vector_transform.osl (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/shaders/n...
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/n...
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/n...
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_b...
File intern/cycles/kernel/svm/svm_blackbody.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/svm/svm_b...
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_b...
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_b...
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_v...
File intern/cycles/kernel/svm/svm_vector_transform.h (right):

https://codereview.appspot.com/12177043/diff/1/intern/cycles/kernel/svm/svm_v...
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#n...
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#n...
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_...
File source/blender/blenkernel/BKE_node.h (right):

https://codereview.appspot.com/12177043/diff/1/source/blender/blenkernel/BKE_...
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_no...
File source/blender/makesdna/DNA_node_types.h (right):

https://codereview.appspot.com/12177043/diff/1/source/blender/makesdna/DNA_no...
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...
File source/blender/makesrna/intern/rna_nodetree.c (right):

https://codereview.appspot.com/12177043/diff/1/source/blender/makesrna/intern...
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/s...
File source/blender/render/intern/source/render_result.c (right):

https://codereview.appspot.com/12177043/diff/1/source/blender/render/intern/s...
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.