Skip to content

fix quatdist clamp: swapped min/max caused function to always return 0#350

Merged
fredinfinite23 merged 1 commit intocollabora:masterfrom
rocketmark:fix/quatdist-clamp
Mar 12, 2026
Merged

fix quatdist clamp: swapped min/max caused function to always return 0#350
fredinfinite23 merged 1 commit intocollabora:masterfrom
rocketmark:fix/quatdist-clamp

Conversation

@rocketmark
Copy link
Copy Markdown
Contributor

Summary

quatdist computes the angular distance between two quaternions as 2 * acos(|dot(q1, q2)|). The acos domain requires the dot product clamped to [-1, 1], but the clamp arguments are swapped:

// BUG:
rtn = linmath_max(1., linmath_min(-1, rtn));

linmath_min(-1, rtn) returns a value ≤ −1 for any input. linmath_max(1, ≤-1) then clamps to exactly 1.0. So acos(|1.0|) = 0 for every input — quatdist always returns 0 regardless of how far apart the quaternions are.

Demonstration

For a 90° rotation around Z (q = [0.707107, 0, 0, 0.707107]) vs identity ([1, 0, 0, 0]):

dot(identity, q) = 0.707107

BEFORE fix:
  linmath_min(-1, 0.707107) = -1.000000   ← min of -1 and 0.707 is always -1
  linmath_max( 1, -1.000000) =  1.000000   ← max of 1 and anything ≤ -1 is always 1
  acos(|1.0|) = 0.000000
  quatdist = 2 * 0.000000 = 0.000000      ← WRONG, expected π/2 = 1.570796

AFTER fix:
  linmath_max(-1., 0.707107) =  0.707107   ← clamp floor
  linmath_min( 1., 0.707107) =  0.707107   ← clamp ceiling
  acos(|0.707107|) = 0.785398
  quatdist = 2 * 0.785398 = 1.570796      ← correct: π/2 radians = 90°

For any input where dot(q1, q2) is positive, linmath_min(-1, rtn) evaluates to −1, guaranteeing the wrong result. The only input that accidentally produces the correct answer is dot = 0 (quaternions exactly 180° apart), where both implementations return 2 * acos(0) = π.

Impact

No existing upstream callers — quatdist is defined in redist/linmath.c but never called from any upstream .c file. Any downstream code using this function to measure angular separation between poses would silently receive 0 for every query.

Change

One file, one line in redist/linmath.c:

// before:
rtn = linmath_max(1., linmath_min(-1, rtn));

// after:
rtn = linmath_min(1., linmath_max(-1., rtn));

Found via

Property-based testing: QuatDistKnownAngle in quat_props.c checks that quatdist(identity, q_θ) ≈ θ for 10,000 random rotation axes and angles in (0, π). Every trial failed immediately, returning 0 instead of the expected angle.

linmath_max(1., linmath_min(-1, rtn)) always clamps the dot product to
1.0: linmath_min(-1, x) is always ≤ -1 for any x, then
linmath_max(1, ≤-1) = 1, so acos(|1.0|) = 0 for every input.

Fix: linmath_min(1., linmath_max(-1., rtn)) correctly clamps to [-1, 1].

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@rocketmark
Copy link
Copy Markdown
Contributor Author

I have a series of property tests I can submit to the project if there's interest in them.

@fredinfinite23
Copy link
Copy Markdown
Collaborator

Good catch and you're right.
canonical clamp is :
f(x) = min(1, max(-1, x))

@fredinfinite23 fredinfinite23 merged commit 1fb4276 into collabora:master Mar 12, 2026
2 of 4 checks passed
@fredinfinite23
Copy link
Copy Markdown
Collaborator

I have a series of property tests I can submit to the project if there's interest in them.

and forgot about this, yes please ... more test's always good :)

rocketmark pushed a commit to rocketmark/libsurvive that referenced this pull request Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants