Fix #124842: Made CPU transforms match the GPU ones #125267

Closed
Bill-Spitzak wants to merge 3 commits from Bill-Spitzak:resolveshift into main

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

Compared to previous pull request, this one is split into more individual commits to see more clearly what is going on, and unnecessary changes have been removed. This also does not include fixes to the GPU transforms, that is a different pull request.

Fixes a number of off-by-½ errors, and incorrect clipping.

Compared to previous pull request, this one is split into more individual commits to see more clearly what is going on, and unnecessary changes have been removed. This also does not include fixes to the GPU transforms, that is a different pull request. Fixes a number of off-by-½ errors, and incorrect clipping.
Bill-Spitzak added 6 commits 2024-07-23 02:08:17 +02:00
Remove off-by-.5 errors and previous attempts to fix them by moving the rotation center.
The bounding box is wrong (too large) but it does not seem worthwhile to fix this.
In particular this makes Scale Absolute produce the size the user
requested, previously it was adding 1 to the size.
Fix off-by-.5 errors. Merged two loops which were otherwise identical,
and extracted the resulting addition factor for the linear equation and
compute it outside the loop.
This uses the math_interp.cc code with Extend (or Repeat if possible)
and then multiplies by a correctly-antialiased clipping rectangle. I had
to reverse which function calls the other so that the two extend parameters
were accessible.

The Nearest sampling also had an off-by-.5 error. This is probably a mistake
in math_interp but I changed the calling code instead.
This one was also wrong at the right/top edges. Make it reuse the code in read().
This removes any risk that it might differ from the float version.
Bill-Spitzak requested review from Omar Emara 2024-07-23 02:08:54 +02:00
Member

@Bill-Spitzak Thanks for splitting the code into individual commits. I will go over them one by one since we probably need to update the tests for each.

@Bill-Spitzak Thanks for splitting the code into individual commits. I will go over them one by one since we probably need to update the tests for each.
Bill-Spitzak force-pushed resolveshift from a0e5840e47 to ff54811f38 2024-07-23 20:43:19 +02:00 Compare
Author
Contributor

Push is a trivial code change to the nearest sampling that does not change behavior. I did not want xy to have different values in the different case statements.

Push is a trivial code change to the nearest sampling that does not change behavior. I did not want xy to have different values in the different case statements.
Omar Emara requested changes 2024-07-25 12:12:21 +02:00
Dismissed
@ -71,3 +70,1 @@
area.xmax = ceilf(scale_coord(area.xmax, center_x, relative_scale_x));
area.ymin = floorf(scale_coord(area.ymin, center_y, relative_scale_y));
area.ymax = ceilf(scale_coord(area.ymax, center_y, relative_scale_y));
area.xmin = floorf(scale_coord(area.xmin, center_x, relative_scale_x) + 0.1f);
Member

This fails for some scales I tried and doesn't seem very robust. How about we compute the minimum x and y as is then add the scaled width and height? Something like:

