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

[lukas.toenne at googlemail.com]

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;
On 2012/02/21 12:27:53, brechtvl wrote:
> 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.

Done.

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;
On 2012/02/21 12:27:53, brechtvl wrote:
> scene.r => scene->r

Done.

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 */
On 2012/02/21 12:27:53, brechtvl wrote:
> 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.

These sockets really are color sockets. They just use some additional
information (image format, paths in name), which requires customized
draw functions and RNA refine. Socket types OTOH are currently based
only on the data type of the socket. If we add a new data type for each
socket with slightly different behavior it means we have to update data
handling and conversions for all existing node systems as well, which is
really not necessary. It would be nicer to actually distinguish general
socket type and data type better, but for now this kind of "subtype"
seems an acceptable solution.

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;
On 2012/02/21 12:27:53, brechtvl wrote:
> 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.

Yes, this was an explicit request. Note that by default all sockets use
the render settings (use_render_format flag), setting custom socket
format is optional. We also discussed using a file format per node
instead of "global" render settings, but this seems to work well for
now.

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? */
On 2012/02/21 12:27:53, brechtvl wrote:
> 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.

Agreed, i would rather add another new node type specifically for
writing multi-layer exr. Can work the same way as this node, just
writing to layers instead individual files and folders.

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