Fix Split node in full-frame compositor when no inputs are connected #117282

Closed

Sergey Sharybin wants to merge 1 commits from Sergey:compositor_fix_split_node into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Sergey Sharybin commented

2024-01-18 15:04:02 +01:00

Owner

Not having inputs connected triggers the constant folding code paths
in the operation builder, which attempts to convert the operation to
a constant memory buffer. This happens before the canvas of the node
is known, and the expected result in area of 1x1 pixels.

The split node can not be constant-folded in such way without causing
regression compared to the tiled compositor: for unconnected inputs
the node is expected to use the output resolution as canvas.

IN theory the node can be constant folded if the split percentage is
0 or 1, but there is no mechanism which allows such option-dependent
constant folding check.

This fixes node_split_unlinked.blend regression test which fails when
full-frame compositor is used by default.

Not having inputs connected triggers the constant folding code paths in the operation builder, which attempts to convert the operation to a constant memory buffer. This happens before the canvas of the node is known, and the expected result in area of 1x1 pixels. The split node can not be constant-folded in such way without causing regression compared to the tiled compositor: for unconnected inputs the node is expected to use the output resolution as canvas. IN theory the node can be constant folded if the split percentage is 0 or 1, but there is no mechanism which allows such option-dependent constant folding check. This fixes node_split_unlinked.blend regression test which fails when full-frame compositor is used by default.

Sergey Sharybin added the

labels 2024-01-18 15:04:02 +01:00

Sergey Sharybin added 1 commit 2024-01-18 15:04:11 +01:00

2af4c291fd Fix Split node in full-frame compositor when no inputs are connected

Not having inputs connected triggers the constant folding code paths
in the operation builder, which attempts to convert the operation to
a constant memory buffer. This happens before the canvas of the node
is known, and the expected result in area of 1x1 pixels.

The split node can not be constant-folded in such way without causing
regression compared to the tiled compositor: for unconnected inputs
the node is expected to use the output resolution as canvas.

IN theory the node can be constant folded if the split percentage is
0 or 1, but there is no mechanism which allows such option-dependent
constant folding check.

This fixes node_split_unlinked.blend regression test which fails when
full-frame compositor is used by default.

Sergey Sharybin commented

2024-01-18 15:06:06 +01:00

Author

Owner

To better demonstrate the regression test which was failing prior to this fix:

To better demonstrate the regression test which was failing prior to this fix: ![Screenshot 2024-01-18 at 15.05.19.png](/attachments/8e828809-74f8-477a-87ca-914069c946dd)

Screenshot 2024-01-18 at 15.05.19.png

136 KiB

Sergey Sharybin requested review from Omar Emara 2024-01-18 15:06:15 +01:00

Omar Emara commented

2024-01-18 15:12:12 +01:00

Member

This is actually the intended behavior, once the node became a processing node, and not an output node, there is no reason for it to use the output resolution as the canvas. The use case of having a split node with nothing connected is not that realistic, so I don't see why we should break the left to right evaluation design for this particular node.

This is actually the intended behavior, once the node became a processing node, and not an output node, there is no reason for it to use the output resolution as the canvas. The use case of having a split node with nothing connected is not that realistic, so I don't see why we should break the left to right evaluation design for this particular node.

Omar Emara commented

2024-01-18 15:14:28 +01:00

Member

The general design rule is that only input and output nodes should assume a canvas, while processing nodes should compute their canvas from their inputs only, not their outputs or some other global property like the render resolution.

The general design rule is that only input and output nodes should assume a canvas, while processing nodes should compute their canvas from their inputs only, not their outputs or some other global property like the render resolution.

Sergey Sharybin commented

2024-01-18 16:42:42 +01:00

Author

Owner

The strict left-to-right rule can only be easily applied when you start with image input, and start processing it. This is something that was not strict left-to-right in the tiled compositor. When you start doing procedural aspects, you do need to allow canvas or domain to flow from right to left. While this might not be the same as other compositing software handles procedural aspects, allowing fields in compositor allows ease of node group sharing across different systems (shaders, geometry nodes, compositor in the future). But that's a bit separate discussion.

In any case, I do not see that we violate any design. You can re-formulate this graph in the following way:

[RGB Node]  -> [Split Node] -> [Viewer]
[RGB Node]  ->

It is the RGB nodes which define canvas, and they do it based on the output canvas. Following the design:

