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

Fix Matrix4x4.CreateReflection when D is not zero #110057

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hez2010
Copy link
Contributor

@hez2010 hez2010 commented Nov 21, 2024

This is a correctness regression from .NET 8 which was introduced in #103527.
Fixed it and added a regression test for it.

Fixes #110050

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Nov 21, 2024
@hez2010
Copy link
Contributor Author

hez2010 commented Nov 22, 2024

I think we should revert to the MathHelper.Equal as the sign of 0.0 in the result is not determined. I think as for this algorithm the sign of zero does not matter.

Plane plane = new Plane(0, 1, 0, 60);
Matrix4x4 actual = Matrix4x4.CreateReflection(plane);

Console.WriteLine(Equal(1.0f, actual.M11));
Console.WriteLine(Equal(0.0f, actual.M12));
Console.WriteLine(Equal(0.0f, actual.M13));
Console.WriteLine(Equal(0.0f, actual.M14));
Console.WriteLine(Equal(0.0f, actual.M21));
Console.WriteLine(Equal(-1.0f, actual.M22));
Console.WriteLine(Equal(0.0f, actual.M23));
Console.WriteLine(Equal(0.0f, actual.M24));
Console.WriteLine(Equal(0.0f, actual.M31));
Console.WriteLine(Equal(0.0f, actual.M32));
Console.WriteLine(Equal(1.0f, actual.M33));
Console.WriteLine(Equal(0.0f, actual.M34));
Console.WriteLine(Equal(-0.0f, actual.M41));
Console.WriteLine(Equal(-120.0f, actual.M42));
Console.WriteLine(Equal(-0.0f, actual.M43));
Console.WriteLine(Equal(1.0f, actual.M44));

(bool, string) Equal(float a, float b)
{
    if (a.ToString() == b.ToString()) return (true, "ok");
    return (false, $"expected: {a}, actual: {b}");
}

On .NET 9 with this PR:

(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(False, expected: -0, actual: 0)
(True, ok)
(False, expected: -0, actual: 0)
(True, ok)

On .NET 8:

(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)

On .NET 7:

(True, ok)
(False, expected: 0, actual: -0)
(False, expected: 0, actual: -0)
(True, ok)
(False, expected: 0, actual: -0)
(True, ok)
(False, expected: 0, actual: -0)
(True, ok)
(False, expected: 0, actual: -0)
(False, expected: 0, actual: -0)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)

On .NET 6:

(True, ok)
(False, expected: 0, actual: -0)
(False, expected: 0, actual: -0)
(True, ok)
(False, expected: 0, actual: -0)
(True, ok)
(False, expected: 0, actual: -0)
(True, ok)
(False, expected: 0, actual: -0)
(False, expected: 0, actual: -0)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)
(True, ok)

Also, the precision of the existing test is an issue as well:

    System.Numerics.Tests.Matrix4x4Tests.Matrix4x4CreateReflectionTest01 [FAIL]
      Assert.Equal() Failure: Values differ
      Expected: -1.57142854
      Actual:   -1.57142889

I want to minimize the change given that this PR need to be backported to .NET 9. I'm reverting the changes to tests.

@tannergooding
Copy link
Member

Determinism with -0 vs +0 is actually relevant and expected

The general issue is that the pre-existing CreateReflection01 test is “bad” and is essentially testing the output is the same as base math, which is faulty and not itself “correct”

I’d be fine with leaving the existing test using the helper to minimize PR churn, but the new test should be strictly doing the right thing here

@hez2010
Copy link
Contributor Author

hez2010 commented Nov 22, 2024

Test failures are unrelated.

@tannergooding PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Matrix4x4.CreateReflection gives incorrect results in .NET 9
2 participants