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

Matrix2 get scale incorrect #587

Closed
leftnoodle opened this issue Jan 24, 2019 · 6 comments
Closed

Matrix2 get scale incorrect #587

leftnoodle opened this issue Jan 24, 2019 · 6 comments
Assignees
Labels
Milestone

Comments

@leftnoodle
Copy link

leftnoodle commented Jan 24, 2019

It returns incorrect values and doesn't take into account negative scale values. This can be seen when creating a matrix using CreateFrom with rotation = 0.296706 radians, scaleX =7 and scaleY = 4. The result from get scale is 6.796, 4.338.

I believe the correct calculation should be:

var scaleX = (float)Math.Sign(M11) * (float)Math.Sqrt(M11 * M11 + M12 * M12);
var scaleY = (float)Math.Sign(M22) * (float)Math.Sqrt(M21 * M21 + M22 * M22);

The get rotation also returns incorrect rotation values

@craftworkgames
Copy link
Collaborator

Thanks for reporting the issue.

To be honest, I didn't write most of that code and I don't use it very often. So it's difficult to tell just from looking at your description what the problem is or if that's the correct solution.

To make this change I suggest we need some unit tests and / or find a good reference for the equations.

@lithiumtoast
Copy link
Contributor

I wrote the code.

@leftnoodle You are correct it does not take into account negative scale values. My proposed solution.

public void Decompose(out Vector2 scale, out float rotation, out Vector2 translation)
{
     translation.X = this.M31;
     translation.Y = this.M32;

     float xs = (Math.Sign(M11 * M12) < 0) ? -1 : 1;
     float ys = (Math.Sign(M21 * M22) < 0) ? -1 : 1;

     scale.X = xs * (float)Math.Sqrt(M11 * M11 + M12 * M12);
     scale.Y = ys * (float)Math.Sqrt(M21 * M21 + M22 * M22);

     rotation = (float)Math.Atan2(M21, M11);
}

For two dimensions, the code can be chopped to get methods (properties) as the status-quo. However, for three dimensions it's been discussed why this is not ideal. To keep consistency with MonoGame I propose marking the get properties, such as Scale, deprecated and rely on the new Decompose method. The similar pattern can be found with Matrix.

@leftnoodle
Copy link
Author

I think you will need to do the same as Matrix.cs and divide M21 and M11 by scale.Y and scale.X respectively to obtain the correct rotation?

I'm also not sure that will calculate the correct scale, I could be mistaken but if we take a standard rotation matrix with PI / 4.0 radians then we get:

M11 = 0.707, M12 = 0.707 xs = 1
M21 = -0.707, M22 = 0.707 ys = -1

Which would result in a negative scale.Y? I might be completely wrong though as I'm doing this in my head.

@lithiumtoast
Copy link
Contributor

@lithiumtoast lithiumtoast self-assigned this May 20, 2020
@lithiumtoast lithiumtoast added this to the v4 milestone May 20, 2020
@lithiumtoast
Copy link
Contributor

In v4 we will consider using Matrix3x2 instead.

@AristurtleDev
Copy link
Collaborator

This is resolved in #870 with the introduction of Matrix3x2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants