[Bf-codereview] Multi File Output Node (issue 5666047)

[brechtvanlommel at gmail.com]

Is there any particular reason to create a new node rather than
extending the File Output node? It seems like a natural extension of
that, only issue I can see is that it requires some version patching.


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

http://codereview.appspot.com/5666047/diff/1/source/blender/editors/space_node/node_header.c#newcode117
source/blender/editors/space_node/node_header.c:117: ntemp.r =
&scene->r;
Instead of passing RenderData, I'd just pass in Main + Scene, that's
sort of the typical context you get in RNA too, which is less specific.

http://codereview.appspot.com/5666047/diff/1/source/blender/editors/space_node/node_header.c#newcode162
source/blender/editors/space_node/node_header.c:162: ntemp.r = &scene.r;
scene.r => scene->r

http://codereview.appspot.com/5666047/diff/1/source/blender/makesdna/DNA_node_types.h
File source/blender/makesdna/DNA_node_types.h (right):

http://codereview.appspot.com/5666047/diff/1/source/blender/makesdna/DNA_node_types.h#newcode80
source/blender/makesdna/DNA_node_types.h:80: short struct_type;			/*
optional identifier for RNA struct subtype */
Why add this struct_type mechanism instead of adding a new socket type?
Maybe it is the right thing to do but I'm not sure, what are the other
possible use cases here?

If it was just a socket type, at least it would fit more cleanly, and it
also makes some sense conceptually, as you can output files with either
color/vector/float values.

http://codereview.appspot.com/5666047/diff/1/source/blender/makesdna/DNA_node_types.h#newcode370
source/blender/makesdna/DNA_node_types.h:370: ImageFormatData format;
Does this need to be a per socket setting? The use case is not entirely
clear to me, maybe this was requested, but it seems inconvenient having
to change the image settings on all sockets rather than once for a node.

The other reason is also, it might make sense to add EXR multi layer
output eventually, and than it makes even more sense to have one per
node.

http://codereview.appspot.com/5666047/diff/1/source/blender/nodes/composite/nodes/node_composite_outputFile.c
File source/blender/nodes/composite/nodes/node_composite_outputFile.c
(right):

http://codereview.appspot.com/5666047/diff/1/source/blender/nodes/composite/nodes/node_composite_outputFile.c#newcode236
source/blender/nodes/composite/nodes/node_composite_outputFile.c:236:
#if 0	/* XXX is this needed? if so, how to integrate z buffer output in
the UI? */
I don't think this is needed, some old file formats can save a Z buffer
in the same file, but that isn't really in use anymore as far as I know,
multilayer EXR would be used instead.

http://codereview.appspot.com/5666047/