Double Edge Matte

Double Edge Matte composite node.

[lukas.toenne, 2012-01-06 16:43:50]

Congrats, couldn't find any issues :)

This is superbly documented code for once, so i have no worries it will be
maintainable in future too.

LGTM!

[zanqdo_gmail.com, 2012-01-06 16:48:31]

Hurray! Congrats Pete on the flattering review :)

Daniel Salazar
3Developer.com


On Fri, Jan 6, 2012 at 10:43 AM, <lukas.toenne@googlemail.com> wrote:

> Congrats, couldn't find any issues :)
>
> This is superbly documented code for once, so i have no worries it will
> be maintainable in future too.
>
> LGTM!
>
> http://codereview.appspot.com/5523046/
> _______________________________________________
> Bf-codereview mailing list
> Bf-codereview@blender.org
> http://lists.blender.org/mailman/listinfo/bf-codereview
>

[brechtvl, 2012-01-06 17:15:09]

Regarding the name of the node, it's a bit strange that this one is also called
Matte while it doesn't create a matte necessarily, it's sort of blending two
masks? Is there a standard name for this type of operation in other software?

http://codereview.appspot.com/5523046/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_nodetree.c (right):

http://codereview.appspot.com/5523046/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_nodetree.c:2134: {1, "ADJONL", 0, "Adjacent
Only", "Only inner mask pixels adjacent to outer mask pixels are considerer
during mask calculation"},
This name does not need to be abbreviated, use ADJACENTE_ONLY, maybe above
BLEED_OUT and KEEP_IN as well.

Typo: considerer => considered

http://codereview.appspot.com/5523046/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_nodetree_types.h (right):

http://codereview.appspot.com/5523046/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_nodetree_types.h:132: DefNode(
CompositorNode, CMP_NODE_DBLEDGMATTE,    def_cmp_dbl_edg_matte,  "DBLEDGMATTE", 
  DblEdgMatte,      "Double Edge Matte", ""              )
Same here, don't use DBLEDGMATTE abbreviation.

http://codereview.appspot.com/5523046/diff/1/source/blender/nodes/composite/n...
File source/blender/nodes/composite/nodes/node_composite_dblEdgMatte.c (right):

http://codereview.appspot.com/5523046/diff/1/source/blender/nodes/composite/n...
source/blender/nodes/composite/nodes/node_composite_dblEdgMatte.c:43: {
SOCK_FLOAT, 0, "D.E.M. Mask"},          // output socket definition
Just call output "Mask".

http://codereview.appspot.com/5523046/diff/1/source/blender/nodes/composite/n...
source/blender/nodes/composite/nodes/node_composite_dblEdgMatte.c:47: static
void node_composit_exec_dbledgmatte(void *UNUSED(data), bNode *node, bNodeStack
**in, bNodeStack **out) {
It might be nice to split this up into smaller pieces, 900 lines is a lot for
one function..

[zanqdo_gmail.com, 2012-01-06 17:16:37]

Would Double Edge Mask be better?

Daniel Salazar
3Developer.com


On Fri, Jan 6, 2012 at 11:15 AM, <brechtvanlommel@gmail.com> wrote:

> Regarding the name of the node, it's a bit strange that this one is also
> called Matte while it doesn't create a matte necessarily, it's sort of
> blending two masks? Is there a standard name for this type of operation
> in other software?
>
>
> http://codereview.appspot.com/**5523046/diff/1/source/blender/**
>
makesrna/intern/rna_nodetree.c<http://codereview.appspot.com/5523046/diff/1/source/blender/makesrna/intern/rna_nodetree.c>
> File source/blender/makesrna/**intern/rna_nodetree.c (right):
>
> http://codereview.appspot.com/**5523046/diff/1/source/blender/**
>
makesrna/intern/rna_nodetree.**c#newcode2134<http://codereview.appspot.com/5523046/diff/1/source/blender/makesrna/intern/rna_nodetree.c#newcode2134>
> source/blender/makesrna/**intern/rna_nodetree.c:2134: {1, "ADJONL", 0,
> "Adjacent Only", "Only inner mask pixels adjacent to outer mask pixels
> are considerer during mask calculation"},
> This name does not need to be abbreviated, use ADJACENTE_ONLY, maybe
> above BLEED_OUT and KEEP_IN as well.
>
> Typo: considerer => considered
>
> http://codereview.appspot.com/**5523046/diff/1/source/blender/**
>
makesrna/intern/rna_nodetree_**types.h<http://codereview.appspot.com/5523046/diff/1/source/blender/makesrna/intern/rna_nodetree_types.h>
> File source/blender/makesrna/**intern/rna_nodetree_types.h (right):
>
> http://codereview.appspot.com/**5523046/diff/1/source/blender/**
>
makesrna/intern/rna_nodetree_**types.h#newcode132<http://codereview.appspot.com/5523046/diff/1/source/blender/makesrna/intern/rna_nodetree_types.h#newcode132>
> source/blender/makesrna/**intern/rna_nodetree_types.h:**132: DefNode(
> CompositorNode, CMP_NODE_DBLEDGMATTE,    def_cmp_dbl_edg_matte,
> "DBLEDGMATTE",    DblEdgMatte,      "Double Edge Matte", ""
> )
> Same here, don't use DBLEDGMATTE abbreviation.
>
> http://codereview.appspot.com/**5523046/diff/1/source/blender/**
>
nodes/composite/nodes/node_**composite_dblEdgMatte.c<http://codereview.appspot.com/5523046/diff/1/source/blender/nodes/composite/nodes/node_composite_dblEdgMatte.c>
> File source/blender/nodes/**composite/nodes/node_**composite_dblEdgMatte.c
> (right):
>
> http://codereview.appspot.com/**5523046/diff/1/source/blender/**
>
nodes/composite/nodes/node_**composite_dblEdgMatte.c#**newcode43<http://codereview.appspot.com/5523046/diff/1/source/blender/nodes/composite/nodes/node_composite_dblEdgMatte.c#newcode43>
> source/blender/nodes/**composite/nodes/node_**composite_dblEdgMatte.c:43:
> {
> SOCK_FLOAT, 0, "D.E.M. Mask"},          // output socket definition
> Just call output "Mask".
>
> http://codereview.appspot.com/**5523046/diff/1/source/blender/**
>
nodes/composite/nodes/node_**composite_dblEdgMatte.c#**newcode47<http://codereview.appspot.com/5523046/diff/1/source/blender/nodes/composite/nodes/node_composite_dblEdgMatte.c#newcode47>
> source/blender/nodes/**composite/nodes/node_**composite_dblEdgMatte.c:47:
> static void node_composit_exec_**dbledgmatte(void *UNUSED(data), bNode
> *node, bNodeStack **in, bNodeStack **out) {
> It might be nice to split this up into smaller pieces, 900 lines is a
> lot for one function..
>
>
http://codereview.appspot.com/**5523046/<http://codereview.appspot.com/5523046/>
>

[xglasyliax, 2012-01-10 17:15:31]

Applied naming changes as per Brecht's suggestions, and split up large function
into 9 smaller ones.

[brechtvl, 2012-01-10 17:44:53]

LGTM