[BUG] SKMatrix44 Matrix Property Incorrect In SkiaSharp 3.0

[mlancione]

Description

The SKMatrix44 Matrix property in SkiaSharp 3.0 is incorrectly building the SKMatrix.

The previous implementation in v2 was performing the conversion in the native Skia library which was using the SkMatrix::get9() method: https://api.skia.org/classSkMatrix.html#aa49a9684efff87a979f8e26607bd2932

The description for the get9() method defines the order of the parameters: Copies nine scalar values contained by SkMatrix into buffer, in member value ascending order: kMScaleX, kMSkewX, kMTransX, kMSkewY, kMScaleY, kMTransY, kMPersp0, kMPersp1, kMPersp2.

The SKMatrix constructor defines the same order:

public SKMatrix(float scaleX, float skewX, float transX, float skewY, float scaleY, float transY, float persp0, float persp1, float persp2);

However, the current Matrix property is passing in the parameters in the wrong order:

public SKMatrix Matrix =>
	new SKMatrix (
		m00, m10, m30,
		m01, m11, m31,
		m03, m13, m33);

Code

Therefore, the corrected Matrix property should be:

public SKMatrix Matrix =>
	new SKMatrix (
		m00, m01, m03,
		m10, m11, m13,
		m30, m31, m33);

Expected Behavior

No response

Actual Behavior

No response

Version of SkiaSharp

3.116.0 (Current)

Last Known Good Version of SkiaSharp

2.88.9 (Previous)

IDE / Editor

Visual Studio Code (macOS)

Platform / Operating System

iOS

Platform / Operating System Version

No response

Devices

No response

Relevant Screenshots

No response

Relevant Log Output

Code of Conduct

• [x] I agree to follow this project's Code of Conduct

[mattleibow]

I have a to matrix and then also a to matrix44

Is that one also wrong? I feel like I have a test that checks a cycle so both may be wrong.

[mlancione]

The Implicit operator in SKMatrix44 is also wrong:

Current Implementation:

		public static implicit operator SKMatrix44 (SKMatrix matrix) =>
			new SKMatrix44 (
				matrix.ScaleX, matrix.SkewY, 0, matrix.Persp0,
				matrix.SkewX, matrix.ScaleY, 0, matrix.Persp1,
				0, 0, 1, 0,
				matrix.TransX, matrix.TransY, 0, matrix.Persp2);

Correct Implementation:

		public static implicit operator SKMatrix44 (SKMatrix matrix) =>
			new SKMatrix44 (
				matrix.ScaleX, matrix.SkewX, 0, matrix.TransX,
				matrix.SkewY, matrix.ScaleY, 0, matrix.TransY,
				0, 0, 1, 0,
				matrix.Persp0, matrix.Persp1, 0, matrix.Persp2);

Where are the ToMatrix and ToMatrix44 methods? I can't find them.

[mlancione]

Any update on this issue?

[mlancione]

No update on this fix?

[molesmoke]

This doesn't sound/feel right... Canvas.SetMatrix uses the implicit operator if you provide it an SKMatrix, and it gives the output I expect. If it were switched to your suggested implementation then it wouldn't. Consider doing it explicitly (e.g. with the WinForms sample for simplicity):

SKMatrix m = SKMatrix.CreateTranslation(100, 0);
SKMatrix4x4 m4 = (SKMatrix44)m;
canvas.SetMatrix(m4);

Since nobody has complained that SKCanvas transforms are hideously broken, and the unit tests cover the round trip between the two types, it seems likely that the current implementation is correct.

[mlancione]

@molesmoke Are you referring to my original post? The SKMatrix44.Matrix property is definitely wrong as I indicated in the post.

I listed the correct order of the elements with a link to the skia documentation.

[molesmoke]

AFAICT, SkMatrix::get9 wasn't used in 2.88.9 for conversions (although the conversion was performed natively).

I don't mind helping out, but could you perhaps provide the additional details that bug reports requests? What you did, what happened, what you expected? That way we could eliminate API usage and/or expectations being at fault. I note that both Skia and SkiaSharp sometimes use row-major and sometimes use column-major, perhaps that's a point of confusion here?

I provided a reasonably straightforward method for you to be able to verify that your suggested changes would make transforms give incorrect output without needing to compile SkiaSharp. Did you try it out? If so, what were your results?