Fix GetArbitraryAxis: the 1/64 threshold is integer division and is always zero - #23
Open
redbluevn wants to merge 1 commit into
Open
Fix GetArbitraryAxis: the 1/64 threshold is integer division and is always zero#23redbluevn wants to merge 1 commit into
redbluevn wants to merge 1 commit into
Conversation
…s always zero The arbitrary axis algorithm picks its reference axis from the world Y when the normal is close to the world Z, and from the world Z otherwise. The threshold was written as (1 / 64), which in C# is integer division between two ints and evaluates to 0, so the condition could never hold and the Y branch was dead code. Every normal then took the Z branch. That is correct for a genuinely tilted normal, but for one that means +Z while carrying the rounding dust a real file records - (-3.7e-13, 8.8e-14, 1) is taken verbatim from an architectural drawing - the cross product with Z is on the order of 1e-13, and normalising it returns a direction decided entirely by that dust. The resulting frame has nothing to do with the entity. The visible effect is that block references land in the wrong place: measured across eighteen drawings, 169 inserts whose normal was +Z to within a rounding error were being positioned by a garbage rotation, which is enough to leave a drawing's computed extents tens of times too large. Neither GetArbitraryAxis nor Matrix4 had any test coverage. The new tests pin the identity and negated-Z cases, the near-Z case above, and that either side of the threshold still yields a frame whose Z is the normal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 30, 2026
redbluevn
pushed a commit
to redbluevn/ACadSharpNew
that referenced
this pull request
Aug 30, 2026
Carries DomCR/CSUtilities#23 into this working branch, so a package built from here places block references by a real rotation rather than by whatever the rounding dust in a near-+Z normal happened to produce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 30, 2026
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.
The problem
Matrix4.GetArbitraryAxis(XYZ zaxis)implements the arbitrary axis algorithm, which picks its reference axis from the world Y when the normal is close to the world Z, and from the world Z otherwise:1 / 64is integer division between twoints, so the threshold is 0. The condition can never hold, and theWybranch is dead code.Why that is not harmless
Every normal then takes the
Wzbranch. That is correct for a genuinely tilted normal. It is badly wrong for a normal that means+Zbut carries the rounding dust real files record — this one is taken verbatim from an architectural drawing:Wz × Nis then on the order of1e-13, and normalising it returns a direction decided entirely by that dust. The frame that comes back has nothing to do with the entity. Thezaxis.Equals(XYZ.AxisZ)fast path does not catch it either, since the vector is not exactly+Z.Downstream in ACadSharp this positions block references: measured across eighteen drawings, 169 inserts whose normal was
+Zto within a rounding error were being placed by a garbage rotation — enough to leave the drawings' computed extents tens of times too large. A block reference withNormal = (-3.7e-13, 8.8e-14, 1),Rotation = 1.2,Scale = (1,1,1), whose block spans0..113 × 0..426, was reported at(-9741106, -1745685)instead of near its insertion point(3952258, -9072802).The fix
1.0 / 64.0.Tests
There were no tests for
Matrix4at all, and none forGetArbitraryAxis, which is why this survived. The newMatrix4Testspins:+Z, and the X/Z mirror for-Z;+Znormal must leave a point essentially where it was (this test fails against the current implementation and passes with the fix);dotnet testonCSMath.Tests: 248 passed / 0 failed.