WIP: Fix #118505: Incorrect strip image transformation #125953

Richard Antalik · 2024-08-06T10:47:34+02:00

Richard Antalik commented

2024-08-06 10:47:34 +02:00

When preview is downscaled and transformation origin is not the center
of the image, this causes unexpected offset. This happened, because one
matrix combined image downscaling, so it fits into preview and user
defined scale. When origin was not center of the image, this results in
incorrect offset.

Solved by splitting 1 matrix in sequencer_image_crop_transform_matrix
into 2 matrices. First matrix just centers and scales image to expected
size. Second matrix performs rest of transform operations. This code is
bit easier to read as well.

When preview is downscaled and transformation origin is not the center of the image, this causes unexpected offset. This happened, because one matrix combined image downscaling, so it fits into preview and user defined scale. When origin was not center of the image, this results in incorrect offset. Solved by splitting 1 matrix in sequencer_image_crop_transform_matrix into 2 matrices. First matrix just centers and scales image to expected size. Second matrix performs rest of transform operations. This code is bit easier to read as well.

Richard Antalik added 3 commits 2024-08-06 10:47:46 +02:00

Fix #118505 : Incorrect strip image transformation 7592b4c706

When preview is downscaled and transformation origin is not the center
of the image, this causes unexpected offset.

Solved by splitting 1 matrix in `sequencer_image_crop_transform_matrix`
into 2 matrices. First matrix just centers and scales image to expected
size. Second matrix performs rest of transform operations. This code is
bit easier to read as well.

Polishing 81ef26a52a

Fix unwanted offset 4fb3618417

Richard Antalik commented

2024-08-06 11:35:42 +02:00

@aras_p Hi, I have committed this PR before and it caused transform filter tests to fail due to unwanted offset being introduced. I have investigated this, and resolved the issue, just wanted to double check user-level change with you. The change is, that some images will change position by 1px. This is because currently, we calculate offset image_center_offs_x = (out->x - in->x) / 2, which caused result to be truncated to integer. Having 2 matrices exaggerated this error by scale, which is how I found this.

On user level, this can be checked by adding image with odd size, so that it can not be centered perfectly. Then translating it by +/-0.5px. Currently this would not match the image outline overlay, but with this PR it will. However the behavior may feel odd, because the "remainder" of image centering will be added to translation in side panel. So for example +0.1px will cause image to move, but -0.1px would not.

Perhaps worse side effect of this PR would be, that this "remainder" offset would cause image being interpolated when translation is set to 0.

I am not sure if I would call current state incorrect. I could compensate for this so the PR preserves current behavior, but it makes the code more complicated. Also seq_image_transform_quad_get_ex() would have to be corrected. But the more I think about this, as I am writing, I believe, that previous behavior was good and we should not change it.

Let me know what you think about this. Hopefully I have explained this well enough.

@aras_p Hi, I have committed this PR before and it caused transform filter tests to fail due to unwanted offset being introduced. I have investigated this, and resolved the issue, just wanted to double check user-level change with you. The change is, that some images will change position by 1px. This is because currently, we calculate offset `image_center_offs_x = (out->x - in->x) / 2`, which caused result to be truncated to integer. Having 2 matrices exaggerated this error by scale, which is how I found this. On user level, this can be checked by adding image with odd size, so that it can not be centered perfectly. Then translating it by +/-0.5px. Currently this would not match the image outline overlay, but with this PR it will. However the behavior may feel odd, because the "remainder" of image centering will be added to translation in side panel. So for example +0.1px will cause image to move, but -0.1px would not. Perhaps worse side effect of this PR would be, that this "remainder" offset would cause image being interpolated when translation is set to 0. I am not sure if I would call current state incorrect. I could compensate for this so the PR preserves current behavior, but it makes the code more complicated. Also `seq_image_transform_quad_get_ex()` would have to be corrected. But the more I think about this, as I am writing, I believe, that previous behavior was good and we should not change it. Let me know what you think about this. Hopefully I have explained this well enough.

Aras Pranckevicius commented

2024-08-06 15:16:31 +02:00

@iss hmm not sure indeed which behavior "feels more correct", I could also see arguments for either way :/

Richard Antalik commented

2024-08-07 15:12:17 +02:00

@Sergey, @ZedDB Can you share opinion on my previous comment?

As a reference, compositor does place image off center and does not interpolate it. So I should probably go with that behavior.

@Sergey, @ZedDB Can you share opinion on my previous comment? As a reference, compositor does place image off center and does not interpolate it. So I should probably go with that behavior.

Sebastian Parborg commented

2024-08-07 18:58:44 +02:00

Perhaps worse side effect of this PR would be, that this "remainder" offset would cause image being interpolated when translation is set to 0.

I think that this is probably the worst side effect of this. So as you pointed out, it is probably best to copy what to compositor does.

> Perhaps worse side effect of this PR would be, that this "remainder" offset would cause image being interpolated when translation is set to 0. I think that this is probably the worst side effect of this. So as you pointed out, it is probably best to copy what to compositor does.

Richard Antalik commented

2024-08-10 21:10:41 +02:00

Thanks for input, unfortunately, I don't think it will be possible to fix the issue this way. I can compensate the offset when scaling, but only setting pivot point would not avoid the issue with incorrect interpolation. So I will have to look at different solution

Sergey Sharybin commented

2024-08-14 16:19:13 +02:00

Since a different approach is being investigated, marking as WIP.

Sergey Sharybin changed title from ~~Fix #118505: Incorrect strip image transformation~~ to WIP: Fix #118505: Incorrect strip image transformation

2024-08-14 16:19:22 +02:00

This pull request is marked as a work in progress.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u 118505:iss-118505

git checkout iss-118505

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Fix #118505: Incorrect strip image transformation #125953

Checkout