Skip to content

Fix GetArbitraryAxis: the 1/64 threshold is integer division and is always zero - #23

Open
redbluevn wants to merge 1 commit into
DomCR:masterfrom
redbluevn:moredwg/arbitrary-axis-threshold
Open

Fix GetArbitraryAxis: the 1/64 threshold is integer division and is always zero#23
redbluevn wants to merge 1 commit into
DomCR:masterfrom
redbluevn:moredwg/arbitrary-axis-threshold

Conversation

@redbluevn

Copy link
Copy Markdown

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:

if (Math.Abs(zaxis.X) < (1 / 64) && Math.Abs(zaxis.Y) < (1 / 64))

1 / 64 is integer division between two ints, so the threshold is 0. The condition can never hold, and the Wy branch is dead code.

Why that is not harmless

Every normal then takes the Wz branch. That is correct for a genuinely tilted normal. It is badly wrong for a normal that means +Z but carries the rounding dust real files record — this one is taken verbatim from an architectural drawing:

(-3.707652143685886E-13, 8.818399612355268E-14, 1)

Wz × N is then on the order of 1e-13, and normalising it returns a direction decided entirely by that dust. The frame that comes back has nothing to do with the entity. The zaxis.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 +Z to 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 with Normal = (-3.7e-13, 8.8e-14, 1), Rotation = 1.2, Scale = (1,1,1), whose block spans 0..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 Matrix4 at all, and none for GetArbitraryAxis, which is why this survived. The new Matrix4Tests pins:

  • identity for +Z, and the X/Z mirror for -Z;
  • the regression case — a near-+Z normal must leave a point essentially where it was (this test fails against the current implementation and passes with the fix);
  • that either side of the threshold still yields a frame whose Z is the normal.

dotnet test on CSMath.Tests: 248 passed / 0 failed.

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant