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

[brechtvanlommel at gmail.com]

First batch of code comments and questions.


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

http://codereview.appspot.com/4476050/diff/3001/source/blender/editors/space_node/node_edit.c#newcode2105
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_node/node_edit.c#newcode3115
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_node/node_edit.c#newcode3178
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_node/node_edit.c#newcode3224
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_node_types.h
File source/blender/makesdna/DNA_node_types.h (right):

http://codereview.appspot.com/4476050/diff/3001/source/blender/makesdna/DNA_node_types.h#newcode47
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_node_types.h#newcode56
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_node_types.h#newcode84
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_node_types.h#newcode91
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_node_types.h#newcode105
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_node_types.h#newcode107
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_node_types.h#newcode134
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_node_types.h#newcode136
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_node_types.h#newcode198
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_node_types.h#newcode200
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_node_types.h#newcode212
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_node_types.h#newcode234
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_node_types.h#newcode283
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.

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