Refactored AffineTransform towards System.Numerics.Matrix3x2

[vpenades]

• Refactored AffineTransform property names to match those of System.Numerics.Matrix3x2, System.Windows.Media.Matrix and so on.
  • ScaleX, ShearX properties are gone in favour of standarised M11, M12, M21, M22 matrix properties.
• Added Translation, Rotation and Scaling properties to AffineTransform.
  • These properties can be get or set irrespective of the state of the others. Special care has been taken to consider negative scales.
• Added UnitTests for the related changes.
• Added the method Deconstruct to get scale, rotation and translation from an affineTransform. Also added GetInstance to reconstruct an affinetransform with equivalent parameters.
  • It's important to take in account that an orthogonal matrix can be deconstructed into two valid solutions, so while being correct, the deconstructed output TRS does not necessarily need to match the original TRS used to contruct the matrix.

Considerations:

• I've called the scale property "Scaling" instead of "Scale" because a Scale(...) method already exists. I would consider renaming the methods Scale, Rotate and Translate to ConcatenateScale, ConcatenateRotation and ConcatenateTranslation, which would allow renaming "Scaling" to "Scale".
• I've noticed the concatenation order of AffineTransform is in reverse order than of Matrix3x2 , it's fine once you know it, but it tricked me for a while, I'm sure it'll happen the same to others.
• I've noticed in some places ( AbstractCanvas and ScalingCanvas ) the scale was being taken by AffineTransform.M11 (previously ScaleX). I am not completely sure why I'ts needed, but I suspect it's used to get the "average scaling" of the transform, to be used in line thinkness, etc. using only M11 would have only worked with an unrotated, unscaled matrix. I've introduced the property "AverageScaling" which returns the average scale provided by the transform, irrespective of the axial scales and rotation.
• I've seen methods like SetMatrix(float[]) and Transform(float[] src, int offset, float[] dst...); ... I would seriously consider using System.Memory and changing all the array arguments in all the methods to Span and ReadOnlySpan.
• I still dislike having AffineTransform around, I understand it's not possible to replace it by Matrix3x2 at this point... but given that AffineTransform can be converted to Matrix3x2 implicitly if needed, I would replace all the methods taking AffineTransform as input by Matrix3x2, an example of this would be ICanvas.ConcatenateTransform(AffineTransform transform); with ICanvas.ConcatenateTransform(in Matrix3x2 transform);

As usual, feel free to add/remove, comment anything you need.... also I humbly request to throughfully test all that code, since it's quite critical stuff.... 2D graphics with odd scalings and rotations are quite tricky to handle.

[jonlipsky]

@vpenades I like your suggestion about changing the method names to be "ConcatenateScale", etc... Would you open to making those changes as well as part of your PR?

Regarding why ScalingCanvas needs to track the scale, it's because in Android and Skia for some drawing operations if you scale and the draw some of the vector shapes, you end up with pixelated results. By tracking the scale, we can simulate what CoreGraphics does and do the scaling ourself so that we get better visual results.

Great idea on switching to using a Span. This library pre-dates the existence of those classes. I'd welcome that change.

[vpenades]

@vpenades I like your suggestion about changing the method names to be "ConcatenateScale", etc... Would you open to making those changes as well as part of your PR?

Sure, I'll do the changes

Regarding why ScalingCanvas needs to track the scale, it's because in Android and Skia for some drawing operations if you scale and the draw some of the vector shapes, you end up with pixelated results. By tracking the scale, we can simulate what CoreGraphics does and do the scaling ourself so that we get better visual results.

Ok, I understand, I hope the "AverageScale" property works for these case scenarios.

Great idea on switching to using a Span. This library pre-dates the existence of those classes. I'd welcome that change.

Also, I've seen the method GetMatrix(float[] ) which is also misleading... does it mean getting the data into the AffineTransform? I would change it to CopyTo(span ...);

[jonlipsky]

@vpenades CopyTo(span) is a much better name. It's intent is to get the values from the Matrix/Transform copied into an existing array of floats.

[vpenades]