diff --git a/source/blender/compositor/operations/COM_ScaleOperation.cc b/source/blender/compositor/operations/COM_ScaleOperation.cc
index 8003b143d6d..4338448e1af 100644
--- a/source/blender/compositor/operations/COM_ScaleOperation.cc
+++ b/source/blender/compositor/operations/COM_ScaleOperation.cc
@@ -67,10 +67,12 @@ void ScaleOperation::scale_area(rcti &area, float relative_scale_x, float relati
   const rcti src_area = area;
   const float center_x = BLI_rcti_size_x(&area) / 2.0f;
   const float center_y = BLI_rcti_size_y(&area) / 2.0f;
+  const int2 new_size = int2(BLI_rcti_size_x(&area) * relative_scale_x,
+                             BLI_rcti_size_y(&area) * relative_scale_y);
   area.xmin = floorf(scale_coord(area.xmin, center_x, relative_scale_x));
-  area.xmax = ceilf(scale_coord(area.xmax, center_x, relative_scale_x));
+  area.xmax = area.xmin + new_size.x;
   area.ymin = floorf(scale_coord(area.ymin, center_y, relative_scale_y));
-  area.ymax = ceilf(scale_coord(area.ymax, center_y, relative_scale_y));
+  area.ymax = area.ymin + new_size.y;

   float scale_offset_x, scale_offset_y;
   ScaleOperation::get_scale_offset(src_area, area, scale_offset_x, scale_offset_y);
This fails for some scales I tried and doesn't seem very robust. How about we compute the minimum x and y as is then add the scaled width and height? Something like: ```diff diff --git a/source/blender/compositor/operations/COM_ScaleOperation.cc b/source/blender/compositor/operations/COM_ScaleOperation.cc index 8003b143d6d..4338448e1af 100644 --- a/source/blender/compositor/operations/COM_ScaleOperation.cc +++ b/source/blender/compositor/operations/COM_ScaleOperation.cc @@ -67,10 +67,12 @@ void ScaleOperation::scale_area(rcti &area, float relative_scale_x, float relati const rcti src_area = area; const float center_x = BLI_rcti_size_x(&area) / 2.0f; const float center_y = BLI_rcti_size_y(&area) / 2.0f; + const int2 new_size = int2(BLI_rcti_size_x(&area) * relative_scale_x, + BLI_rcti_size_y(&area) * relative_scale_y); area.xmin = floorf(scale_coord(area.xmin, center_x, relative_scale_x)); - area.xmax = ceilf(scale_coord(area.xmax, center_x, relative_scale_x)); + area.xmax = area.xmin + new_size.x; area.ymin = floorf(scale_coord(area.ymin, center_y, relative_scale_y)); - area.ymax = ceilf(scale_coord(area.ymax, center_y, relative_scale_y)); + area.ymax = area.ymin + new_size.y; float scale_offset_x, scale_offset_y; ScaleOperation::get_scale_offset(src_area, area, scale_offset_x, scale_offset_y); ```
Author
Contributor

Probably a good idea to force the size, my version could move one edge when not moving the other, which I think is what produced your errors. Any particular scale or input size that you observed failing?

I think the offsets are still needed for the floorf() so the whole box does not shift left due to rounding errors. And I'm a little concerned that int2() is not what is wanted to get new_size and some rounding or ceil is needed.

Looking at this a bit more, I think perhaps a more proper fix is to make ScaleFixedSizeOperation::determine_canvas directly output the width/height, and not mess with other transforms at all.

Probably a good idea to force the size, my version could move one edge when not moving the other, which I think is what produced your errors. Any particular scale or input size that you observed failing? I think the offsets are still needed for the `floorf()` so the whole box does not shift left due to rounding errors. And I'm a little concerned that `int2()` is not what is wanted to get `new_size` and some rounding or ceil is needed. Looking at this a bit more, I think perhaps a more proper fix is to make ScaleFixedSizeOperation::determine_canvas directly output the width/height, and not mess with other transforms at all.
Author
Contributor

Actually now I'm thinking of removing this. The pixels are ending up in the right place, it's just that the bounding box is somewhat wrong (which also means that a second scale to a fixed size will be wrong). I encountered this while testing what the CPU compositor does with even/odd sized domains but it seems rather unimportant for real use.

Actually now I'm thinking of removing this. The pixels are ending up in the right place, it's just that the bounding box is somewhat wrong (which also means that a second scale to a fixed size will be wrong). I encountered this while testing what the CPU compositor does with even/odd sized domains but it seems rather unimportant for real use.
Bill-Spitzak force-pushed resolveshift from ff54811f38 to 18e9eecb8c 2024-07-25 21:29:45 +02:00 Compare
Author
Contributor

I pushed a new version that skips the steps you already applied and removes the bouding box changes for Scale.

I pushed a new version that skips the steps you already applied and removes the bouding box changes for Scale.
Omar Emara requested changes 2024-07-26 11:49:41 +02:00
Dismissed
@ -542,0 +435,4 @@
const float w = get_width();
const float h = get_height();
if (sampler == PixelSampler::Nearest) {
Member

Turn this into a guard condition and eliminate the else.

Turn this into a guard condition and eliminate the else.
Bill-Spitzak marked this conversation as resolved
@ -546,2 +447,2 @@
this->wrap_pixel(u, v, extend_x, extend_y);
this->read_elem_sampled(u, v, sampler, result);
// compute (linear interpolation) intersection with Clip
float m = 1.0f;
Member

Give m a more meaningful name.

Give `m` a more meaningful name.
Bill-Spitzak marked this conversation as resolved
@ -548,0 +456,4 @@
clear_elem(result);
return;
}
if (is_a_single_elem_) {
Member

This should probably be at the very start of the function. Blender might crash if sampler is nearest and is_a_single_elem_ is true. Much like read_elem_bicubic_bspline does.

This should probably be at the very start of the function. Blender might crash if sampler is nearest and `is_a_single_elem_` is true. Much like `read_elem_bicubic_bspline` does.
Author
Contributor

The border condition is slightly different for Nearest than for the antialiased version.
However it could calculate this 'm' value for both and use a different threshold.

The border condition is slightly different for Nearest than for the antialiased version. However it could calculate this 'm' value for both and use a different threshold.
Author
Contributor

Futher investigation made me not use math::interpolate_nearest_fl as I am worried about it making a different decision about 0.5 than the clipping code. Uses the old function which does Extend always and works with is_a_single_elem_.

Futher investigation made me not use `math::interpolate_nearest_fl` as I am worried about it making a different decision about 0.5 than the clipping code. Uses the old function which does Extend always and works with `is_a_single_elem_`.
Bill-Spitzak marked this conversation as resolved
@ -548,0 +461,4 @@
}
else if (sampler == PixelSampler::Bilinear) {
// Sample using Extend or Repeat
math::interpolate_bilinear_wrap_fl(buffer_, result, w, h, num_channels_, x, y,
Member

Your code is not formatted. Please run clang format for all code: https://developer.blender.org/docs/handbook/tooling/clangformat/.

Your code is not formatted. Please run clang format for all code: https://developer.blender.org/docs/handbook/tooling/clangformat/.
Bill-Spitzak marked this conversation as resolved
@ -548,0 +467,4 @@
}
else { // PixelSampler::Bicubic
// Wrap is not implemented in interpolate_cubic_bspline, this uses Extend always.
// This will produce minor errors in pixels next to wrapped borders.
Member

Is the issue described in the comment and existing issue? Or is it introduced by the patch?

Is the issue described in the comment and existing issue? Or is it introduced by the patch?
Author
Contributor

Existing issue. Bicubic gets the other pixel samples for the filter using Extend mode and does not implement Repeat.

Existing issue. Bicubic gets the other pixel samples for the filter using Extend mode and does not implement Repeat.
Bill-Spitzak marked this conversation as resolved
Member

@Bill-Spitzak We have tests for the CPU compositor, this might help you catch errors, like the crash for the compositor-nodes-desintegrate-wipe-01 tests. https://developer.blender.org/docs/handbook/testing/setup/

@Bill-Spitzak We have tests for the CPU compositor, this might help you catch errors, like the crash for the `compositor-nodes-desintegrate-wipe-01` tests. https://developer.blender.org/docs/handbook/testing/setup/
Bill-Spitzak force-pushed resolveshift from 18e9eecb8c to 9da8be538a 2024-07-27 18:22:35 +02:00 Compare
Author
Contributor

Updated with new version to address comments.

It would be nice if all the functions were consistent about whether minx/y are subtracted from the index!

Updated with new version to address comments. It would be nice if all the functions were consistent about whether `minx/y` are subtracted from the index!
Omar Emara requested changes 2024-07-31 14:19:53 +02:00
Dismissed
@ -548,0 +454,4 @@
}
if (is_a_single_elem_) {
memcpy(result, buffer_, get_elem_bytes_len());
Member

I still don't get why this isn't at the top. It might be confusing considering the code in read_elem_bilinear. But single element buffers are dimensionless and shouldn't have boundaries or boundary handling, they should be return as is.

I still don't get why this isn't at the top. It might be confusing considering the code in `read_elem_bilinear`. But single element buffers are dimensionless and shouldn't have boundaries or boundary handling, they should be return as is.
Author
Contributor

It's not there because I wanted Clip to work for constant buffers.

However it looks like the design is to ignore the extend for constant buffers. This is what the GPU is doing. So it seems ok to change it like you are requesting. I was unable to make a script that caused the CPU to hit this code so it was difficult to tell what the previous version was doing.

In the long term (as part of your refactor, I guess) you must add a size to constants and have clip work on them. Clipping also has to be antialiased at the output resolution, not the input, as this is what users expect (sometimes pretty vehemently) and it is required to avoid black leaking in from the edges of scales. Therefore I believe this can be cleanly done by making a 1x1 domain but setting the transform so that pixel covers the entire bounding box.

It's not there because I wanted Clip to work for constant buffers. However it looks like the design is to ignore the extend for constant buffers. This is what the GPU is doing. So it seems ok to change it like you are requesting. I was unable to make a script that caused the CPU to hit this code so it was difficult to tell what the previous version was doing. In the long term (as part of your refactor, I guess) you must add a size to constants and have clip work on them. Clipping also has to be antialiased at the output resolution, not the input, as this is what users expect (sometimes pretty vehemently) and it is required to avoid black leaking in from the edges of scales. Therefore I believe this can be cleanly done by making a 1x1 domain but setting the transform so that pixel covers the entire bounding box.
Author
Contributor

I pushed a new version that moves the constant test to the top of the function. Added a comment indicating that ignoring extend is on purpose.

I pushed a new version that moves the constant test to the top of the function. Added a comment indicating that ignoring extend is on purpose.
Bill-Spitzak marked this conversation as resolved
Bill-Spitzak force-pushed resolveshift from 9da8be538a to ace542d0ad 2024-07-31 19:58:02 +02:00 Compare
Bill-Spitzak force-pushed resolveshift from ace542d0ad to f7a12440c0 2024-07-31 21:17:56 +02:00 Compare
Author
Contributor

Further push to remove change to GPU code that was merged to this branch for testing.

Further push to remove change to GPU code that was merged to this branch for testing.
Omar Emara approved these changes 2024-08-01 13:23:48 +02:00
Member

Committed and tests updated in ef1d22aea5, 1479c9cde6, and 984feb48fc.

Committed and tests updated in ef1d22aea57ff2bd9f8d3dab922d7d9727d9a03b, 1479c9cde6cdc865b6028e1abd808f5a83ff5e5a, and 984feb48fce2e973705b242a1c6a42fbc8c7955c.
Omar Emara closed this pull request 2024-08-01 13:24:34 +02:00
Bill-Spitzak deleted branch resolveshift 2024-08-01 23:07:11 +02:00

Pull request closed

Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
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#125267
No description provided.