Fix: axis permutations silently erased by transform simplification - #53
Merged
bogovicj merged 2 commits intoJul 3, 2026
Merged
Conversation
isExlusiveTranslation currently accepts matrices whose off-diagonal entries equal 1.0 (e.g. axis permutations), so simplifyAffineGet collapses them to a pure Translation and silently drops the permutation. Add tests pinning the expected behaviour: - isExlusiveTranslation must reject PERMUTATION2D/3D - simplifying a permutation must not yield a Translation* - simplifying a permutation must preserve the full matrix These fail against the current code and document the bug. Also ignore the IntelliJ /.idea/ directory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The old test coupled the off-diagonal check to val != 1.0, so an off-diagonal entry equal to 1.0 (e.g. an axis permutation) was not disqualified. Such matrices were misclassified as pure translations and simplifyAffineGet then rebuilt them from the last column only, silently dropping the permutation. Replace the conflated condition with the three explicit rules: the translation column accepts any value, diagonal entries must be 1, and off-diagonal entries must be 0. Permutations now fall through to the lossless generic affine copy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bogovicj
approved these changes
Jul 2, 2026
bogovicj
left a comment
Contributor
There was a problem hiding this comment.
Good catch. This looks good
Contributor
|
Thanks @NicoKiaru ! |
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.
Fixes #52
Summary
RealViewsSimplifyUtils.isExlusiveTranslationmisclassified axis-permutationmatrices as pure translations.
simplifyAffineGetthen rebuilt them from thetranslation column only, silently discarding the permutation: shapes stayed
intact and the translation was roughly right, but the orientation was lost.
Root cause
The classifier coupled the off-diagonal test to
val != 1.0:For an off-diagonal entry (
r != c,c < n) equal to1.0, neither clausefires, so it is accepted. The diagonal rule ("must be 1") was wrongly applied to
every in-matrix entry, instead of "off-diagonal must be 0". A concrete Y/Z swap:
passed as a pure translation. A tiny rotation (e.g. 0.5 deg) turns the exact
1.0entries into0.9999..., which tripsval != 1.0and accidentally masksthe bug -- explaining why a small "nudge" appeared to fix it.
Fix
Replace the conflated condition with the three explicit rules:
Permutations now fail classification and fall through to the lossless generic
affine copy. No information is dropped.
Scope
The sibling classifiers (
isExclusiveScale,isExclusiveScaleAndTranslation)were reviewed and are not affected: both already disqualify any non-zero
off-diagonal entry regardless of value.
boundingInterval/estimateBoundsusethe full matrix and handle off-diagonal terms correctly. This was the only
erasure path.
The public method name
isExlusiveTranslation(sic) is kept unchanged to avoidbreaking API compatibility.
Tests
Added to
RealViewsSimplificationsTest(committed first, red before the fix):isExlusiveTranslationmust rejectPERMUTATION2D/PERMUTATION3DTranslation*Full module test suite passes after the fix.