Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5666)

Issue 5602051: Channel Matte Node (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 3 months ago by dfelinto
Modified:
12 years, 3 months ago
Reviewers:
jbkkavt, bf-codereview
Base URL:
https://svn.blender.org/svnroot/bf-blender/branches/tile/blender
Visibility:
Public.

Description

This is a huge node (too many sub-options) so this is my first take. I didn't review it, but in my quick test (with rgb) it seems to match trunk. My questions: 1) is better to store settings and custom1 and custom2 OR the individual values (as implemented in the patch). 2) it's better to access the class once (const int limit_min = this->limit_min) or it's fine to do it every time it's needed (as it's now) 3) I unified both methods (max and simple) by forcing the simple to use the max algorithm. I think it's acceptable. The alternative would be to have a new operation for each algorithm or have an IF statement in the execute code. Note: I didn't review the colorspace code. I may need to normalize the input from the convert operation. I may not, too late to look at that. I think there is enough on this patch to keep Jeroen busy while I take my turn into sleep ;) -- edit: committed at #43973

Patch Set 1 #

Patch Set 2 : patch updated (morning review) - removed custom1 from settings, rest looks good #

Unified diffs Side-by-side diffs Delta from patch set Stats (+328 lines, -1 line) Patch
source/blender/compositor/CMakeLists.txt View 2 chunks +4 lines, -0 lines 0 comments Download
source/blender/compositor/intern/COM_Converter.cpp View 3 chunks +4 lines, -1 line 0 comments Download
source/blender/compositor/nodes/COM_ChannelMatteNode.h View 1 chunk +38 lines, -0 lines 0 comments Download
source/blender/compositor/nodes/COM_ChannelMatteNode.cpp View 1 1 chunk +88 lines, -0 lines 0 comments Download
source/blender/compositor/operations/COM_ChannelMatteOperation.h View 1 1 chunk +76 lines, -0 lines 0 comments Download
source/blender/compositor/operations/COM_ChannelMatteOperation.cpp View 1 chunk +118 lines, -0 lines 0 comments Download

Messages

Total messages: 5
dfelinto
12 years, 3 months ago (2012-02-01 08:19:10 UTC) #1
dfelinto
patch updated (morning review) - removed custom1 from settings, rest looks good
12 years, 3 months ago (2012-02-01 17:32:08 UTC) #2
jbkkavt
seems fine!
12 years, 3 months ago (2012-02-03 11:36:47 UTC) #3
jbkkavt
On 2012/02/03 11:36:47, jbkkavt wrote: > seems fine! 1. custom1 and custom2 are legacy, I ...
12 years, 3 months ago (2012-02-03 11:39:46 UTC) #4
dfelinto
12 years, 3 months ago (2012-02-03 12:26:22 UTC) #5
> 1. custom1 and custom2 are legacy, I would not like to have them in tiles
That means I should leave as it's in the patch? or you have something else in
mind?

> 2. I prefer to use const int. but there are cases where it doesn't work (M$
bug,
> just let DingTo help out there.)
Strange. I'll investigate.

> 3. it is ok how you implemented it. having two operations is the second best
> option, third if the if-statement
I'm glad I did get the priority order right ;)


I will change the const to this-> and commit (after I nap and wake up)
Thanks Jeroen
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b