[Bf-codereview] Particle Info node for Cycles (issue 6242069)

[brechtvanlommel at gmail.com]

Thanks for the patch, node setups with this kind of data could be pretty
powerful. Comments below.


http://codereview.appspot.com/6242069/diff/1/intern/cycles/blender/blender_object.cpp
File intern/cycles/blender/blender_object.cpp (left):

http://codereview.appspot.com/6242069/diff/1/intern/cycles/blender/blender_object.cpp#oldcode325
intern/cycles/blender/blender_object.cpp:325: bool render_emitter =
false;
This code can't be simplified like this. The intention is make existence
of a particle system hide the emitter, not only to unhide it if
use_render_emitter is on.

http://codereview.appspot.com/6242069/diff/1/intern/cycles/blender/blender_object.cpp
File intern/cycles/blender/blender_object.cpp (right):

http://codereview.appspot.com/6242069/diff/1/intern/cycles/blender/blender_object.cpp#newcode274
intern/cycles/blender/blender_object.cpp:274: sync_particles(object,
b_ob);
This should be done only on demand, by adding a
ParticleInfoNode::attributes(..) callback, which adds a
ATTR_STD_PARTICLE request, which can then be checked here with
object->mesh->need_attribute(scene, ATTR_STD_PARTICLE);

This is usually for mesh element attributes but actually was intended to
work for requesting per object attributes as well.

http://codereview.appspot.com/6242069/diff/1/intern/cycles/blender/blender_object.cpp#newcode329
intern/cycles/blender/blender_object.cpp:329: int particle_index =
(b_index > num_particles ? 0 : b_index + particle_offset);
This is not going to work for particles inside dupligroups, trying to
match these indices is not reliable. I think the proper solution is to
put the particle index in the DupliObject.

http://codereview.appspot.com/6242069/diff/1/intern/cycles/blender/blender_sync.cpp
File intern/cycles/blender/blender_sync.cpp (right):

http://codereview.appspot.com/6242069/diff/1/intern/cycles/blender/blender_sync.cpp#newcode83
intern/cycles/blender/blender_sync.cpp:83: if(b_ob->is_updated() ||
b_ob->is_updated_data()) {
Why is this needed?

http://codereview.appspot.com/6242069/diff/1/intern/cycles/blender/blender_util.h
File intern/cycles/blender/blender_util.h (right):

http://codereview.appspot.com/6242069/diff/1/intern/cycles/blender/blender_util.h#newcode277
intern/cycles/blender/blender_util.h:277: if(parent.ptr.data) {
This change can be reverted.

http://codereview.appspot.com/6242069/diff/1/intern/cycles/render/object.cpp
File intern/cycles/render/object.cpp (right):

http://codereview.appspot.com/6242069/diff/1/intern/cycles/render/object.cpp#newcode213
intern/cycles/render/object.cpp:213: object_map[offset_map] =
make_uint4(ob->pass_id, ob->random_id, ob->particle_id, 0);
Why not stick the particle_id in the make_float4 above? __int_as_float
and __float_as_int can be used to avoid any precision loss.

The loss of precision for pass_id is not a problem because it ends up in
a float buffer or as a float in shader nodes anyway.

I try to not use too many textures because OpenCL/CUDA can only handle a
limited number of arrays.

http://codereview.appspot.com/6242069/diff/1/intern/cycles/render/object.cpp#newcode286
intern/cycles/render/object.cpp:286: /* create a dummy 1-particle
texture to avoid invalid lookups */
This avoids invalid lookups in case you have no objects with particles,
but the particle info node is still used.

But for the case you have some objects with particles, and another
object with a particle info node but no particles, it will still do an
invalid lookup. Maybe the first element in the arrays should always be
this dummy element?

http://codereview.appspot.com/6242069/