Fix #124842: Made CPU transforms match the GPU ones #125267
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#125267
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "Bill-Spitzak:resolveshift"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 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.
a0e5840e47
toff54811f38
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.
@ -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);
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:
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 thatint2()
is not what is wanted to getnew_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.
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.
ff54811f38
to18e9eecb8c
I pushed a new version that skips the steps you already applied and removes the bouding box changes for Scale.
@ -542,0 +435,4 @@
const float w = get_width();
const float h = get_height();
if (sampler == PixelSampler::Nearest) {
Turn this into a guard condition and eliminate the else.
@ -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;
Give
m
a more meaningful name.@ -548,0 +456,4 @@
clear_elem(result);
return;
}
if (is_a_single_elem_) {
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 likeread_elem_bicubic_bspline
does.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.
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 withis_a_single_elem_
.@ -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,
Your code is not formatted. Please run clang format for all code: https://developer.blender.org/docs/handbook/tooling/clangformat/.
@ -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.
Is the issue described in the comment and existing issue? Or is it introduced by the patch?
Existing issue. Bicubic gets the other pixel samples for the filter using Extend mode and does not implement Repeat.
@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/18e9eecb8c
to9da8be538a
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!@ -548,0 +454,4 @@
}
if (is_a_single_elem_) {
memcpy(result, buffer_, get_elem_bytes_len());
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.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.
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.
9da8be538a
toace542d0ad
ace542d0ad
tof7a12440c0
Further push to remove change to GPU code that was merged to this branch for testing.
Committed and tests updated in
ef1d22aea5
,1479c9cde6
, and984feb48fc
.Pull request closed