[Type] Quat : Add more unit tests and two bugfixes#5980
Open
fredroy wants to merge 2 commits intosofa-framework:masterfrom
Open
[Type] Quat : Add more unit tests and two bugfixes#5980fredroy wants to merge 2 commits intosofa-framework:masterfrom
fredroy wants to merge 2 commits intosofa-framework:masterfrom
Conversation
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
082db74 to
7cd17e9
Compare
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
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.
While adding more extensive unit tests, 2 bug were detected by Claude:
The one on createFromRotationVector looks good to me but as for slerp, Quat maths are a bit obscure, so I asked an other one (🤖)
[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