Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix off-by-one error when centering a transform. #2760

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Fixes #2753

Fixes the off-by-one error introduced when rotating or skewing images.

Comment on lines +341 to +343
Matrix3x2 matrix = Matrix3x2.CreateTranslation(-sourceRectangle.Location);

foreach (Func<Size, Matrix3x2> factory in this.boundsMatrixFactories)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Matrix3x2 matrix = Matrix3x2.CreateTranslation(-sourceRectangle.Location);
foreach (Func<Size, Matrix3x2> factory in this.boundsMatrixFactories)
Matrix3x2 matrix = Matrix3x2.CreateTranslation(-sourceRectangle.Location);
matrix.Translation += new Vector2(1);
foreach (Func<Size, Matrix3x2> factory in this.transformMatrixFactories)
{

I believe this change should prevent the need of having the 2nd collection of Funcs as I think matrix's for sizing should originate at (+1,+1) from the location of the transform matrix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’ve got me second guessing myself now but I’m not sure that would work. For the most part we want to return the transformed extents based on the explicitly provided information (translate/rotate/scale x, y). It’s only when we perform our own maths to calculate the centre point we go astray. That matrix must be zero based but can then not be used for bounds calculation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed. This example would add an additional unwanted pixel to the right side of the image with the suggested changes.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for validating, just want to make sure we hadn't missed something more simple, this PR looks fine to me then. 👍

@JimBobSquarePants JimBobSquarePants merged commit e0d4576 into release/3.1.x Jul 3, 2024
22 checks passed
@JimBobSquarePants JimBobSquarePants deleted the js/transforms-offset branch July 3, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants