RSDK-14475: pin orientation vector longitude at both poles, not just the north - #188
Open
Vignesh P (vpandiarajan20) wants to merge 5 commits into
Open
RSDK-14475: pin orientation vector longitude at both poles, not just the north#188Vignesh P (vpandiarajan20) wants to merge 5 commits into
Vignesh P (vpandiarajan20) wants to merge 5 commits into
Conversation
to_quaternion guarded the pole case on 1.0 - o_z rather than 1.0 - o_z.abs(), so the guard could only ever fire near o_z = +1. At the south pole 1.0 - (-1.0) = 2.0, which clears ANGLE_ACCEPTANCE and takes the ordinary non-pole branch instead of pinning the longitude. rdk pins at both poles (spatialmath/orientationVector.go:134), and the reverse conversion in this same file already uses .abs() at line 186, so the two directions were inconsistent with each other near o_z = -1.
Vignesh P (vpandiarajan20)
requested review from
Naveed Jooma (njooma) and
Ethan (stuqdog)
August 27, 2026 20:39
|
|
Vignesh P (vpandiarajan20)
marked this pull request as draft
August 27, 2026 20:43
Comments only; no code or test logic changed. Drops comments that restated the following line and moves the rationale to the PR description.
Cross-repo line numbers drift. Name the Go function instead.
Comments should describe the code as it exists; the rationale for the fix lives in the PR description.
Vignesh P (vpandiarajan20)
marked this pull request as ready for review
August 31, 2026 16:55
Vignesh P (vpandiarajan20)
changed the title
Vignesh P (vpandiarajan20)
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
utils.rs:135guards on the signed value instead of the magnitude:At
o_z = -1that is1.0 - (-1.0) = 2.0, which clearsANGLE_ACCEPTANCE(1e-4) and takes the non-pole branch. So the guard only ever fires nearo_z = +1, and vectors pointing nearly straight down keep a longitude that should have been discarded.rdk — which is what encodes these vectors — guards on the magnitude and pins at both poles:
orientationVector.go:134, withdefaultAngleEpsilon = 1e-4. Its doc comment states the rule in prose:1 - abs(OZ) > OrientationVectorPoleRadius.Most likely an oversight rather than a convention, because the reverse conversion in this same file already uses the magnitude:
utils.rs:183. The two directions currently disagree about where the south pole band is.Effect
Both build
Rz(lon)·Ry(lat)·Rz(theta). rdk useslon = 0in the band, this crate usedlon = atan2(o_y, o_x), so the rotations differ by exactly the vector's azimuth, up to 180°. Measured: 45.0000° off at azimuth 45°, 90.0000° off at azimuth 90°.Fix
One guard expression,
val→val.abs().ANGLE_ACCEPTANCEunchanged, surrounding math unchanged, reverse conversion untouched.Tests
Three added to the existing inline module. Expected quaternions are hardcoded literals computed independently of the code under test.
o_z = -0.99999at azimuth 45° and 90°; both must give the one pinned rotation. Fails onmain(0.15224 vs 0.0).o_z = +0.99999. Already correct; guards against fixing one pole and breaking the other../etc/test.sh: 14 passed onmain, 17 passed on this branch. No pre-existing test asserted the old behaviour.