Skip to content

approxEqAbs comparison type changed to f64 #19

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

Merged
merged 1 commit into from
Jun 25, 2025

Conversation

Nefistofeles
Copy link
Contributor

const orthographics = math.orthographicLh(1366.0, 768.0, -1000.0, 1000.0) 
const inv = math.inverse(orthographics);

Ortographics matrix is:

0.0014641288, 0,            0,      0
0,            0.0026041667, 0,      0
0,            0,            0.0005, 0
0,            0,            0.5,    1

Determinant result is: 0.00000000190641773273548
f32 epsilon is math.floatEps(f32) = 0.0000001192092900
f64 epsilon is math.floatEps(f64) = 0.0000000000000002

Inverse matrix result give all column 0 because of this lines:

zmath/src/root.zig

Lines 2622 to 2629 in 634ffd6

if (math.approxEqAbs(f32, det[0], 0.0, math.floatEps(f32))) {
return .{
f32x4(0.0, 0.0, 0.0, 0.0),
f32x4(0.0, 0.0, 0.0, 0.0),
f32x4(0.0, 0.0, 0.0, 0.0),
f32x4(0.0, 0.0, 0.0, 0.0),
};
}

Increasing approxEqAbs type to f64 solve the problem.

@Srekel
Copy link
Member

Srekel commented Jun 24, 2025

We had a similar problem with this line on Tides recently and simply removed it. Might be worth checking if this works better. What is the reason for it in the first place @michal-z ?

Copy link
Member

@hazeycode hazeycode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Lots of error can be accumulated calculating determinants so making the epsilon smaller to mitigate this seems reasonable.

@hazeycode hazeycode requested a review from Copilot June 24, 2025 11:24
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the determinant near-zero check in the matrix inversion function by using double-precision tolerance to avoid falsely detecting non-singular matrices as singular.

  • Switched math.approxEqAbs from f32 to f64 when comparing the determinant against epsilon
  • Updated epsilon argument to math.floatEps(f64) for higher precision
Comments suppressed due to low confidence (2)

src/root.zig:2620

  • [nitpick] Add a comment above this check explaining why f64 precision is needed for the determinant comparison to help future maintainers understand the rationale.
    }

src/root.zig:2622

  • Add a unit test for inverseDet using a near-singular matrix (like the orthographic example) to verify that the new f64 comparison correctly avoids returning a zero matrix.
    if (math.approxEqAbs(f64, det[0], 0.0, math.floatEps(f64))) {

@hazeycode hazeycode merged commit 5128dc8 into zig-gamedev:main Jun 25, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants