Page MenuHome
Differential D11881

Convert ID Mask node AntiAliasing to SMAA
ClosedPublic
Actions

Authored by Aidan Haile (tactical_fluke) on Jul 11 2021, 1:22 PM.

Details

Reviewers
Manuel Castilla (manzanilla)
Commits
rB98c4224ff032: Fix T49944: Compositor ID Mask Anti-Aliasing not working
Summary

Intended as a fix for https://developer.blender.org/T49944

This patch takes @Jeroen Bakker (jbakker) 's suggestion to switch the anti-aliasing on an ID mask node to use the pre-existing SMAA operations.

using the blend file attached to the issue, we get the following:

Before:

After:

I have run and confirmed the automated tests still pass.

Diff Detail

Repository
rB Blender

Event Timeline

Aidan Haile (tactical_fluke) requested review of this revision.Jul 11 2021, 1:22 PM
Aidan Haile (tactical_fluke) created this revision.
Aidan Haile (tactical_fluke) mentioned this in T49944: Compositor - ID Mask Anti Aliasing not working.Jul 11 2021, 1:25 PM
Aidan Haile (tactical_fluke) updated this revision to Diff 39405.Jul 11 2021, 2:43 PM

Format the comment

Aidan Haile (tactical_fluke) updated this revision to Diff 39407.Jul 11 2021, 4:55 PM
Aidan Haile (tactical_fluke) added a comment.Jul 12 2021, 2:23 PM

hey @Jeroen Bakker (jbakker) would you okay to or have the time to review this?

Jeroen Bakker (jbakker) added a reviewer: Manuel Castilla (manzanilla).Jul 12 2021, 4:34 PM
Jeroen Bakker (jbakker) added a subscriber: Manuel Castilla (manzanilla).

@Manuel Castilla (manzanilla) can you review this patch? Also check if the aa operation can be removed

Manuel Castilla (manzanilla) added a comment.Jul 12 2021, 8:36 PM

Look good to me. It does fix issue. Only a few changes to follow our code style guidelines:

  • We prefer to always use C-style comments, starting with a capital letter and ending with a full stop. (/* Comment. */)
source/blender/compositor/nodes/COM_IDMaskNode.cc
48

As these values are used more than once, please define them in "node_composite_util.h" with names CMP_* and replace them in "node_composite_antialiasing.c" too.

Manuel Castilla (manzanilla) added a comment.Jul 12 2021, 8:42 PM

@Jeroen Bakker (jbakker) The AA operation can be removed if we do the same on ZCombineNode and DilateErodeNode. It won't affect tests either because they don't reach the code path where it's used.
Don't see a reason why not, only downside is performance, but result is far better. I can create a "Good First Issue" task for doing it. Just need your ok.

Manuel Castilla (manzanilla) requested changes to this revision.Jul 12 2021, 8:45 PM
This revision now requires changes to proceed.Jul 12 2021, 8:45 PM
Manuel Castilla (manzanilla) added inline comments.Jul 12 2021, 9:00 PM
source/blender/compositor/nodes/COM_IDMaskNode.cc
48

Call the setters on SMAA operations constructors with these default values, so we don't need to set them on all future nodes that may use SMAA.

Manuel Castilla (manzanilla) added inline comments.Jul 12 2021, 11:56 PM
source/blender/compositor/nodes/COM_IDMaskNode.cc
48

As these values are used more than once, please define them in "node_composite_util.h" ...

Sorry, not in "node_composite_util.h" but in "BKE_node.h".

Aidan Haile (tactical_fluke) updated this revision to Diff 39454.Jul 13 2021, 11:26 AM

I believe I have addressed your review actions @Manuel Castilla (manzanilla) , could I get a re-review when you've got the time?

Manuel Castilla (manzanilla) accepted this revision.Jul 13 2021, 1:17 PM

All good on my part :). I'll commit it tomorrow, if there are no objections.

This revision is now accepted and ready to land.Jul 13 2021, 1:17 PM
Aidan Haile (tactical_fluke) added a comment.Jul 15 2021, 11:54 AM

Sounds good to me :)

Closed by commit rB98c4224ff032: Fix T49944: Compositor ID Mask Anti-Aliasing not working (authored by Aidan Haile (tactical_fluke), committed by Manuel Castilla (manzanilla)). · Explain WhyJul 15 2021, 10:26 PM
This revision was automatically updated to reflect the committed changes.
Manuel Castilla (manzanilla) added a commit: rB98c4224ff032: Fix T49944: Compositor ID Mask Anti-Aliasing not working.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
5 lines
compositor/
nodes/
26 lines
operations/
7 lines
nodes/
composite/
nodes/
6 lines

Diff 39621

View Options

source/blender/blenkernel/BKE_node.h

Loading...
View Options

source/blender/compositor/nodes/COM_IDMaskNode.cc

Loading...
View Options

source/blender/compositor/operations/COM_SMAAOperation.cc

Loading...
View Options

source/blender/nodes/composite/nodes/node_composite_antialiasing.c

Loading...