Skip to content

[Type] Quat : Add more unit tests and two bugfixes#5980

Open
fredroy wants to merge 2 commits intosofa-framework:masterfrom
fredroy:add_more_unittests_quat
Open

[Type] Quat : Add more unit tests and two bugfixes#5980
fredroy wants to merge 2 commits intosofa-framework:masterfrom
fredroy:add_more_unittests_quat

Conversation

@fredroy
Copy link
Contributor

@fredroy fredroy commented Feb 27, 2026

While adding more extensive unit tests, 2 bug were detected by Claude:

1. createFromRotationVector(Real, Real, Real) at line 540: Changed phi >= 1.0e-5 to phi <= 1.0e-5. The condition was inverted compared to the Vec3 overload, causing it to return identity for any real rotation and
  divide-by-zero/NaN for zero input.
2. slerp(const Quat&, Real) at line 390-394: Changed sin(t * angle) / cos(t * angle) to sin(t * angle / 2) / cos(t * angle / 2). Quaternions encode rotations using half-angles, but quatToAxis returns the full rotation angle. The
  reconstruction was missing the half-angle division, producing incorrect interpolation for all t != 0

The one on createFromRotationVector looks good to me but as for slerp, Quat maths are a bit obscure, so I asked an other one (🤖)

Why the other AI is right

In your quatToAxis function, you explicitly calculate the full rotation:
angle = 2 * half_theta;

When you go to rebuild the interpolated quaternion in slerp, you are trying to create a rotation of magnitude t×angle. According to the formula above, the w component (the scalar) must be the cosine of half that magnitude.

Without that division by 2, your interpolation would rotate twice as fast as it should. If you were trying to rotate 90° over 1 second, your code would actually complete the 90° turn in 0.5 seconds and keep going to 180°.

[ci-build][with-all-tests]


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@fredroy fredroy added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request pr: test PR adding test(s) in SOFA pr: AI-aided Label notifying the reviewers that part or all of the PR has been generated with the help of an AI labels Feb 27, 2026
fredroy added 2 commits March 3, 2026 07:06
Constructors (7 tests):
  - DefaultConstructor, ParameterConstructor, ArrayConstructor, CopyConstructorFromDifferentType, ConstructorFromAxisAngle, ConstructorFromTwoVectors, NoInitConstructor

  Previously untested methods (12 tests):
  - IsNormalized, ToHomogeneousMatrix, QuatVectMult, VectQuatMult, OperatorPlusEqualsQuat, OperatorMultEqualsQuat, OperatorEquals, OperatorNotEquals, SlerpMember, AngularDisplacement, AngularDisplacementIdentity,
  CreateFromRotationVectorScalars

  Euler order variants + fromEuler (7 tests):
  - FromEuler, CreateQuaterFromEulerXYZ/YXZ/ZXY/ZYX/YZX/XZY

  Stream I/O (3 tests):
  - StreamOutput, StreamInput, StreamRoundTrip

  Float variant + misc (8 tests):
  - WriteOpenGlMatrixFloat, SizeAndConstants, GetTemplateAccessor, GetTemplateAccessorConst, QuatNoInit, QuatfDefaultConstructor, QuatfNormalize, QuatfRotate, QuatfInverse, QuatfOperatorMultQuat

  Mathematical property / round-trip tests (13 tests):
  - InverseRotateUndoesRotate, QuatTimesInverseIsIdentity, IdentityRotateIsNoOp, NormalizePreservesDirection, ToMatrixFromMatrixRoundTrip, AxisToQuatQuatToAxisRoundTrip, CreateFromRotationVectorRoundTrip,
  MultiplicationIsAssociative, RotateConsistentWithToMatrix, SetFromUnitVectorsOppositeVectors, SetFromUnitVectorsSameVector, NormalizeZeroQuaternion, AxisToQuatZeroAxis, SlerpIdenticalQuaternions,
  ToHomogeneousMatrixConsistentWithToMatrix, BuildRotationMatrixConsistentWithWriteOpenGlMatrix

  Two issues found in the implementation during testing:
  - createFromRotationVector(Real, Real, Real) at Quat.inl:540 has an inverted condition (>= instead of <=), causing it to always return identity for non-tiny inputs and produce NaN for zero input.
  - slerp(const Quat&, Real) (2-arg member) uses sin(t*angle)/cos(t*angle) instead of the half-angle, making it incorrect for t != 0.
1. createFromRotationVector(Real, Real, Real) at line 540: Changed phi >= 1.0e-5 to phi <= 1.0e-5. The condition was inverted compared to the Vec3 overload, causing it to return identity for any real rotation and
  divide-by-zero/NaN for zero input.
  2. slerp(const Quat&, Real) at line 390-394: Changed sin(t * angle) / cos(t * angle) to sin(t * angle / 2) / cos(t * angle / 2). Quaternions encode rotations using half-angles, but quatToAxis returns the full rotation angle. The
  reconstruction was missing the half-angle division, producing incorrect interpolation for all t != 0
@fredroy fredroy force-pushed the add_more_unittests_quat branch from 082db74 to 7cd17e9 Compare March 2, 2026 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: AI-aided Label notifying the reviewers that part or all of the PR has been generated with the help of an AI pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request pr: test PR adding test(s) in SOFA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant