Skip to content

Conversation

@nicolaslg
Copy link
Collaborator

@nicolaslg nicolaslg commented Mar 18, 2025

#33 introduces a bug, where isNearlyEqual(a, b) returns false when (a == 0) xor (b == 0) (it is an exclusive or), even when the other argument is near zero.

We might have to properly investigate further for a more reliable check, but this fix should do in the meantime.

Careful, as it affects the Utils::Math::Point operator == it is widely used in all kinds of situations by quite a lot of the methods

@nicolaslg nicolaslg requested a review from lelandaisb March 18, 2025 23:59
@nicolaslg nicolaslg force-pushed the fix_isnearlyequal_floating_point_comparison branch from f1c13fa to aae600c Compare March 19, 2025 00:05
@nicolaslg nicolaslg added the bug Something isn't working label Mar 19, 2025
Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

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

I am not fully aware of Magix3D coding style, so some of my remarks may not apply.

I think the comparison can be improved using methods like https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/ or https://realtimecollisiondetection.net/blog/?p=89 .

GoogleTest has also some utilities to do this, see https://github.com/google/googletest/blob/3af834740f58ef56058e6f8a1be51f62bc3a6515/googletest/include/gtest/internal/gtest-internal.h#L336 .

Perhaps you may add a unit test directly calling this specific function with your motivating use case (a == 0.0 )^(b == 0.0)?

static size_t mgxDoubleFixedNotationCharMax;
#endif // SWIG

static inline bool isNearlyEqual(const double a, const double b)
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, constexpr, but it requires implementing it with only one return.

@nicolaslg nicolaslg force-pushed the fix_isnearlyequal_floating_point_comparison branch 2 times, most recently from e1599fa to 5d3db59 Compare March 27, 2025 20:29
@nicolaslg nicolaslg force-pushed the fix_isnearlyequal_floating_point_comparison branch from 5d3db59 to 1ac28b7 Compare March 27, 2025 20:30
@nicolaslg
Copy link
Collaborator Author

Thanks for the insightful feedback

@nicolaslg nicolaslg mentioned this pull request Mar 27, 2025
@nicolaslg nicolaslg changed the title Fix isnearlyequal floating point comparison issue-202 Fix isnearlyequal floating point comparison Mar 28, 2025
@lelandaisb lelandaisb merged commit 392f99e into main Mar 31, 2025
2 checks passed
@lelandaisb lelandaisb deleted the fix_isnearlyequal_floating_point_comparison branch March 31, 2025 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants