Give the three Three.js audit gates one owner for their XYZ transform math - #5826
Merged
Conversation
… transform math (#5681) `threejsModel.js`, `threejsModelPenetration.js` and `threejsModelPhysicalAudit.js` each carried their own row-major 3x3 copy of the same rotation/compose/apply math, all three claiming to reproduce `THREE.Euler` order 'XYZ' — the composition the preview canvas and the exported factory actually render with. They were algebraically identical but textually different with nothing pinning them together, so the three gates could silently start measuring three different scenes. They had already drifted on input hardening: `threejsModel.js` coerced a non-finite rotation component to 0 degrees, while both audit modules fed the raw value into `Math.cos`. A stored spec with a `NaN`/`undefined` rotation therefore gave those two gates `NaN` world bounds, and every touch and overlap test against `NaN` is false — a block sitting flat on the ground was reported as `floating-part`, and a fully buried part as no overlap at all. The new `server/lib/threejsTransform.js` owns the primitives and takes the coercing behaviour as canonical (a non-finite rotation component reads as 0 degrees, a non-finite scale component as 1), so the audits measure the same scene the renderer draws. `composeTransform` takes the local TRS object-shaped, which is what stored specs already carry, so a whole part passes straight through.
`IDENTITY_LINEAR` and `IDENTITY_TRANSFORM` used to be module-private literals in each of the three gates; exporting them makes one object the frame every walk in the process starts from, so a stray write would move it for every later caller. Freeze both, and pin it with a test.
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.
Summary
THREE.EulerorderXYZtransform math thatserver/lib/threejsModel.js,server/lib/threejsModelPenetration.jsandserver/lib/threejsModelPhysicalAudit.jseach carried a private copy of into one owner,server/lib/threejsTransform.js. The three copies were algebraically identical but textually different, with nothing pinning them together — so the three gates could silently start measuring three different scenes from the one the preview canvas and the exported factory render.threejsModel.jscoerced a non-finite rotation component to0degrees, while both audit modules fed the raw value intoMath.cos. A stored spec with aNaN/undefinedrotation gave those two gatesNaNworld bounds, and every touch and overlap test againstNaNis false — a block sitting flat on the ground came back asfloating-part, and a fully buried part as no overlap at all. The shared module takes the coercing behaviour as canonical (non-finite rotation component →0degrees, non-finite scale component →1), which is what the renderer already does with a malformed spec.composeTransformnow takes the local TRS object-shaped ({ position, rotationDegrees, scale }) — the shape stored specs already carry, so a whole part passes straight through and the penetration gate's call site is unchanged. The physical audit's two positional call sites were updated.server/lib/index.jsbarrel andserver/lib/README.mdper the Module Organization rule.invertTransformstays in the penetration gate (one consumer) andpartLinearstays inthreejsModel.js(a two-line composition specific to that file).IDENTITY_LINEAR/IDENTITY_TRANSFORMare frozen: they were module-private literals before, and exporting them makes one object the frame every walk in the process starts from.Closes #5681
Test plan
server/lib/threejsTransform.test.jspins the convention against hand-computed values, not a re-implementation: the three single-axisXYZmatrices, right-handed basis-vector rotation on each axis,rotationMatrix([30,40,50])equallingRx·Ry·Rzcomposed separately (catches an order swap), and a rotated+scaled child nested under a rotated parent applied to a point ([3, 2, 0], hand-computed).threejsModelPenetration.test.jsmutates a schema-parsed spec to a non-finite rotation/scale and asserts a finiteburied-partfraction still comes back;threejsModelPhysicalAudit.test.jsasserts a malformed-transform block resting on the ground audits identically to the well-formed one, with no spuriousfloating-part.threejsTransform.js, all five hardening assertions go red (2 in the audit suites, 3 in the new unit suite), and the physical-audit case specifically flips to the falsefloating-partreport.cd server && npm test— 1833 files / 37345 tests pass. Six unrelated suites (routes/imageGen.*,routes/settings.secretsStrip,services/imageTo3d/trellis2NormalBake,services/sprites/atlas,services/voice/fineTuning) flaked under full-run load; all six pass in isolation both on the unmodified baseline and with this branch applied, so they are pre-existing load flakes, not this change.