Tiles Compositing Branch

Tiles Compositing Branch

[brechtvl, 2012-05-11 13:04:48]

Hi Jeroen & Monique, I added a code review because it's easier for me to comment
on specific lines then.

Some issues found in testing:
* Preview images in node (e.g. in the Bokeh Image node) are flickering when
tweaking parameters. Other preview renders in Blender don't clear the old image
each but overwrite it tile by tile, which is much nicer.

* Button labels are invisible in many nodes, due to nodes not being wide enough
or too many buttons being packed in one row. For example in the Box/Ellipse mask
nodes, the labels are invisible even at the maximum node width.

* The default node with of some nodes should be increased (e.g. Set Sampler),
they're often too narrow to read the labels.

* The color correction node in particular is quite problematic in button layout.
Maybe there should be a column of row labels on the left side?

* For the switch node it's not immediately clear which input socket is used
when. Maybe name the input sockets Off and On? Makes it immediately clear.

* Bokeh Image angle should allow negative angles, so you don't have to go all
the way around to rotate it a bit in the other direction.

* Regarding chunksize, could the tooltip be made more clear about what kind of
tradeoff it gives (I'm guessing smaller chunk size gives better distribution of
multiple threads, but more overhead).

* Dilate/Erode (2) is not a good name for a node :) Is there any particular
reason this functionality couldn't be merged in the original node?

* How does this new NodeRenderlayerData work, where is it configured? And why
even have it at all, instead of keeping such transformation in a separate node?
It seems strange that this particular node would have it while an
Image/MovieClip/Texture node does not?

* Adding a Bokeh Image node gives me OpenCL errors in the console. This is with
Use OpenCL disabled.

OPENCL error: OpenCL Warning : clBuildProgram failed: could not build program
for device[0] 0x10301e398 (GeForce 9600M GT) (err:-44)
OPENCL error: [CL_BUILD_ERROR] : OpenCL Build Error : Compiler build log:
<program source>:3:58: error: parameter may not be qualified with an address
space
__kernel void testKernel(__global __write_only image2d_t output){

[brechtvl, 2012-05-11 13:12:58]

I'm also unsure about the Set Sampler node. It's unlike any other node, and
seems like one of those things that would not get used. 99% of the time you want
to have Bicubic filtering anyway. It feels like there should be another way to
configure this instead of using a node, but I'm not sure what to suggest.

Also the name is not really clear, maybe something like "Set Resize Filter"?

[brechtvl, 2012-05-11 13:36:47]

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

http://codereview.appspot.com/6199066/diff/6071/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:13064: if
(node->type==CMP_NODE_R_LAYERS) {
For a moment here I thought this code was wrong, because it didn't go into node
groups, but it seems render layer nodes are not allowed in node groups. Still
might be good to add a comment about that exception here.

http://codereview.appspot.com/6199066/diff/6071/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_nodetree.c (right):

http://codereview.appspot.com/6199066/diff/6071/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_nodetree.c:1447: {3, "NOT",           0,
"Not",           ""},
Can we find a better name than "Not"? It's not clear to what such an operation
would do. Or at least fill in the tooltip then.

Also in this enum and a few others in this file, hardcoded number values are
used, while e.g. CMP_NODE_MASKTYPE_ADD is defined.

http://codereview.appspot.com/6199066/diff/6071/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_nodetree.c:1832: {0, NULL, 0, NULL, NULL}};
More hardcoded number values.

http://codereview.appspot.com/6199066/diff/6071/source/blender/nodes/composit...
File source/blender/nodes/composite/node_composite_tree.c (right):

http://codereview.appspot.com/6199066/diff/6071/source/blender/nodes/composit...
source/blender/nodes/composite/node_composite_tree.c:140: node->new_node->id=
(ID *)node->id;
What is the reason for this change?

http://codereview.appspot.com/6199066/diff/6071/source/creator/creator.c
File source/creator/creator.c (right):

http://codereview.appspot.com/6199066/diff/6071/source/creator/creator.c#newc...
source/creator/creator.c:1186: OCL_init();
Could we switch this to lazy initialization when it is used for the first time?
With Cycles we've found some corrupt ATI OpenCL installations which crashed on
init, it would be good if in such cases at least Blender itself would not
immediately crash.

[j.bakker, 2012-05-11 13:58:47]

Also added your remarks as issues in the issue tracker.

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

http://codereview.appspot.com/6199066/diff/6071/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:13064: if
(node->type==CMP_NODE_R_LAYERS) {
On 2012/05/11 13:36:47, brechtvl wrote:
> For a moment here I thought this code was wrong, because it didn't go into
node
> groups, but it seems render layer nodes are not allowed in node groups. Still
> might be good to add a comment about that exception here.

See your comment about the NodeRenderLayerData. this part will be removed due to
no usage atm. (is part of canvas compositing)

http://codereview.appspot.com/6199066/diff/6071/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_nodetree.c (right):

http://codereview.appspot.com/6199066/diff/6071/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_nodetree.c:1447: {3, "NOT",           0,
"Not",           ""},
On 2012/05/11 13:36:47, brechtvl wrote:
> Can we find a better name than "Not"? It's not clear to what such an operation
> would do. Or at least fill in the tooltip then.
> 
> Also in this enum and a few others in this file, hardcoded number values are
> used, while e.g. CMP_NODE_MASKTYPE_ADD is defined.
Oke, will add defines to dna and perhaps difference/union/exclusion is a better
name for masking.

http://codereview.appspot.com/6199066/diff/6071/source/blender/nodes/composit...
File source/blender/nodes/composite/node_composite_tree.c (right):

http://codereview.appspot.com/6199066/diff/6071/source/blender/nodes/composit...
source/blender/nodes/composite/node_composite_tree.c:140: node->new_node->id=
(ID *)node->id;
On 2012/05/11 13:36:47, brechtvl wrote:
> What is the reason for this change?
To have direct feedback to the viewer/backdrop the compositor writes directly to
the imagebuffer of the viewernode. This cannot be the copy of that image buffer
as the UI is not looking to that instance.

[brechtvl, 2012-05-11 15:46:51]

http://codereview.appspot.com/6199066/diff/6071/source/blender/compositor/int...
File source/blender/compositor/intern/COM_WorkScheduler.cpp (right):

http://codereview.appspot.com/6199066/diff/6071/source/blender/compositor/int...
source/blender/compositor/intern/COM_WorkScheduler.cpp:79: if
(WorkScheduler::isStopping()) {
I've made a patch to avoid the sleep calls, it could be a performance issue with
small chunk sizes, and the main thread is constantly waking up for no good
reason. Will leave to you to commit if you think it's a good change.
http://www.pasteall.org/31729/diff

[brechtvl, 2012-05-11 17:33:40]

On the Blender integration/UI level, these are all the comments I have and bugs
I found. I didn't look into the code for all nodes and operations and test them,
don't really have time to verify them all, from what I gather users have been
testing this quite well with many reports in the tile tracker fixed.

Further, I added a hidden switch to use the old compositor in the branch now.
Eventually that should of course be removed, along with most of the old node
execution code, but for now it's nice to test.