Input RGB nodes determine their canvas to the output (just as they'll do it if they are connected directly to the viewer, or via Brightness/Contrast node)
The RGB nodes effectively output an image of the determined canvas resolution
The Split node selects first input for the top part, second input for the bottom, based on the fact

Even sematically, the Split node can only constant if its factor is either 0 or 1. But even then, it is not so much a constant fold, but re-re-wiring the node out of the graph entirely. In any case, graph evaluation should give exactly the same results, no matter if something got constant folded, or got reviewed (unless there is a bug in the constant folding, or in the rewiting part).

So from the design and user expectation point of view the current Tiled behavior is the correct one, and the current Full-Frame is not.
And even if we say the current behavior of it is correct (which I don't agree with) it should not achieved by marking node as constant-foldable. You should be able to comment out constant folding part of the convert_to_operations() without affecting the render result, which is not the case here.

The strict left-to-right rule can only be easily applied when you start with image input, and start processing it. This is something that was not strict left-to-right in the tiled compositor. When you start doing procedural aspects, you do need to allow canvas or domain to flow from right to left. While this might not be the same as other compositing software handles procedural aspects, allowing fields in compositor allows ease of node group sharing across different systems (shaders, geometry nodes, compositor in the future). But that's a bit separate discussion. In any case, I do not see that we violate any design. You can re-formulate this graph in the following way: ``` [RGB Node] -> [Split Node] -> [Viewer] [RGB Node] -> ``` It is the RGB nodes which define canvas, and they do it based on the output canvas. Following the design: - Input RGB nodes determine their canvas to the output (just as they'll do it if they are connected directly to the viewer, or via Brightness/Contrast node) - The RGB nodes effectively output an image of the determined canvas resolution - The Split node selects first input for the top part, second input for the bottom, based on the fact Even sematically, the Split node can only constant if its factor is either 0 or 1. But even then, it is not so much a constant fold, but re-re-wiring the node out of the graph entirely. In any case, graph evaluation should give exactly the same results, no matter if something got constant folded, or got reviewed (unless there is a bug in the constant folding, or in the rewiting part). So from the design and user expectation point of view the current Tiled behavior is the correct one, and the current Full-Frame is not. And even if we say the current behavior of it is correct (which I don't agree with) it should not achieved by marking node as constant-foldable. You should be able to comment out constant folding part of the `convert_to_operations()` without affecting the render result, which is not the case here.

Omar Emara commented

2024-01-18 17:04:45 +01:00

Member

You are correct in that constant folding is wrong here, but that is just because we currently abuse it to achieve single value processing. And we do that temporarily to avoid rewriting many parts of the CPU compositor.

I strongly disagree that nodes like RGB should effectively output images that assume the output resolution. RGB should output a single value, and it should be processed as a single value. This is not just done as an optimization but also as a clear design. Otherwise you get reports like #97385, which relies on the behavior you described but at the same time finds it conflusing.

I am surprised that we are now going in the direction of field evaluation and right to left evaluation, while so far I thought we were pursuing left to right evaluation.

You are correct in that constant folding is wrong here, but that is just because we currently abuse it to achieve single value processing. And we do that temporarily to avoid rewriting many parts of the CPU compositor. I strongly disagree that nodes like RGB should effectively output images that assume the output resolution. RGB should output a single value, and it should be processed as a single value. This is not just done as an optimization but also as a clear design. Otherwise you get reports like #97385, which relies on the behavior you described but at the same time finds it conflusing. I am surprised that we are now going in the direction of field evaluation and right to left evaluation, while so far I thought we were pursuing left to right evaluation.

Sergey Sharybin commented

2024-01-18 17:50:31 +01:00

Author

Owner

You are correct in that constant folding is wrong here, but that is just because we currently abuse it to achieve single value processing. And we do that temporarily to avoid rewriting many parts of the CPU compositor.

Oh man. That is quite confusing. Maybe we should add a note around the constant folding code block?
Having an unintuitive but documented behavior of graph simplification will be much better than having it uninutitive and undocumented, and would have save some time :)

I strongly disagree that nodes like RGB should effectively output images that assume the output resolution. RGB should output a single value, and it should be processed as a single value. This is not just done as an optimization but also as a clear design. Otherwise you get reports like #97385, which relies on the behavior you described but at the same time finds it conflusing.

For a lot of cases proper and clear solution would be to have explicit "Set Format" node (think this is how we called it in #108944).

I am surprised that we are now going in the direction of field evaluation and right to left evaluation, while so far I thought we were pursuing left to right evaluation.

Field is evaluated from left to right, its only its domain is propagated from right to left.

> You are correct in that constant folding is wrong here, but that is just because we currently abuse it to achieve single value processing. And we do that temporarily to avoid rewriting many parts of the CPU compositor. Oh man. That is quite confusing. Maybe we should add a note around the constant folding code block? Having an unintuitive but documented behavior of graph simplification will be much better than having it uninutitive and undocumented, and would have save some time :) > I strongly disagree that nodes like RGB should effectively output images that assume the output resolution. RGB should output a single value, and it should be processed as a single value. This is not just done as an optimization but also as a clear design. Otherwise you get reports like #97385, which relies on the behavior you described but at the same time finds it conflusing. For a lot of cases proper and clear solution would be to have explicit "Set Format" node (think this is how we called it in #108944). > I am surprised that we are now going in the direction of field evaluation and right to left evaluation, while so far I thought we were pursuing left to right evaluation. Field is evaluated from left to right, its only its domain is propagated from right to left.

Sergey Sharybin closed this pull request

2024-01-18 17:50:38 +01:00

Omar Emara commented

2024-01-18 18:07:35 +01:00

Member

Oh man. That is quite confusing.

@Sergey Yah, hopefully this should be resolved once we reuse the same scheduler/evaluator across compositors.

> Oh man. That is quite confusing. @Sergey Yah, hopefully this should be resolved once we reuse the same scheduler/evaluator across compositors.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#117282

No description provided.