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

Matrix3x2.Invert doesn't actually have tolerance #62714

Open
lcsondes opened this issue Dec 13, 2021 · 5 comments
Open

Matrix3x2.Invert doesn't actually have tolerance #62714

lcsondes opened this issue Dec 13, 2021 · 5 comments

Comments

@lcsondes
Copy link

Due to the definition of float.Epsilon this if will only work for ±0.0f (possibly not even that on ARM with no denormals), but the way the code was written suggests that there's some tolerance involved.

See also https://docs.microsoft.com/en-us/dotnet/api/system.single.epsilon#remarks

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Dec 13, 2021
@ghost
Copy link

ghost commented Dec 13, 2021

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Due to the definition of float.Epsilon this if will only work for ±0.0f (possibly not even that on ARM with no denormals), but the way the code was written suggests that there's some tolerance involved.

See also https://docs.microsoft.com/en-us/dotnet/api/system.single.epsilon#remarks

Author: lcsondes
Assignees: -
Labels:

area-System.Numerics, untriaged

Milestone: -

@tannergooding tannergooding added this to the Future milestone Jul 15, 2022
@tannergooding tannergooding added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jul 15, 2022
@PranavSenthilnathan
Copy link
Member

The check looks like it's just to make sure the determinant isn't zero. Another instance of this pattern actually has a comment:

// Check determinate is not zero
if (float.Abs(det) < float.Epsilon)

@tannergooding Is there any downside to just changing this to if (MathF.Abs(det) == 0.0f)?

@tannergooding
Copy link
Member

Its functionally the same as the current code, but that is itself potentially a bug/issue and likely not the "right" fix.

What the code is "meant" to be doing is finding cases where the matrix is "not invertible" and while for strict math that is determinant != 0, it doesn't always work out the same with IEEE 754 floating-point due to the precision limitations.

A "correct" implementation would find the appropriate cutoff and treat values below that threshold as "not invertible", where that threshold can sometimes differ based on the matrix. However, doing that is far too expensive for the use-cases that this type is designed for. Given the cases the type is designed for, more ideally this API would have a signature closer to what GLM, DirectX Math, HLSL, and GLSL provide which includes the result being generally undefined if the value is not inversible, such that it isn't returning a bool at all.

However, since we need to work with what we have the more correct thing would be to define a general epsilon that works for most typical scenarios. A better check would be if (float.IsSubnormal(det)) as that includes everything that is very close to zero. But a more appropriate check is likely 0.00001f, which is the "epsilon" value used by many other matrix functions in the core libraries I listed above. 0.0000001f is likely the smallest value that would be "acceptable" as it produces a reciprocal of 10000000 (which has a log2 of 23.2 and pushes up against the precision limitations of float, which has 23+1 mantissa bits).

@PranavSenthilnathan
Copy link
Member

Matrix4x4 can use the same change: here

@PranavSenthilnathan PranavSenthilnathan removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants