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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jul 3, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 57 additions & 16 deletions src/ImageSharp/Processing/AffineTransformBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ namespace SixLabors.ImageSharp.Processing;
/// </summary>
public class AffineTransformBuilder
{
private readonly List<Func<Size, Matrix3x2>> matrixFactories = new List<Func<Size, Matrix3x2>>();
private readonly List<Func<Size, Matrix3x2>> transformMatrixFactories = new();
private readonly List<Func<Size, Matrix3x2>> boundsMatrixFactories = new();

/// <summary>
/// Prepends a rotation matrix using the given rotation angle in degrees
Expand All @@ -29,7 +30,9 @@ public AffineTransformBuilder PrependRotationDegrees(float degrees)
/// <param name="radians">The amount of rotation, in radians.</param>
/// <returns>The <see cref="AffineTransformBuilder"/>.</returns>
public AffineTransformBuilder PrependRotationRadians(float radians)
=> this.Prepend(size => TransformUtils.CreateRotationMatrixRadians(radians, size));
=> this.Prepend(
size => TransformUtils.CreateRotationTransformMatrixRadians(radians, size),
size => TransformUtils.CreateRotationBoundsMatrixRadians(radians, size));

/// <summary>
/// Prepends a rotation matrix using the given rotation in degrees at the given origin.
Expand Down Expand Up @@ -65,7 +68,9 @@ public AffineTransformBuilder AppendRotationDegrees(float degrees)
/// <param name="radians">The amount of rotation, in radians.</param>
/// <returns>The <see cref="AffineTransformBuilder"/>.</returns>
public AffineTransformBuilder AppendRotationRadians(float radians)
=> this.Append(size => TransformUtils.CreateRotationMatrixRadians(radians, size));
=> this.Append(
size => TransformUtils.CreateRotationTransformMatrixRadians(radians, size),
size => TransformUtils.CreateRotationBoundsMatrixRadians(radians, size));

/// <summary>
/// Appends a rotation matrix using the given rotation in degrees at the given origin.
Expand Down Expand Up @@ -140,7 +145,9 @@ public AffineTransformBuilder AppendScale(Vector2 scales)
/// <param name="degreesY">The Y angle, in degrees.</param>
/// <returns>The <see cref="AffineTransformBuilder"/>.</returns>
public AffineTransformBuilder PrependSkewDegrees(float degreesX, float degreesY)
=> this.Prepend(size => TransformUtils.CreateSkewMatrixDegrees(degreesX, degreesY, size));
=> this.Prepend(
size => TransformUtils.CreateSkewTransformMatrixDegrees(degreesX, degreesY, size),
size => TransformUtils.CreateSkewBoundsMatrixDegrees(degreesX, degreesY, size));

/// <summary>
/// Prepends a centered skew matrix from the give angles in radians.
Expand All @@ -149,7 +156,9 @@ public AffineTransformBuilder PrependSkewDegrees(float degreesX, float degreesY)
/// <param name="radiansY">The Y angle, in radians.</param>
/// <returns>The <see cref="AffineTransformBuilder"/>.</returns>
public AffineTransformBuilder PrependSkewRadians(float radiansX, float radiansY)
=> this.Prepend(size => TransformUtils.CreateSkewMatrixRadians(radiansX, radiansY, size));
=> this.Prepend(
size => TransformUtils.CreateSkewTransformMatrixRadians(radiansX, radiansY, size),
size => TransformUtils.CreateSkewBoundsMatrixRadians(radiansX, radiansY, size));

/// <summary>
/// Prepends a skew matrix using the given angles in degrees at the given origin.
Expand Down Expand Up @@ -178,7 +187,9 @@ public AffineTransformBuilder PrependSkewRadians(float radiansX, float radiansY,
/// <param name="degreesY">The Y angle, in degrees.</param>
/// <returns>The <see cref="AffineTransformBuilder"/>.</returns>
public AffineTransformBuilder AppendSkewDegrees(float degreesX, float degreesY)
=> this.Append(size => TransformUtils.CreateSkewMatrixDegrees(degreesX, degreesY, size));
=> this.Append(
size => TransformUtils.CreateSkewTransformMatrixDegrees(degreesX, degreesY, size),
size => TransformUtils.CreateSkewBoundsMatrixDegrees(degreesX, degreesY, size));

/// <summary>
/// Appends a centered skew matrix from the give angles in radians.
Expand All @@ -187,7 +198,9 @@ public AffineTransformBuilder AppendSkewDegrees(float degreesX, float degreesY)
/// <param name="radiansY">The Y angle, in radians.</param>
/// <returns>The <see cref="AffineTransformBuilder"/>.</returns>
public AffineTransformBuilder AppendSkewRadians(float radiansX, float radiansY)
=> this.Append(size => TransformUtils.CreateSkewMatrixRadians(radiansX, radiansY, size));
=> this.Append(
size => TransformUtils.CreateSkewTransformMatrixRadians(radiansX, radiansY, size),
size => TransformUtils.CreateSkewBoundsMatrixRadians(radiansX, radiansY, size));

/// <summary>
/// Appends a skew matrix using the given angles in degrees at the given origin.
Expand Down Expand Up @@ -254,7 +267,7 @@ public AffineTransformBuilder AppendTranslation(Vector2 position)
public AffineTransformBuilder PrependMatrix(Matrix3x2 matrix)
{
CheckDegenerate(matrix);
return this.Prepend(_ => matrix);
return this.Prepend(_ => matrix, _ => matrix);
}

/// <summary>
Expand All @@ -270,7 +283,7 @@ public AffineTransformBuilder PrependMatrix(Matrix3x2 matrix)
public AffineTransformBuilder AppendMatrix(Matrix3x2 matrix)
{
CheckDegenerate(matrix);
return this.Append(_ => matrix);
return this.Append(_ => matrix, _ => matrix);
}

/// <summary>
Expand All @@ -281,7 +294,7 @@ public AffineTransformBuilder AppendMatrix(Matrix3x2 matrix)
public Matrix3x2 BuildMatrix(Size sourceSize) => this.BuildMatrix(new Rectangle(Point.Empty, sourceSize));

/// <summary>
/// Returns the combined matrix for a given source rectangle.
/// Returns the combined transform matrix for a given source rectangle.
/// </summary>
/// <param name="sourceRectangle">The rectangle in the source image.</param>
/// <exception cref="DegenerateTransformException">
Expand All @@ -296,11 +309,11 @@ public Matrix3x2 BuildMatrix(Rectangle sourceRectangle)
Guard.MustBeGreaterThan(sourceRectangle.Height, 0, nameof(sourceRectangle));

// Translate the origin matrix to cater for source rectangle offsets.
var matrix = Matrix3x2.CreateTranslation(-sourceRectangle.Location);
Matrix3x2 matrix = Matrix3x2.CreateTranslation(-sourceRectangle.Location);

Size size = sourceRectangle.Size;

foreach (Func<Size, Matrix3x2> factory in this.matrixFactories)
foreach (Func<Size, Matrix3x2> factory in this.transformMatrixFactories)
{
matrix *= factory(size);
}
Expand All @@ -310,6 +323,32 @@ public Matrix3x2 BuildMatrix(Rectangle sourceRectangle)
return matrix;
}

/// <summary>
/// Returns the size of a rectangle large enough to contain the transformed source rectangle.
/// </summary>
/// <param name="sourceRectangle">The rectangle in the source image.</param>
/// <exception cref="DegenerateTransformException">
/// The resultant matrix is degenerate containing one or more values equivalent
/// to <see cref="float.NaN"/> or a zero determinant and therefore cannot be used
/// for linear transforms.
/// </exception>
/// <returns>The <see cref="Size"/>.</returns>
public Size GetTransformedSize(Rectangle sourceRectangle)
{
Size size = sourceRectangle.Size;

// Translate the origin matrix to cater for source rectangle offsets.
Matrix3x2 matrix = Matrix3x2.CreateTranslation(-sourceRectangle.Location);

foreach (Func<Size, Matrix3x2> factory in this.boundsMatrixFactories)
Comment on lines +341 to +343
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. 👍

{
matrix *= factory(size);
CheckDegenerate(matrix);
}

return TransformUtils.GetTransformedSize(size, matrix);
}

private static void CheckDegenerate(Matrix3x2 matrix)
{
if (TransformUtils.IsDegenerate(matrix))
Expand All @@ -318,15 +357,17 @@ private static void CheckDegenerate(Matrix3x2 matrix)
}
}

