Node changes from Particles Branch

Node changes from Particles Branch

[brechtvl, 2011-05-06 11:26:46]

First batch of code comments and questions.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/node_edit.c:2105: if (!(link->flag &
NLINK_VALID)) {
It seems like NLINK_VALID will never be set at this point, only after
ntreeUpdateTree?

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/node_edit.c:3115: if (ntemp.type >= 0) {
Bit silly comment here, but I find it easier to read if the failure cases with
OPERATOR_CANCELLED are inside if() and the successful case with
OPERATOR_FINISHED does not get nested.

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/node_edit.c:3178: char treename[22] =
"NodeTree";
Better use MAX_ID_NAME-2 instead of 22, same in the property definition.

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/node_edit.c:3224: ot->exec=
new_node_tree_exec;
There is a poll function missing here, or at least exec should check if
SpaceNode is actually in the context.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
File source/blender/makesdna/DNA_node_types.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:47: struct bNodeSocketTemplate;
These 2 struct declarations seem unnecessary.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:56: struct SimDataContext;
These last 4 struct declarations seem unnecessary.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:84: } bNodeSocketPanel;
I'd prefer to leave out these socket panels, in my opinion nodes with that many
sockets should not be added.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:91: void *storage;				/* custom storage
*/
Is this really necessary, wouldn't any storage be in default_value?

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:105: short need_exec;
need_exec doesn't seem to be used anywhere?

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:107: void *cache;				/* cached data
from execution */
This cache pointer seems like it should be part of execution data and not in
bNodeSocket?

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:134: #define SOCK_INTERNAL			32
SOCK_STATIC and SOCK_INTERNAL don't seem to be set in do_versions, is this
unnecessary?

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:136: #define SOCK_VIEW_OUTPUT		64
This socket viewing system doesn't seem to be implemented, best to leave it out
until that is done?

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:198: #define NODE_BACKGROUND		(1<<12)
NODE_CONST_OUTPUT and NODE_BACKGROUND seem more like things that should be part
of the node type, not the node?

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:200: #define NODE_TRANSFORM		(1<<13)
It seems to me it would be easier to not tag nodes with NODE_TRANSFORM but just
check for selection in the transform code.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:212: #define NLINK_VALID			1
Some comment on this would be good. The purpose is a bit unclear to me at the
moment.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:234: int stacksize;
Is this stacksize still needed, seems to be in bNodeTreeExec now?

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:283: int subtype;				/* RNA subtype */
The RNA subtype wasn't really intended to be saved in files. Not sure about this
yet, but at the very least there should be a comment in RNA_types.h that you
must be careful changing the enum values for compatibility reasons.

[brechtvl, 2011-05-06 13:00:23]

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...
File source/blender/blenkernel/BKE_node.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE...
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...
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...
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...
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/int...
File source/blender/blenkernel/intern/texture.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/int...
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/int...
File source/blender/blenloader/intern/readfile.c (right):

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/int...
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/int...
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/int...
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/int...
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/int...
File source/blender/blenloader/intern/writefile.c (right):

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/int...
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/SConsc...
File source/blender/editors/SConscript (right):

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

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
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_...
File source/blender/editors/space_node/node_header.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
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_...
File source/blender/editors/space_node/space_node.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
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/transf...
File source/blender/editors/transform/transform_conversions.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/transf...
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/inter...
File source/blender/makesrna/intern/rna_internal.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesrna/inter...
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/SConscri...
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...
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/S...
File source/blender/nodes/intern/SHD_nodetree.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/S...
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/n...
File source/blender/nodes/intern/node_common.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/n...
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/n...
File source/blender/nodes/intern/node_socket.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/n...
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/n...
File source/blender/nodes/intern/node_socket.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/n...
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/...
File source/blender/windowmanager/intern/wm_init_exit.c (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/windowmanager/...
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...
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...
source/creator/CMakeLists.txt:751: bf_editor_particleset
Not part of node stuff.

[lukas.toenne, 2011-05-06 13:48:36]

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/node_edit.c:2105: if (!(link->flag &
NLINK_VALID)) {
On 2011/05/06 11:26:46, brechtvl wrote:
> It seems like NLINK_VALID will never be set at this point, only after
> ntreeUpdateTree?

Exactly, adding an ntreeUpdateTree call after nodeAddLink does the trick.

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/node_edit.c:3115: if (ntemp.type >= 0) {
On 2011/05/06 11:26:46, brechtvl wrote:
> Bit silly comment here, but I find it easier to read if the failure cases with
> OPERATOR_CANCELLED are inside if() and the successful case with
> OPERATOR_FINISHED does not get nested. 

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/node_edit.c:3178: char treename[22] =
"NodeTree";
On 2011/05/06 11:26:46, brechtvl wrote:
> Better use MAX_ID_NAME-2 instead of 22, same in the property definition.

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/node_edit.c:3224: ot->exec=
new_node_tree_exec;
On 2011/05/06 11:26:46, brechtvl wrote:
> There is a poll function missing here, or at least exec should check if
> SpaceNode is actually in the context.

Has the usual ED_operator_node_active now.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
File source/blender/makesdna/DNA_node_types.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:47: struct bNodeSocketTemplate;
On 2011/05/06 11:26:46, brechtvl wrote:
> These 2 struct declarations seem unnecessary.

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:56: struct SimDataContext;
On 2011/05/06 11:26:46, brechtvl wrote:
> These last 4 struct declarations seem unnecessary.

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:84: } bNodeSocketPanel;
On 2011/05/06 11:26:46, brechtvl wrote:
> I'd prefer to leave out these socket panels, in my opinion nodes with that
many
> sockets should not be added.

Yes, those "panels" are an old and unnecessary feature, will remove them.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:91: void *storage;				/* custom storage
*/
On 2011/05/06 11:26:46, brechtvl wrote:
> Is this really necessary, wouldn't any storage be in default_value?

No, the storage pointer in sockets is supposed to work like the storage in
nodes: as a generic pointer to custom data for that particular socket/node
instance. This is not used by the existing tree types, but i used it for new
tree types. E.g. some nodes can dynamically create new sockets for arbitrary
properties and then need to store some property identification for each socket.
This kind of information can then be stored in a custom data struct (which must
be freed by the node's freestorage function). The default_value on the other
hand only depends on the socket data type and stores the singleton input value.
I'd like to leave that storage pointer there for future nodes.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:105: short need_exec;
On 2011/05/06 11:26:46, brechtvl wrote:
> need_exec doesn't seem to be used anywhere?

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:107: void *cache;				/* cached data
from execution */
On 2011/05/06 11:26:46, brechtvl wrote:
> This cache pointer seems like it should be part of execution data and not in
> bNodeSocket?

The execution data is only temporary data used during the actual execution call.
The cache retains that data after execution (currently only used for compositor
output), so it can be reused if the input didn't change. Before this, the
ubiquitous bNodeStack struct member was used for this, but that is was a hack
imo. Using an explicit pointer instead is a cleaner solution.
However, this cache pointer only makes sense for top-level compositor nodes,
because there is only one instance of them. It would be better if this data was
stored outside of the actual node tree. That way node trees could be linked like
real ID data and reused in different places, but still have a cache for reusing
previous results. Can be done later.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:134: #define SOCK_INTERNAL			32
On 2011/05/06 11:26:46, brechtvl wrote:
> SOCK_STATIC and SOCK_INTERNAL don't seem to be set in do_versions, is this
> unnecessary?

SOCK_STATIC should be set in do_versions, yes. Most sockets are static, only
group sockets are dynamically added. So i'd better change this to SOCK_DYNAMIC
since that is the exception and not the default.
SOCK_INTERNAL is used to flag internal group tree sockets, so they are not
exposed on group instance nodes. However, this is only used for special loop
node sockets (the 'Condition' output in while loops), so no need to deal with
this in do_versions.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:136: #define SOCK_VIEW_OUTPUT		64
On 2011/05/06 11:26:46, brechtvl wrote:
> This socket viewing system doesn't seem to be implemented, best to leave it
out
> until that is done?

Yes, it's probably better to keep unused code out of here. It's a really nice
way of selecting viewer output, but can easily be added again later.

[lukas.toenne, 2011-05-06 19:46:20]

Fixed a few more issues.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
File source/blender/makesdna/DNA_node_types.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:198: #define NODE_BACKGROUND		(1<<12)
On 2011/05/06 11:26:46, brechtvl wrote:
> NODE_CONST_OUTPUT and NODE_BACKGROUND seem more like things that should be
part
> of the node type, not the node?

The node->flag and node->typeinfo->flag use the same flags. node->flag is
initialised from typeinfo->flag and some flags (OPTIONS and PREVIEW) are used as
general type flags as well as toggles for each node. The CONST_OUTPUT and
BACKGROUND flags should not be changed, but i will make sure they are only read
from the type.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:200: #define NODE_TRANSFORM		(1<<13)
On 2011/05/06 11:26:46, brechtvl wrote:
> It seems to me it would be easier to not tag nodes with NODE_TRANSFORM but
just
> check for selection in the transform code.

The reason i'm using a separate flag for this is that there is now an attachment
feature for nodes (used by frame nodes). All nodes attached to a selected parent
are included in it's transformation, regardless of selection state (no relative
coordinates are used). The transform flag is set before the actual
transformation for all nodes that are either selected or attached to a
transforming node.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:212: #define NLINK_VALID			1
On 2011/05/06 11:26:46, brechtvl wrote:
> Some comment on this would be good. The purpose is a bit unclear to me at the
> moment.

Done. Also renamed it to NODE_LINK_VALID for clarity.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:234: int stacksize;
On 2011/05/06 11:26:46, brechtvl wrote:
> Is this stacksize still needed, seems to be in bNodeTreeExec now?

Correct, removed it.

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_n...
source/blender/makesdna/DNA_node_types.h:283: int subtype;				/* RNA subtype */
On 2011/05/06 11:26:46, brechtvl wrote:
> The RNA subtype wasn't really intended to be saved in files. Not sure about
this
> yet, but at the very least there should be a comment in RNA_types.h that you
> must be careful changing the enum values for compatibility reasons.

Done. This now uses a separate subtype list in DNA, which is converted to RNA
property subtypes only in the RNA definition. Also moved the socket types back
to DNA from node_socket.h to avoid this problem.

[lukas.toenne, 2011-05-09 09:02:32]

Fixed most remaining issues.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE...
source/blender/blenkernel/BKE_node.h:78: struct bNodeDataSource;
On 2011/05/06 13:00:23, brechtvl wrote:
> Some unecessary declarations here: StructRNA, PropertyRNA, DerivedMesh,
> ParticleSystem, bNodeDataReader, bNodeDataSource.

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE...
source/blender/blenkernel/BKE_node.h:193: void (*edit_clear)(struct bNode
*node);
On 2011/05/06 13:00:23, brechtvl wrote:
> Would be good to have a group_ prefix here for clarity.

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE...
source/blender/blenkernel/BKE_node.h:255: char id_name[24];				/* id name for
RNA identification */
On 2011/05/06 13:00:23, brechtvl wrote:
> For consistency, this should be "char idname[64];"

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/BKE...
source/blender/blenkernel/BKE_node.h:262: void (*foreachNodeTree)(struct Main
*main, void *calldata, bNodeTreeCallback func);		/* iteration over all node
trees */
On 2011/05/06 13:00:23, brechtvl wrote:
> There some inconsistency here in callback names, some are camel case, some
not.

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:143: #include "RNA_types.h"
On 2011/05/06 13:00:23, brechtvl wrote:
> Unnecessary #includes.

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:145: #include "intern/node_socket.h"
On 2011/05/06 13:00:23, brechtvl wrote:
> 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?

Fixed, the socket type values are now defined in DNA again.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:2192: 
On 2011/05/06 13:00:23, brechtvl wrote:
> 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?

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:11727: /* Initialize texture point
density curve falloff */
On 2011/05/06 13:00:23, brechtvl wrote:
> This texture point density code is nested differently than in trunk, seems
like
> a merge error?

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:11747: /* Convert default socket
values from bNodeStack */
On 2011/05/06 13:00:23, brechtvl wrote:
> This conversion should be behind a subversion check, now
> do_versions_socket_default_value set default_value even if it's already there.

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/int...
source/blender/blenloader/intern/writefile.c:158: #include "RNA_access.h"
On 2011/05/06 13:00:23, brechtvl wrote:
> Unnecessary #include.

Done.

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenloader/int...
source/blender/blenloader/intern/writefile.c:666: writestruct(wd, DATA,
stype->value_structname, 1, sock->default_value);
On 2011/05/06 13:00:23, brechtvl wrote:
> Perhaps a write_node_socket utility to avoid duplicated code would be good.

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/SConsc...
source/blender/editors/SConscript:41: 'particleset/SConscript'])
On 2011/05/06 13:00:23, brechtvl wrote:
> Particle leftover.

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_api/spacetypes.c:66: #include "ED_particleset.h"
On 2011/05/06 13:00:23, brechtvl wrote:
> Particle leftover.

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/node_header.c:190: uiItemV(layout, "-- new
group --", 0, -NODE_GROUP);
On 2011/05/06 13:00:23, brechtvl wrote:
> Label here should be changed, -- and lowercase should not be used, just call
it
> "New Group", "New For Loop", "New While Loop"?

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_...
source/blender/editors/space_node/space_node.c:217: break;
On 2011/05/06 13:00:23, brechtvl wrote:
> Seems to be a modifiers nodes leftover.

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/transf...
source/blender/editors/transform/transform_conversions.c:5146: //		td->flag |=
TD_NO_LOC;
On 2011/05/06 13:00:23, brechtvl wrote:
> Could get rid of these two commented lines.

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_internal.h:161: void
RNA_def_particleset(struct BlenderRNA *brna);
On 2011/05/06 13:00:23, brechtvl wrote:
> Some more particle leftovers in this file.

Done.

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/SConscri...
source/blender/nodes/SConscript:9: #parsources =
env.Glob('intern/PAR_nodes/*.c')
On 2011/05/06 13:00:23, brechtvl wrote:
> Some leftover code from modifiers/particles.

Done.

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...
source/blender/nodes/SHD_node.h:48: void
register_node_type_group_shader(ListBase *lb);
On 2011/05/06 13:00:23, brechtvl wrote:
> Consistency nitpick, these functions could be named:
register_node_type_sh_group

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/S...
source/blender/nodes/intern/SHD_nodetree.c:139: /* XXX TODO */
On 2011/05/06 13:00:23, brechtvl wrote:
> Seems like this should be fixed before committing.

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/n...
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);
On 2011/05/06 13:00:23, brechtvl wrote:
> Perhaps the functions here could get a node_ prefix? Otherwise potentially
> confusing with object groups.

Done.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/n...
source/blender/nodes/intern/node_socket.c:68: static void
foreach_converter_vector(void *userdata, int type,
NodeSocketConverterWalkFunction cb)
On 2011/05/06 13:00:23, brechtvl wrote:
> I don't understand the purpose of these foreach_converter callbacks, guess
this
> will become clear when it's actually used.

Yes, another one of these unused features lingering around. I want to implement
explicit conversion methods on inputs in cases where more than one method is
possible. These functions would just allow the UI to iterate over all possible
conversion methods. Can remove this for now.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/nodes/intern/n...
source/blender/nodes/intern/node_socket.h:54: SOCK_FLOAT,
On 2011/05/06 13:00:23, brechtvl wrote:
> What is the difference between SOCK_VALUE and SOCK_FLOAT?

Short answer: there is none.
I only kept the SOCK_VALUE type for old nodes, but it is not really necessary.
So to simplify things i now just renamed it to SOCK_FLOAT.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/windowmanager/...
source/blender/windowmanager/intern/wm_init_exit.c:138:
ED_init_node_socket_buttonfuncs();
On 2011/05/06 13:00:23, brechtvl wrote:
> Could we just have a single ED_nodes_init() function?

Done.

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...
source/creator/CMakeLists.txt:45: /usr/local/cuda/include
On 2011/05/06 13:00:23, brechtvl wrote:
> Not part of node stuff.

Done.

http://codereview.appspot.com/4476050/diff/3001/source/creator/CMakeLists.txt...
source/creator/CMakeLists.txt:751: bf_editor_particleset
On 2011/05/06 13:00:23, brechtvl wrote:
> Not part of node stuff.

Done.

[lukas.toenne, 2011-05-09 09:16:23]

The storage of the execdata in node trees is an intermediate solution until a
better way of handling "compiled" node tree execution graphs in existing trees
is implemented.
Currently the execdata pointer in node trees is cleared in begin/end functions,
so compositor, material and texture nodes can work as before.

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

http://codereview.appspot.com/4476050/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/texture.c:772: tex->nodetree->execdata = NULL;
On 2011/05/06 13:00:23, brechtvl wrote:
> Setting execdata to NULL is not done in other places, but would prefer this
> instead of doing it using the backpointer everywhere.

Done.

[brechtvl, 2011-05-09 11:15:24]

Latest changes look fine to me, except for this one thing.

http://codereview.appspot.com/4476050/diff/16001/source/blender/makesdna/DNA_...
File source/blender/makesdna/DNA_node_types.h (right):

http://codereview.appspot.com/4476050/diff/16001/source/blender/makesdna/DNA_...
source/blender/makesdna/DNA_node_types.h:314: #define SOCK_SUBTYPE_XYZ			12
I don't think there is much point in duplicating these from RNA, it's just more
work to keep them in sync this way. A comment in RNA_types.h about these values
being written to file should be ok (I should have been more clear in my earlier
comment).

[lukas.toenne, 2011-05-10 11:21:50]

http://codereview.appspot.com/4476050/diff/16001/source/blender/makesdna/DNA_...
File source/blender/makesdna/DNA_node_types.h (right):

http://codereview.appspot.com/4476050/diff/16001/source/blender/makesdna/DNA_...
source/blender/makesdna/DNA_node_types.h:314: #define SOCK_SUBTYPE_XYZ			12
On 2011/05/09 11:15:24, brechtvl wrote:
> I don't think there is much point in duplicating these from RNA, it's just
more
> work to keep them in sync this way. A comment in RNA_types.h about these
values
> being written to file should be ok (I should have been more clear in my
earlier
> comment).

Ok, replaced this with PropertySubType values again and added a warning in
RNA_types.h.

[brechtvl, 2011-05-10 16:06:25]

Also reviewed node.c now, not much to comment.

http://codereview.appspot.com/4476050/diff/16001/source/blender/blenkernel/in...
File source/blender/blenkernel/intern/node.c (right):

http://codereview.appspot.com/4476050/diff/16001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/node.c:74: #include "intern/node_socket.h"
The functions used here should be in a public header for the nodes module, e.g.
NOD_nodes.h, going into intern like that is not a good precedent.

http://codereview.appspot.com/4476050/diff/16001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/node.c:1546: #if 0
Seems this code can be removed.

[brechtvl, 2011-05-10 16:08:42]

I've reviewed all the code now, looks good to go in if remaining issues are
fixed. I did a bunch of testing today, and found some bugs.

Texture nodes memory leak
* open node editor, change to texture nodes, click use nodes
* go to material tab in properties
* quit -> memory leak prints in console

Texture nodes preview
* open node editor, change to texture nodes, click use nodes
* go to material tab in properties -> preview ok
* switch to texture tab -> preview ok
* switch back to material tab -> preview missing texture

Curve mapping memory leak
* add vector curves node, duplicate node
* quit -> memory link prints in console

RGB/value node initialization
* add RGB node in material nodes
* color or value is 0 instead of proper default

Compo test file crash
* Open file from 2.49 test files:
http://users.telenet.be/blendix/compo_idmask_cubes.blend
* Press F12 -> crash
* File works fine in trunk, and 2.57 version of test file also works

GLSL
* connect geometry node global to output color
* GLSL compile error
* it seems node stack sockettype is wrong for the first socket

Crash when node type no longer exists
* found this in more complex case, but here is simpler way
* in node.c, comment out //register_node_type_sh_mix_rgb(ntypelist)
* open file http://users.telenet.be/blendix/removed_mix_node.blend -> crash
* due to NodeTagChanged being called on removed node, with node->typeinfo
invalid

[lukas.toenne, 2011-05-11 17:28:44]

http://codereview.appspot.com/4476050/diff/16001/source/blender/blenkernel/in...
File source/blender/blenkernel/intern/node.c (right):

http://codereview.appspot.com/4476050/diff/16001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/node.c:74: #include "intern/node_socket.h"
On 2011/05/10 16:06:25, brechtvl wrote:
> The functions used here should be in a public header for the nodes module,
e.g.
> NOD_nodes.h, going into intern like that is not a good precedent.

Done. The node tree types now have their own directory each. NOD is the primary
prefix for all public node headers in the nodes directory.

http://codereview.appspot.com/4476050/diff/16001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/node.c:1546: #if 0
On 2011/05/10 16:06:25, brechtvl wrote:
> Seems this code can be removed.

Done.

[brechtvl, 2011-05-13 11:46:36]

As mentioned in IRC, the doxygen comments & #ifdefs for headers were not updated
for the renames.

But besides that, LGTM!