Skip to content

Add Gram-Schmidt orthonormalize() to Matrix3 and Matrix4#137

Merged
Renanse merged 1 commit into
masterfrom
matrix-orthonormalize
Jun 26, 2026
Merged

Add Gram-Schmidt orthonormalize() to Matrix3 and Matrix4#137
Renanse merged 1 commit into
masterfrom
matrix-orthonormalize

Conversation

@Renanse

@Renanse Renanse commented Jun 26, 2026

Copy link
Copy Markdown
Owner

What

Adds orthonormalize(store) / orthonormalizeLocal() to both Matrix3 and Matrix4, using the Gram-Schmidt process.

  • Matrix3 — orthonormalizes its rows, matching the row-based isOrthonormal() check, so a cleaned matrix is guaranteed to pass it.
  • Matrix4 — cleans only the upper-left 3×3 rotational block, copying the translation column (m03/m13/m23) and bottom row (m30m33) through unchanged. This is the rendering-useful operation and matches how the Collada path decomposes a homogeneous matrix into rotation + translation.

In both, the first row's direction is preserved exactly; the remaining rows are made orthogonal to it (and each other) and unit length. A rank-deficient block throws ArithmeticException, mirroring invert()'s contract. Both variants are store-safe when the source is passed as the store.

Why

Rotation matrices from external tools — notably Autodesk FBX→Collada / OpenCollada exporters — frequently carry enough floating-point rounding error to fail isOrthonormal(), which trips the long-standing "supplied transform with non-rotational matrices. May have unexpected results." warning flood on skeletal import and degrades the rotation extracted downstream. This adds the math primitive needed to clean those matrices up. Closes the math half of #32.

Note on the Matrix4 contract

Matrix4.isOrthonormal() tests the full 4×4 as a 4D orthonormal matrix, which is only ever true for a translation-free rotation. So Matrix4.orthonormalize() does not make isOrthonormal() return true on a matrix carrying translation — nor should it. Its contract is the 3×3 block (spelled out in the javadoc; the test asserts result.toMatrix3(null).isOrthonormal()).

Scope

Pure math addition — no existing call sites changed. Wiring this into the Collada importer (behind an opt-in flag) is deliberately left for a follow-up so this stays low-risk and reviewable on its own.

Tests

TestMatrix3 / TestMatrix4 each gain testOrthonormalize + testBadOrthonormalize: a skewed near-rotation matrix becomes orthonormal; translation column / bottom row verified preserved (Matrix4); a pure rotation round-trips unchanged; store and in-place variants covered; rank-deficient input throws. Full ardor3d-math suite green (241 tests, 0 failures).

🤖 Generated with Claude Code

Rotation matrices coming from external tools (notably Collada/FBX
exporters) frequently carry enough rounding error to fail isOrthonormal()
and degrade downstream rotation extraction, producing the long-standing
"non-rotational matrices" warnings on skeletal import (issue #32).

Add orthonormalize(store)/orthonormalizeLocal() to both classes:

- Matrix3 applies the Gram-Schmidt process to its rows, matching the
  row-based isOrthonormal() check, so the result is guaranteed to pass it.
- Matrix4 cleans only the upper-left 3x3 rotational block and copies the
  translation column and bottom row through unchanged, which is the
  rendering-useful operation (and consistent with how the Collada path
  decomposes a homogeneous matrix into rotation + translation).

The first row's direction is preserved exactly; the remaining rows are
made orthogonal to it and unit length. A rank-deficient block throws
ArithmeticException, mirroring invert()'s contract. Both methods are
store-safe when passed the source as the store.

This is a pure math addition with no existing call sites changed; wiring
it into the Collada importer is left for a follow-up.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Renanse, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 33 minutes and 31 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 46d65688-af10-45d3-b4bd-1b2bc0260996

📥 Commits

Reviewing files that changed from the base of the PR and between 3edc60c and b43f6e1.

📒 Files selected for processing (4)
  • ardor3d-math/src/main/java/com/ardor3d/math/Matrix3.java
  • ardor3d-math/src/main/java/com/ardor3d/math/Matrix4.java
  • ardor3d-math/src/test/java/com/ardor3d/math/TestMatrix3.java
  • ardor3d-math/src/test/java/com/ardor3d/math/TestMatrix4.java
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matrix-orthonormalize

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Renanse Renanse merged commit c299b50 into master Jun 26, 2026
2 checks passed
@Renanse Renanse deleted the matrix-orthonormalize branch June 26, 2026 23:29
Renanse added a commit that referenced this pull request Jun 26, 2026
* Add opt-in transform orthonormalization to the Collada importer

Some exporters (notably Autodesk FBX-to-Collada and OpenCollada) bake
enough floating-point rounding error into <matrix> transforms that the
rotational 3x3 block is no longer orthonormal. On skeletal import this
trips Ardor3D's "supplied transform with non-rotational matrices" warning
flood and degrades the rotation extracted for animation channels
(issue #32).

Add ColladaImporter.setOrthonormalizeTransforms(boolean), off by default.
When enabled, the upper-left 3x3 of each baked node and animation channel
transform is cleaned with Matrix4.orthonormalizeLocal() (added in #137)
before it becomes a Transform, with the translation preserved. A
rank-deficient block is left as-is with a warning rather than aborting the
whole load.

The importer is now threaded into ColladaAnimUtils so it can read the
flag, mirroring ColladaNodeUtils/ColladaMaterialUtils. Enabling the flag
discards any scale or shear carried in those matrices, so it is opt-in and
documented as such.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Release pooled workingMat/finalMat temps in the transform bakers

bakeTransforms (ColladaAnimUtils) and getNodeTransforms (ColladaNodeUtils)
each fetched two Matrix4 instances via Matrix4.fetchTempInstance() but
never released them, leaking them from the per-thread math pool on every
call. Wrap each body in try/finally so both temps are returned to the pool
on all paths, including exceptions. finalMat is still fully consumed by
fromHomogeneousMatrix in the return expression before the finally runs, so
there is no use-after-release.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (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