[Bug] AffineTransform ScaleX and ScaleY return invalid values.

[vpenades]

I think the root of the problem is that AffineTransform's public properties for matrix members seem to be defined after SKMatrix, which uses some funky names for the matrix members, to say the least.

So:

• ScaleX is M00
• SkewX is M10
• SkewY is M01
• ScaleY is M11
• TransX is M02
• TransY is M12

The problem with this naming convention is that it is misleading, to say the least. And misleading names use to be the root of many bugs.

To someone that is used to work with transforms, ScaleX is usually interpreted as the Scale in the X axis of the matrix, not the scale along the horizon.

This is specially horrible on smartphones with display rotation: at 0 degrees, ScaleX and ScaleY will return (1,1) , and at 90 degrees will return (0,0)

So the problem is renaming M00 and M11 as ScaleX and ScaleY is that to the end developer, you don't know what these properties really return, it can be one of these:

• M00 and M11
• Scale along the horizon and the vertical
• Scale along the X and Y axes.

In fact, the misinterpretation of what ScaleX means might have already propagated here which is assuming the Scale will remain the same regardless of the rotation angle.

Furthermore: Given that AffineTransform has methods like GetScaleInstance, one can believe that you can use ScaleX and ScaleY as arguments of GetScaleInstance in a general way and this is not the case for rotated transforms. Notice that GetScaleInstance expects values to be Scale along the X and Y axes.

So something like this will give incorrect results:

var at1 = AffineTransform.Multiply( AffineTransform.GetScaleInstance(1,2) , AffineTransform.GetRotateInstance(1) );

var sx = at1.ScaleX; // a developer would expect this to be 1, but it is not.
var sy = at1.ScaleY; // a developer would expect this to be 2, but it is not.

var at2 = AffineTransform.Multiply( AffineTransform.GetScaleInstance(sx, sy) , AffineTransform.GetRotateInstance(1) );

// at this point, at1 and at2 should be the same, but they're not.

So I strongly request: at the very least, the public properties of AffineTransform must be renamed to follow the field names (M00, M01 and so on)

And after that, to have, correct, mathematical definitions of ScaleX and ScaleY:

public float ScaleX => _m10 == 0 ? _m00 : new Vector2(_m00,_m10).Length();
public float ScaleY => _m01 == 0 ? _m11 : new Vector2(_m01,_m11).Length();

or just:

public float ScaleX => new Vector2(_m00,_m10).Length();
public float ScaleY => new Vector2(_m01,_m11).Length();

A generalization of deconstructing an AffineTransform as long as it's orthogonal:

public void Deconstruct(out Vector2 scale, out float rotation, out Vector2 translation)
        {         
            var sx = new Vector2(_m00,_m10).Length();
            var sy = new Vector2(_m01,_m11).Length();

            scale = new Vector2(sx, sy);

            rotation = (float)Math.Atan2(_m10, _m00);
            translation = new Vector2(_m02, _m12);
        }

My general impression is that AffineTransform is designed in a way that it doesn't really support rotations. All the math inside seem to work only as long as M01 and M10 are zero, that is: no rotation at all. The moment you add arbitrary concatenations with rotations, most of the methods and properties become misleading.

I predict this is going to be a source of headaches in the long run.

[jonlipsky]

@vpenades You are correct that this could be misleading, and I'm happy to accept a pull request to change the behavior.

Like most things you've commented on, things are implemented the way they are because of specific use cases that I had, and I wasn't designing for anything beyond what I needed.

As an FYI, this code pre-dates Skia, so it's not based on SKMatrix. If you're curious, it was based on Java2D.

[vpenades]

Okey, then I would suggest to rename the properties... looking at microsoft's matrices, we have

• System.Numerics.Matrix3x2
  • M11, M12, M21, M22, M31, M32
• System.Windows.Media.Matrix
  • M11, M12, M21, M22, OffsetX, OffsetY

I would suggest to follow Systen.Numerics.Matrix3x2, so I would rename both the internal fields and the public properties.

Then, I would add these extra properties:

• SizeF Scale { get; }
• PointF Translation {get;}

Translation follows the same naming convention as Matrix3x2.

Scale would return the values along the axes of the matrix, so it would return the right scales, regardless of the rotation,

let me know if you agree, or if you need to change something about the proposal, and I'll do the pull request.

[jonlipsky]

@vpenades I agree with your proposal. I really appreciate your collaboration on this!

[vpenades]

Okey, I'll be working on the PR... I think I will also need to add some unit tests

[vpenades]

resolved with #189