Compositor: relative space blurnode
Needs ReviewPublic
Actions

Authored by Monique Dewanchand (mdewanchand) on Sep 18 2020, 9:38 PM.

Details

Reviewers

None

Group Reviewers

Compositing

Maniphest Tasks

T80562: Relative space

Summary

With this patch the BlurNode uses the render resolution percentage in the case that the relative option is not selected. The results are similar for different render resolution percentages compared to 100%.

As the render resolution percentage was already available in the blurnode (compositorcontext), only a factor is calculated and passed on to the BaseBlurOperation. The passing on happens as part of the setData methode, when data is "copied" to the operation.

During the initExecution the factor is applied to the sizes. Why here? In the InitExecution the relative option is checked and evaluated (can only be checked here as socketdata, size, becomes available). It seems like a logical place to add the factor here, so that the logic for relative calculations is at one place.

However this patch should only have been for the BlurNode, the dilate/erode and keying nodes are also part of this patch but the functionality is not affected. These nodes are part of the patch because they inherit from the BlurBaseOperation. But the InitExecution method is not called in the related operations of these nodes. I guess that the relative space for these nodes should be implemented along with the remarks in the code that suggests adding a "size" to the nodes or perhaps some other solution. Either way I guess that the InitExecution was commented out for a reason.

Diff Detail

Repository: rB Blender

Event Timeline

Monique Dewanchand (mdewanchand) requested review of this revision.Sep 18 2020, 9:38 PM

Monique Dewanchand (mdewanchand) created this revision.

Monique Dewanchand (mdewanchand) added a task: T80562: Relative space.

Jeroen Bakker (jbakker) awarded a token.Sep 19 2020, 11:10 AM

Gilberto Rodrigues (gilberto_rodrigues) awarded a token.Sep 20 2020, 5:59 AM

Evan Wilson (EAW) added a subscriber: Evan Wilson (EAW).Oct 6 2020, 4:51 PM

This comment was removed by Evan Wilson (EAW).

The idea seems to be what we had in mind.

From the implementation notes which do not touch existing aspects of the design:

Please use more descriptive comments, focusing on why-s not what-s.
There is noticeable amount of duplication of const float render_resolution_factor = context.getRenderData()->size * 0.01f;. This can easily become an utility function in either converter or the node base class.

Making access to the resolution factor as a function is that you can put all the descriptive comment in there about why is it needed, when to use.

Going a bit deeper into class organization, the question is: why is it up to a user of blur node to calculate resolution factor? Why can't this be left out to the blur node itself to calculate all the things it needs from the context?

source/blender/compositor/operations/COM_BlurBaseOperation.h
68	Strictly speaking, the function does not copy render resolution. The resolution is not even passed to the function. The comment should also give more clues of how exactly the data is used. Why is this resolution factor is needed, how is it used, when is it used.

Monique Dewanchand (mdewanchand) mentioned this in D9566: Cleanup hardcoded render percentage to factor conversion.Nov 14 2020, 4:34 PM

Jeroen Bakker (jbakker) mentioned this in rBc751d40e070c: Cleanup hardcoded render percentage to factor conversion.Nov 30 2020, 8:02 AM

Updated the patch for latest master and changed code comments.

The patch is much cleaner now.

Few things to improve still. Mainly about the setData() method. The naming is quite bad (is too generic, forcing you to check on either API comment or even look into implementation). You can also see this from the updated comment: the method does foo and bar. One method -- one function.

There are two ways to make things more clear here I see.

As simple as rename setData() to setBlurSettings(). Then the behavior can be commented as:

/**
 * Set operation settings controlling amount of blur.
 *
 * These settings are coming from two sources: user settings in the interface (denoted by
 * `NodeBlurData`) and from run-time compositor scaling (denoted by the `factor`).
 *
 * The factor is initialized based on render resolution percentage, and allows to have same looking
 * blur result when blur size is specified in pixels and user changes render resolution percentage
 * in the interface. */

Also note that the comment describes how the factor affects the behavior. It is more important to know when reading API doc than specific behavior that settings are copied and actual pixel blur size is calculated later on.

Split the setData() and setResolutionFactor() into separate methods.

There is still this ambiguous setData(), but that's how it already was and can be changed in the future.

Sergey Sharybin (sergey) mentioned this in D10126: Fix 50212: Box/Ellipse Mask compositor nodes are affected by render percentage changes.Jan 18 2021, 10:17 AM

Agree that the setData should be refactored for all nodes. It's to generic. However I suggest to do the refactoring in a separate patch and not make it part of this patch.

For this patch I've created a separate method "setResolutionFactor" to set the resolution factor.

Sergey Sharybin (sergey) added inline comments.Jan 25 2021, 10:10 AM

source/blender/compositor/operations/COM_BlurBaseOperation.h
54	For the consistency, call it `m_render_resolution_factor` and initialize to 1.0f in the constructor.

Good one, forgot initialisation.

When blurring render result this patch makes the compositor to behave the way it is expected to.

Unfortunately, there is one case which we didn't think before: blur node applied on a non-render result. For example, it is not impossible that a backdrop plate gets blurred before doing alpha-over of CG element on it. The blut used on the plate should not be scaled with the render resolution factor.

Here is the test file.

comp.blend857 KBDownload

When comparing final render results with percentage of 100% and 25% you can see that the blur on the monkey is scaling nicely, but the blur on backdrop is becoming "wrong" on 25% resolution.

So the problem becomes way of a lot more interesting now, eh ;)

@Monique Dewanchand (mdewanchand), from some quick thoughts, think the issue I've described above will be solved by scaling down non-render-result input images. Such scaling, however, is not really related to this patch. So how about making a branch, where such incremental changes can go in, and once all the main usecases are covered merge it to master? Such approach will allow to incrementally move towards desired goal, keep reviews small and isolated, allow users to test things before they get into release.