[Bf-codereview] Node changes from Particles Branch (issue4476050)

[brechtvanlommel at gmail.com]

Some more comments, some a bit nitpicky. Still need to review node.c and
space_node code.


http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE_node.h
File source/blender/blenkernel/BKE_node.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE_node.h#newcode78
source/blender/blenkernel/BKE_node.h:78: struct bNodeDataSource;
Some unecessary declarations here: StructRNA, PropertyRNA, DerivedMesh,
ParticleSystem, bNodeDataReader, bNodeDataSource.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE_node.h#newcode193
source/blender/blenkernel/BKE_node.h:193: void (*edit_clear)(struct
bNode *node);
Would be good to have a group_ prefix here for clarity.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE_node.h#newcode255
source/blender/blenkernel/BKE_node.h:255: char id_name[24];				/* id
name for RNA identification */
For consistency, this should be "char idname[64];"

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE_node.h#newcode262
source/blender/blenkernel/BKE_node.h:262: void (*foreachNodeTree)(struct
Main *main, void *calldata, bNodeTreeCallback func);		/* iteration over
all node trees */
There some inconsistency here in callback names, some are camel case,
some not.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/intern/texture.c
File source/blender/blenkernel/intern/texture.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/intern/texture.c#newcode772
source/blender/blenkernel/intern/texture.c:772: tex->nodetree->execdata
= NULL;
Setting execdata to NULL is not done in other places, but would prefer
this instead of doing it using the backpointer everywhere.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/intern/readfile.c
File source/blender/blenloader/intern/readfile.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/intern/readfile.c#newcode143
source/blender/blenloader/intern/readfile.c:143: #include "RNA_types.h"
Unnecessary #includes.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/intern/readfile.c#newcode145
source/blender/blenloader/intern/readfile.c:145: #include
"intern/node_socket.h"
Would rather avoid this kind of include of files from intern/, if it
needs to call things in the nodes module a public header could be made?

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/intern/readfile.c#newcode2192
source/blender/blenloader/intern/readfile.c:2192:
Can't see execdata being set to NULL here. Not sure it can actually
happen with node trees now being copied for thread safety, but probably
good to do anyway?

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/intern/readfile.c#newcode11727
source/blender/blenloader/intern/readfile.c:11727: /* Initialize texture
point density curve falloff */
This texture point density code is nested differently than in trunk,
seems like a merge error?

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/intern/readfile.c#newcode11747
source/blender/blenloader/intern/readfile.c:11747: /* Convert default
socket values from bNodeStack */
This conversion should be behind a subversion check, now
do_versions_socket_default_value set default_value even if it's already
there.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/intern/writefile.c
File source/blender/blenloader/intern/writefile.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/intern/writefile.c#newcode158
source/blender/blenloader/intern/writefile.c:158: #include
"RNA_access.h"
Unnecessary #include.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/intern/writefile.c#newcode666
source/blender/blenloader/intern/writefile.c:666: writestruct(wd, DATA,
stype->value_structname, 1, sock->default_value);
Perhaps a write_node_socket utility to avoid duplicated code would be
good.

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/SConscript
File source/blender/editors/SConscript (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/SConscript#newcode41
source/blender/editors/SConscript:41: 'particleset/SConscript'])
Particle leftover.

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_api/spacetypes.c
File source/blender/editors/space_api/spacetypes.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_api/spacetypes.c#newcode66
source/blender/editors/space_api/spacetypes.c:66: #include
"ED_particleset.h"
Particle leftover.

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_node/node_header.c
File source/blender/editors/space_node/node_header.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_node/node_header.c#newcode190
source/blender/editors/space_node/node_header.c:190: uiItemV(layout, "--
new group --", 0, -NODE_GROUP);
Label here should be changed, -- and lowercase should not be used, just
call it "New Group", "New For Loop", "New While Loop"?

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_node/space_node.c
File source/blender/editors/space_node/space_node.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_node/space_node.c#newcode217
source/blender/editors/space_node/space_node.c:217: break;
Seems to be a modifiers nodes leftover.

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/transform/transform_conversions.c
File source/blender/editors/transform/transform_conversions.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/transform/transform_conversions.c#newcode5146
source/blender/editors/transform/transform_conversions.c:5146:
//		td->flag |= TD_NO_LOC;
Could get rid of these two commented lines.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesrna/intern/rna_internal.h
File source/blender/makesrna/intern/rna_internal.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesrna/intern/rna_internal.h#newcode161
source/blender/makesrna/intern/rna_internal.h:161: void
RNA_def_particleset(struct BlenderRNA *brna);
Some more particle leftovers in this file.

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/SConscript
File source/blender/nodes/SConscript (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/SConscript#newcode9
source/blender/nodes/SConscript:9: #parsources =
env.Glob('intern/PAR_nodes/*.c')
Some leftover code from modifiers/particles.

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/SHD_node.h
File source/blender/nodes/SHD_node.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/SHD_node.h#newcode48
source/blender/nodes/SHD_node.h:48: void
register_node_type_group_shader(ListBase *lb);
Consistency nitpick, these functions could be named:
register_node_type_sh_group

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/SHD_nodetree.c
File source/blender/nodes/intern/SHD_nodetree.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/SHD_nodetree.c#newcode139
source/blender/nodes/intern/SHD_nodetree.c:139: /* XXX TODO */
Seems like this should be fixed before committing.

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/node_common.h
File source/blender/nodes/intern/node_common.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/node_common.h#newcode36
source/blender/nodes/intern/node_common.h:36: struct bNodeSocket
*group_add_extern_socket(struct bNodeTree *ntree, ListBase *lb, int
in_out, struct bNodeSocket *gsock);
Perhaps the functions here could get a node_ prefix? Otherwise
potentially confusing with object groups.

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/node_socket.c
File source/blender/nodes/intern/node_socket.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/node_socket.c#newcode68
source/blender/nodes/intern/node_socket.c:68: static void
foreach_converter_vector(void *userdata, int type,
NodeSocketConverterWalkFunction cb)
I don't understand the purpose of these foreach_converter callbacks,
guess this will become clear when it's actually used.

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/node_socket.h
File source/blender/nodes/intern/node_socket.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/node_socket.h#newcode54
source/blender/nodes/intern/node_socket.h:54: SOCK_FLOAT,
What is the difference between SOCK_VALUE and SOCK_FLOAT?

http://codereview.appspot.com/4476050/diff/3001/source/blender/windowmanager/intern/wm_init_exit.c
File source/blender/windowmanager/intern/wm_init_exit.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/windowmanager/intern/wm_init_exit.c#newcode138
source/blender/windowmanager/intern/wm_init_exit.c:138:
ED_init_node_socket_buttonfuncs();
Could we just have a single ED_nodes_init() function?

http://codereview.appspot.com/4476050/diff/3001/source/creator/CMakeLists.txt
File source/creator/CMakeLists.txt (right):

http://codereview.appspot.com/4476050/diff/3001/source/creator/CMakeLists.txt#newcode45
source/creator/CMakeLists.txt:45: /usr/local/cuda/include
Not part of node stuff.

http://codereview.appspot.com/4476050/diff/3001/source/creator/CMakeLists.txt#newcode751
source/creator/CMakeLists.txt:751: bf_editor_particleset
Not part of node stuff.

http://codereview.appspot.com/4476050/