Page MenuHome

Fix 50212: Box/Ellipse Mask compositor nodes are affected by render percentage changes
Needs ReviewPublic

Authored by Manuel Castilla (manzanilla) on Jan 16 2021, 5:31 PM.

Details

Summary

Box/Ellipse Mask nodes are not applied correctly on output operations when they have no inputs. Specially when you change render percentage as it's affecting the result when it shouldn't. If using images with different resolution than scene output resolution is not applied correctly either.

The reason is this old patch -> https://developer.blender.org/rBed792b5de08258f3c906d36381d67da55456ec34
ScaleFixedOperation is being used to set an input fixed resolution based on current scene resolution with its percentage applied. I see the reason why this was done but by forcing it as an input resolution breaks the rest of cases. Users may want the mask to be applied to another input of any resolution that is connected in an outer node.
This patch takes into account the case described in the old patch and the rest of cases to make it work as expected in all cases by using determineResolution to set a default preferred resolution that will be applied when these 2 points are not met:

  1. The node/operation doesn't have an input from which it can take a resolution or all of them has no resolution.
  2. The operation receives in determineResolution an output "preferredResolution" of 0 and cannot set any resolution if input resolution is 0 too.

With this patch we can set a third condition for determineResolution method so that it achieves what the old patch wants and keeps the right behaviour for the rest of cases:

  1. We optionally can set a preferred resolution for the cases where there is no output preferred resolution, so that generated inputs which have no resolution by themselves, may be rendered with that resolution.

This is the case of these mask nodes when they don't have inputs connected, they become generated inputs.

Diff Detail

Repository
rB Blender
Branch
T50212 (branched from master)
Build Status
Buildable 12221
Build 12221: arc lint + arc unit

Event Timeline

Manuel Castilla (manzanilla) requested review of this revision.Jan 16 2021, 5:31 PM
Manuel Castilla (manzanilla) created this revision.
Manuel Castilla (manzanilla) retitled this revision from Fix 50212 to Fix 50212: Box/Ellipse Mask compositor nodes are affected by render percentage changes.Jan 16 2021, 5:34 PM
Manuel Castilla (manzanilla) edited the summary of this revision. (Show Details)
Manuel Castilla (manzanilla) edited the summary of this revision. (Show Details)
  • fix wrong clang format
  • fix array index typo

Nice to see this issue being addressed!

Sounds very similar to D8952. Would be nice to have single mechanism of preserving compositor look when changing resolution percentage. Did you check whether same idea from D8952 can be applied here and reduce boiler plate code here?

Ah I see now. So the render resolution seems to behave OK when doing F12, but not when doing adjustments in the editor.

The resolution detection is somehow tricky part of compositor. @Monique Dewanchand (mdewanchand) Do you have time to help with test and review?

From quick testing the patch seems to behave correct, but wouldn't mind some extra pair of eyes here.

source/blender/compositor/intern/COM_NodeOperation.h
269

What do you mean by "current and inputs" ?

I made a patch D10611 as an alternative for the use of setDefaultPreferredResolution but might imply a bit of design change and should be decided by a member of the module. I rather keep this patch as it is for now, it seems to work fine.

source/blender/compositor/intern/COM_NodeOperation.h
269

I mean it will propagate throughout the tree from current node to input nodes when input nodes don't have resolutions on their own and have to use the preferred resolution. I thought about using this method in Viewers and Previews, but D10611 patch is a better solution.

Thanks for an update Manuel!

@Jeroen Bakker (jbakker) This is a bit tricky area of code. Do you want to have a review here? Otherwise would say lets go with what seems to work for artists :)

We should clean up the rd->ysch * render_size_factor and move it to the render context... but that is a separate patch and can be done afterwards.
Something like

std::array<unsigned int, 2> pref_resolution = context.get_scaled_render_resolution()

or add a Resolution struct.

Next to the mask nodes there might be other nodes that benefit from this as well.
I don't see an issue with this patch code-wise and am fine to let artists test drive it in master.