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

[lukas.toenne at googlemail.com]

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)) {
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_node/node_edit.c#newcode3115
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_node/node_edit.c#newcode3178
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_node/node_edit.c#newcode3224
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_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;
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_node_types.h#newcode56
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_node_types.h#newcode84
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_node_types.h#newcode91
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_node_types.h#newcode105
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_node_types.h#newcode107
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_node_types.h#newcode134
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_node_types.h#newcode136
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.

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