private AffineTransformBuilder Prepend(Func<Size, Matrix3x2> factory)
private AffineTransformBuilder Prepend(Func<Size, Matrix3x2> transformFactory, Func<Size, Matrix3x2> boundsFactory)
{
this.matrixFactories.Insert(0, factory);
this.transformMatrixFactories.Insert(0, transformFactory);
this.boundsMatrixFactories.Insert(0, boundsFactory);
return this;
}

private AffineTransformBuilder Append(Func<Size, Matrix3x2> factory)
private AffineTransformBuilder Append(Func<Size, Matrix3x2> transformFactory, Func<Size, Matrix3x2> boundsFactory)
{
this.matrixFactories.Add(factory);
this.transformMatrixFactories.Add(transformFactory);
this.boundsMatrixFactories.Add(boundsFactory);
return this;
}
}
4 changes: 2 additions & 2 deletions src/ImageSharp/Processing/Extensions/Transforms/TransformExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public static IImageProcessingContext Transform(
IResampler sampler)
{
Matrix3x2 transform = builder.BuildMatrix(sourceRectangle);
Size targetDimensions = TransformUtils.GetTransformedSize(sourceRectangle.Size, transform);
Size targetDimensions = builder.GetTransformedSize(sourceRectangle);
return source.Transform(sourceRectangle, transform, targetDimensions, sampler);
}

