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 glm::is_normalized epsilon test #1349

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Fix glm::is_normalized epsilon test #1349

merged 1 commit into from
Mar 21, 2024

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Jan 13, 2024

The existing comparison bound of $\epsilon^2$ is improperly scaled for testing an epsilon of the squared vector magnitude. Let $\epsilon$ be our specified epsilon and $\delta$ be the permissible delta of the squared magnitude. Thus, for a nearly-normalized vector, we have

$$\begin{align} \sqrt{1 + \delta} &= 1 + \epsilon \\ \delta &= (1 + \epsilon)^2 - 1 \\ \delta &= \epsilon^2 + 2\epsilon \text{ .}\end{align}$$

Since we only care about small epsilon, we can assume that $\epsilon^2$ is small and just use $\delta = 2\epsilon$. And in fact, this is the bound used by GLM (MIT license) ... except they're using length and not length2 for some reason.

If we stick an epsilon of 1.0e-6 into the current implementation, this gives us a computed delta of 1.0e-12: smaller than the f32 machine epsilon, and thus no different than direct float comparison without epsilon. This also gives an effetive epsilon of 5.0e-13; much less than the intended 1.0e-6 of intended permitted slack! By doing a bit more algebra, we can find the effective epsilon is $\sqrt{\texttt{epsilon}^2 + 1} - 1$. This patch makes the effective epsilon $\sqrt{2\times\texttt{epsilon} + 1} - 1$ which still isn't perfect, but it's effectively linear in the domain we care about, only really making a practical difference above an epsilon of 10%.

TL;DR: the existing is_normalized considers a vector normalized if the squared magnitude is within epsilon*epsilon of 1. This is wrong and it should be testing if it's within 2*epsilon. This PR fixes it.

For absence of doubt, a comparison epsilon of $\texttt{epsilon}^2$ is correct when comparing squared magnitude against zero, such as when testing if a displacement vector is nearly zero.

@CAD97
Copy link
Contributor Author

CAD97 commented Jan 13, 2024

I don't know enough about the specific function(s) failing tests to diagnose where the incorrect logic is, but I don't think it could possibly be caused by this change, because it's failing in an nalgebra test and this changes nalgebra-glm.

@CAD97 CAD97 changed the title Fix glm::is_normalized epsilon Fix glm::is_normalized epsilon test Jan 13, 2024
The existing comparison bound of $\epsilon^2$ is improperly scaled for
testing an epsilon of the squared vector magnitude. Let $\epsilon$ be
our specified epsilon and $\delta$ be the permissible delta of the
squared magnitude. Thus, for a nearly-normalized vector, we have

$$\begin{align}
\sqrt{1 + \delta} &=  1 + \epsilon        \\
          \delta  &= (1 + \epsilon)^2 - 1 \\
          \delta  &=  \epsilon^2 + 2\epsilon
\text{ .}\end{align}$$

Since we only care about small epsilon, we can assume
that $\epsilon^2$ is small and just use $\delta = 2\epsilon$. And in
fact, [this is the bound used by GLM][GLM#isNormalized] (MIT license)
... except they're using `length` and not `length2` for some reason.

[GLM#isNormalized]: https://github.com/g-truc/glm/blob/b06b775c1c80af51a1183c0e167f9de3b2351a79/glm/gtx/vector_query.inl#L102

If we stick an epsilon of `1.0e-6` into the current implementation,
this gives us a computed delta of `1.0e-12`: smaller than the `f32`
machine epsilon, and thus no different than direct float comparison
without epsilon. This also gives an effetive epsilon of `5.0e-13`;
*much* less than the intended `1.0e-6` of intended permitted slack!
By doing a bit more algebra, we can find the effective epsilon is
$\sqrt{\texttt{epsilon}^2 + 1} - 1$. This patch makes the effective
epsilon $\sqrt{2\times\texttt{epsilon} + 1} - 1$ which still isn't
*perfect*, but it's effectively linear in the domain we care about,
only really making a practical difference above an epsilon of 10%.

TL;DR: the existing `is_normalized` considers a vector normalized if
the squared magnitude is within `epsilon*epsilon` of `1`. This is wrong
and it should be testing if it's within `2*epsilon`. This PR fixes it.

For absence of doubt, a comparison epsilon of $\texttt{epsilon}^2$ is
correct when comparing squared magnitude against zero, such as when
testing if a displacement vector is nearly zero.
@CAD97
Copy link
Contributor Author

CAD97 commented Jan 13, 2024

Looks like the above failure was spurious, or at least nondeterministic. Reported as #1351

@tpdickso
Copy link
Collaborator

LGTM! A very sensible change. This does mean this implementation diverges from glm's implementation (in not using length-unsquared), but since it evidently already did diverge before this change, this isn't a regression. As long as nobody's complaining about the lack of parity I suppose we don't need to address it and can feel free to do the mathematically-correct thing.

@tpdickso tpdickso merged commit 9948bf7 into dimforge:dev Mar 21, 2024
11 checks passed
@CAD97 CAD97 deleted the patch-3 branch March 21, 2024 05:28
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.

2 participants