Sadly, adding System.Memory to allow supporting Span and ReadonlySpan was unsuccessful, because apparently Tizen40 is failing to compile. Maybe it's related to my dev environment. So I'll skip this one until someone adds System.Memory nuget package

[vpenades]

I've done with renaming the methods agreed. let me know if you see something else that can be improved.

[jonlipsky]

Thanks. I'll look at the PR tonight. Really appreciate all of your contributions!!!

[vpenades]

Would you consider making AffineTransform inmutable? people are used to deal with matrices "by value" and not "by ref". By being a class you have some advantages, but class+mutable is quite dangerous to say the least.

Consider this example:

var t = AffineTransform.GetScaleInstance(5,5);

var state1 = new CanvasState();
state1.Transform = t;

t.RotateInDegrees(90);

var state2 = new CanvasState();
state2.Transform = t;

In this case, one can believe that state1 has a Scale(5,5) and state2 has a Scale(5,5)xRotate(90) , but in fact, since the reference used in state1.Transform is dragged on, the following RotateInDegrees operation is still affecting state1.Transform.

To avoid this kind of scenarios, the best approach is to make AffineTransform inmutable, so every operation will produce a new copy.

[SparkieLabs]

I think this PR doesn't go far enough as I can't see the point of this class as .NET already has Matrix3x2, and soon some generic versions as well, which have been through a full api review, see dotnet/runtime#24168.

If this project adopted Matrix3x2 it would benefit from the work already done on SIMD h/w acceleration and make code sharing across .NET libraries easier. Introducing another overlapping class just confuses developers.

As this project is still experimental, now is the time to make such changes to put it in a stronger position for the long-term.

I say this as I want Microsoft.Maui.Graphics to thrive!

[vpenades]

If this project adopted Matrix3x2 it would benefit from the work already done on SIMD h/w acceleration and make code sharing across .NET libraries easier. Introducing another overlapping class just confuses developers.

I completely agree with you, but @jonlipsky has the last word on this.

So if he agrees on moving to Matrix3x2, I'll be happy to do the changes and add appropiate extensions to preserve the key funcionality of AffineTransform into Matrix3x2.

If not, I'll try to make AffineTransform as friendly as possible to those used to Matrix3x2.

My preferences would be (in order from most desired to least desired)

• Replace AffineTransform by Matrix3x4
• Make AffineTransform a readonly struct which would have Matrix3x4 under the hood.
• Keep AffineTransform as a class, but make it inmutable to prevent undesirable "by ref" antipatterns.
• Mutable AffineTransform class (as it is right now)

[jonlipsky]

@vpenades @sparkie108 I have been thinking along the same lines. The AffineTransform class served it's purpose 11 years ago when System.Numerics didn't exist; however, now that we have a cross platform library that serves that need, I think we can drop it.

My main concern would be keeping the helper methods available in AffineTransform so that users don't have to reimplement those themselves. @vpenades I like your proposed solution of moving to Matrix3x2 with extension methods to augment it.

Thoughts?

[vpenades]

My main concern would be keeping the helper methods available in AffineTransform so that users don't have to reimplement those themselves. @vpenades I like your proposed solution of moving to Matrix3x2 with extension methods to augment it.

The thing is... the issue is not with AffineTransform itself, but with the rest of the APIs , that is the fact that the rest of the APIs enforce you to use AffineTransform.

My idea is to replace AffineTransform in the APIs, so whoever wants to use Matrix3x2 it can do so.

But I also want to keep AffineTransform and much of its functionality around, so it can be used as a Matrix3x2 factory of sorts. Than, and some additional extensions (if needed) will do the trick.

Also, I would like to add Transform(matrix3x2) methods to PointF ... it needs to be done this way because matrix3x2 is closed.

So if you all agree, I'll be working on it

[jonlipsky]

@vpenades I agree with your approach, which is what I was trying to express initially; however, perhaps I wasn't clear enough. I look forward to reviewing your changes. This should be a significant improvement!

[vpenades]

Okey, I've did the replacement.

Now, all the APIs that were using AffineTransform now use Matrix3x2

Now AffineTransform could be completely removed, or repurposed in this way:

public readonly struct AffineTransform
{
    private readonly Matrix3x2 _Matrix;

    // move here some of the new extension methods & keep those AffineTransform methods that can be useful.
}

My intention would not be writing a full fledged replacement of Matrix3x2, just the stuff that's missing from matrix3x4, stuff like:

• GetScale / GetRotation / SetScale / SetRotation
• Transform PointF and SizeF (Although these types already have a PointF.TransformBy(Matrix3x2 matrix);

About the Concatenate* methods: I would either remove them, or reverse the multiplication order to match the one of Matrix3x2.

In any case, AffineTransform is isolated from the rest of the code now, so it's easy to refactor it at will,

[SparkieLabs]

I vote for extension methods on Matrix3x2 and remove AffineTransform entirely.

If you want to calculate the "average" scale, then the maths is here:
Decomposing an Affine transformation.

The correct extension for Matrix3x2 would be:

public static float AverageScale(this Matrix3x2 matrix) => MathF.Sqrt(matrix.M11 * matrix.M11 + matrix.M21 * matrix.M21);

[vpenades]

I vote for extension methods on Matrix3x2 and remove AffineTransform entirely.

I'm hesitant about using extensions all the way because two reasons:

• There's some methods that require more than one parameter, for example, Matrix3x2 lacks a full Create(scale, rotation, translation); and there's no nice way of doing it with an extension (actually there is, which is to do an extension on a value tuple, but it's not standard)
• System.Numerics.Vectors is still growing (slowly) so what if, in the future, it introduces a new method that collisions with an existing extension, and the funcionality is not quite the same? A good candidate for this is Deconstruct; Matrix4x4 has a Decompose method, but Matrix3x2, for some reason it's missing it. There's good chances they'll introduce it in the future to align with Matrix4x4 .... that's why the Maui extension is called Deconstruct and not Decompose... but what about the others...

So my gut tells me that the safe approach is to move the extension methods to AffineTransform as static methods (but not extensions), so Matrix3x2.GetAverageScale(); would become AffineTransform.GetAverageScale(Matrix3x2); ... in the hopes that in the future, Numerics.Vectors will eventually introduce these methods in one form or another.

If you want to calculate the "average" scale, then the maths is here: Decomposing an Affine transformation.

The correct extension for Matrix3x2 would be:

public static float AverageScale(this Matrix3x2 matrix) => MathF.Sqrt(matrix.M11 * matrix.M11 + matrix.M21 * matrix.M21);

Yeah, that one makes more sense, I'll correct it.

I've ran some unit tests, and I don't think MathF.Sqrt(matrix.M11 * matrix.M11 + matrix.M21 * matrix.M21) does what's expected from it.

The idea of average scale is that if you have a scale of 1 over the x axis and a scale of 2 over the y axis, the "average" scale would be 1.5f ... and that should also stand even after rotating the matrix.

you could also define it in terms of how much the area of an object will grow or shrink based on the transform, irrespective of any rotation applied, and also taking into account non orthogonal scaling.

In other words, an "average scale of a matrix" must meet this test:

var scale1 = Matrix3x2.CreateScale( 2, 3 ).GetAverageScale();
var scale2 = (Matrix3x2.CreateScale( 2, 3 ) * Matrix3x2.CreateRotation(1) ).GetAverageScale();
Assert.Equal( scale1, scale2 );

But I think a better way would be to calculate the area growth based on the scale, and from there the length.

And as it happens, the determinant of a 2D Matrix is also the (signed) area of a matrix. In other words: the absolute
determinat of a matrix also represents how much the area of a 1 unit area square surface will grow.

so this:

public static float GetAverageScale(this in Matrix3x2 matrix)
{
	var s = matrix.GetScale();
	return (Math.Abs(s.X) + Math.Abs(s.Y)) / 2;
}

would become this:

public static float GetLengthScale(this in Matrix3x2 matrix)
{
	var area = Math.Abs(matrix.GetDeterminant());
	return (float)Math.Sqrt(area);
}

This solution works for all kinds of matrices, even skewed ones.

[vpenades]

I've also noticed this on CanvasState, it has both a Transform and a Scale properties, but it's clear that Scale value is linked to Transform value, so I would change it from:

public class CanvasState
{
	public float Scale { get; set; } = 1;
	public Matrix3x2 Transform { get; set; }
}

to:

public class CanvasState
{
	private float transformScale = 1;
	private Matrix3x2 transform = Matrix3x2.Identity;

	public float TransformScale => this.scale
	public Matrix3x2 Transform
        {
            get => this.transform;
            set
                {
                this.transform = value;
                this.scale = value.GetLengthScale();
                }
        }
}

This ensures that transform and scale are always linked.

Also, if CanvasState is disposable, it should implement proper dispose pattern.

[SparkieLabs]

Agreed sqrt of determinant is a more general solution.

Apologies I missed the Matrix3x2Extensions class (there's a lot of files changed!). I think that's all that is required and AffineTransform can be dropped, it's just another way of doing the same thing.

[vpenades]

I almost agree that AffineTransform can be completely removed, With the introduction of Matrix3x2, it's no longer being used anywere.

Also, with the lastest commit, the only important methods (the scaling, construction and deconstruction) can be moved into CanvasState directly, so we could also avoid the risk of future method collision, and we could have the transform operations where it matters; it's important to keep the API surface to a minimum

[vpenades]

@jonlipsky @sparkie108 Trying to move this forward:

The only thing's left is to decide what to do with AffineTransform, now that it's no longer being used by the rest of the APIs.

Choices are:

• Remove it completely (my vote)
  • Pros: no redundancies with Matrix3x2, smaller API surface, focus on Maui namespace, not in System,Numerics namespace.
  • Cons: There's a few methods that need to find a "home" and are now defined as extensions.
    • I was considering CanvasState as the home for these methods, or making the extensions internal.
• Make AffineTransform static and home of the Matrix3x2 extensions (explicit)
  • Pros: good place to add Maui specific transformation operations, no extension conflicts.
  • Cons: I feel like the class could be used for more stuff.
• Repurpose it to be an utility struct defined as Scale/Rotation/Translation (*), and add appropiate methods there.
  • Pros, it can be useful in some cases, for example decomposing a matrix3x2 to a meaningful structure.
  • Cons: it's really needed? it seems like a random numerics extension for no reason.
• Make AffineTransform a pefect clone of Matrix3x2 but with Double precission.
  • Pros: if the API is exactly the same as Matrix3x2, developers won't need to learn a new API, also, double precission.
  • Cons: might collision with a future double precission Matrix3x2 at system,numerics

My feeling is that Matrix3x2 is a very complete API and there's plenty of libraries that add addional stuff to it; I don't think MAUI is the right library for adding stuff to the System.Numerics namespace.

(*) The idea of defining AffineTransform as S.R.T would make it look like this:

struct AffineTransform
{
    public Vector2 Scale;
    public float Rotation;
    public Vector2 Translation;
}

But again, I feel like I'm extending System.Numerics in the wrong place.

[jonlipsky]

@vpenades I would vote for removing it completely and we'll see if we get requests to add it back. I agree with you that in many ways we're extending System.Numerics in the wrong place.

For my own personal application, I do need some of those methods; however, I'll just add them in as extension methods within my own codebase that extend Matrix3x2.

[vpenades]

@jonlipsky Okey, I've removed AffineTransform, I've kept the extensions but I've made the extensions internal to the library.

I've had to add a static GetLengthScale to CanvasState so it's available to a derived classes of CanvasState.

I think this PR is ready to review, as it's completed its main purpose to replace AffineTransform with Matrix3x2.

There's some design issues I've noticed, that I would leave for another PR: it's the relationship between ICanvas, CanvasState, and the Native transform methods: Right now, modifying the native transforms doesn't reflect on the CanvasState base class, which can be an issue; I think it's possible to reverse, and letting CanvasState base class control the transform of the derived classes via the TransformChanged I've introduced, which can greatly simplify development of CanvasState derived classes.

[SparkieLabs]

I think this has ended up with a very good solution, @vpenades thanks for the great work!