Expand Down Expand Up @@ -113,7 +113,7 @@ public static IImageProcessingContext Transform(
IResampler sampler)
{
Matrix4x4 transform = builder.BuildMatrix(sourceRectangle);
Size targetDimensions = TransformUtils.GetTransformedSize(sourceRectangle.Size, transform);
Size targetDimensions = builder.GetTransformedSize(sourceRectangle);
return source.Transform(sourceRectangle, transform, targetDimensions, sampler);
}

Expand Down
8 changes: 4 additions & 4 deletions src/ImageSharp/Processing/Processors/Transforms/Linear/LinearTransformUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ public static float GetSamplingRadius<TResampler>(in TResampler sampler, int sou
/// </summary>
/// <param name="radius">The radius.</param>
/// <param name="center">The center position.</param>
/// <param name="min">The min allowed amouunt.</param>
/// <param name="max">The max allowed amouunt.</param>
/// <param name="min">The min allowed amount.</param>
/// <param name="max">The max allowed amount.</param>
/// <returns>The <see cref="int"/>.</returns>
[MethodImpl(InliningOptions.ShortMethod)]
public static int GetRangeStart(float radius, float center, int min, int max)
Expand All @@ -51,8 +51,8 @@ public static int GetRangeStart(float radius, float center, int min, int max)
/// </summary>
/// <param name="radius">The radius.</param>
/// <param name="center">The center position.</param>
/// <param name="min">The min allowed amouunt.</param>
/// <param name="max">The max allowed amouunt.</param>
/// <param name="min">The min allowed amount.</param>
/// <param name="max">The max allowed amount.</param>
/// <returns>The <see cref="int"/>.</returns>
[MethodImpl(InliningOptions.ShortMethod)]
public static int GetRangeEnd(float radius, float center, int min, int max)
Expand Down
7 changes: 4 additions & 3 deletions src/ImageSharp/Processing/Processors/Transforms/Linear/RotateProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ public RotateProcessor(float degrees, Size sourceSize)
/// <param name="sourceSize">The source image size</param>
public RotateProcessor(float degrees, IResampler sampler, Size sourceSize)
: this(
TransformUtils.CreateRotationMatrixDegrees(degrees, sourceSize),
TransformUtils.CreateRotationTransformMatrixDegrees(degrees, sourceSize),
TransformUtils.CreateRotationBoundsMatrixDegrees(degrees, sourceSize),
sampler,
sourceSize)
=> this.Degrees = degrees;

// Helper constructor
private RotateProcessor(Matrix3x2 rotationMatrix, IResampler sampler, Size sourceSize)
: base(rotationMatrix, sampler, TransformUtils.GetTransformedSize(sourceSize, rotationMatrix))
private RotateProcessor(Matrix3x2 rotationMatrix, Matrix3x2 boundsMatrix, IResampler sampler, Size sourceSize)
: base(rotationMatrix, sampler, TransformUtils.GetTransformedSize(sourceSize, boundsMatrix))
{
}

Expand Down
7 changes: 4 additions & 3 deletions src/ImageSharp/Processing/Processors/Transforms/Linear/SkewProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ public SkewProcessor(float degreesX, float degreesY, Size sourceSize)
/// <param name="sourceSize">The source image size</param>
public SkewProcessor(float degreesX, float degreesY, IResampler sampler, Size sourceSize)
: this(
TransformUtils.CreateSkewMatrixDegrees(degreesX, degreesY, sourceSize),
TransformUtils.CreateSkewTransformMatrixDegrees(degreesX, degreesY, sourceSize),
TransformUtils.CreateSkewBoundsMatrixDegrees(degreesX, degreesY, sourceSize),
sampler,
sourceSize)
{
Expand All @@ -39,8 +40,8 @@ public SkewProcessor(float degreesX, float degreesY, IResampler sampler, Size so
}

// Helper constructor:
private SkewProcessor(Matrix3x2 skewMatrix, IResampler sampler, Size sourceSize)
: base(skewMatrix, sampler, TransformUtils.GetTransformedSize(sourceSize, skewMatrix))
private SkewProcessor(Matrix3x2 skewMatrix, Matrix3x2 boundsMatrix, IResampler sampler, Size sourceSize)
: base(skewMatrix, sampler, TransformUtils.GetTransformedSize(sourceSize, boundsMatrix))
{
}

Expand Down
Loading
Loading