From ee5d514ae1164016c5b36622a8f3f61a85a6e3ee Mon Sep 17 00:00:00 2001 From: Marco Stagni Date: Tue, 23 Jun 2026 17:53:35 +0100 Subject: [PATCH] fix(entity): preserve world transform when reparenting out of/between parents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reparent() removed the body from its THREE.js parent before calling attach(). attach() preserves an object's world transform by reading `object.parent.matrixWorld`, so nulling the parent first made it skip that step and keep the stale local coordinates — a child at local (5,5,5) under a parent at (5,5,5) (world (10,10,10)) snapped to world (5,5,5) when moved to the root. attach() already re-parents via add() (which detaches from the old parent), so the manual remove() was both redundant and the cause of the bug. Dropping it lets attach() see the old parent and preserve the world transform. Root->parent drops were unaffected (local == world at the root), which is why only moving out of / between parents regressed. Add Entity.reparent regression tests that exercise real THREE matrix math (out-to-root, root-to-parent, parent-to-parent, and Entity bookkeeping). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/entities/Entity.js | 11 +- .../__tests__/Entity.reparent.test.js | 137 ++++++++++++++++++ 2 files changed, 143 insertions(+), 5 deletions(-) create mode 100644 src/entities/__tests__/Entity.reparent.test.js diff --git a/src/entities/Entity.js b/src/entities/Entity.js index 5720cdba..09569891 100644 --- a/src/entities/Entity.js +++ b/src/entities/Entity.js @@ -258,11 +258,12 @@ export default class Entity extends EventDispatcher { if (index !== -1) oldParent.children.splice(index, 1); } - // Detach from old THREE.js parent + // NOTE: do NOT remove the body from its current THREE.js parent here. + // attach() preserves the world transform by reading body.parent.matrixWorld, + // and internally re-parents via add() (which detaches from the old parent). + // Removing first nulls body.parent, so attach() skips the world-preservation + // step and the entity snaps to its stale local coordinates instead. const body = this.getBody(); - if (body.parent) { - body.parent.remove(body); - } if (newParent && newParent.hasBody()) { // Attach to new parent (preserves world position) @@ -270,7 +271,7 @@ export default class Entity extends EventDispatcher { this.setParent(newParent); newParent.getBody().attach(body); } else { - // Move to scene root + // Move to scene root (also preserves world position) this.setParent(false); Scene.getScene().attach(body); } diff --git a/src/entities/__tests__/Entity.reparent.test.js b/src/entities/__tests__/Entity.reparent.test.js new file mode 100644 index 00000000..d4fbf958 --- /dev/null +++ b/src/entities/__tests__/Entity.reparent.test.js @@ -0,0 +1,137 @@ +// Regression tests for Entity.reparent(). +// +// Reparenting must preserve the entity's WORLD transform: dragging an element +// under/out of/between parents in the editor should not make it jump on screen. +// These use the REAL `three` (not the lightweight mock) so attach()/matrixWorld +// math is genuinely exercised — that math is the whole point of the fix. +jest.mock("three", () => jest.requireActual("three")); +jest.mock("xstate", () => ({ + createMachine: jest.fn(), + interpret: jest.fn(() => ({ + start: jest.fn(), + stop: jest.fn(), + send: jest.fn(), + onTransition: jest.fn(), + })), +})); +jest.mock("../../scripts/Scripts", () => ({ get: jest.fn() })); +jest.mock("../../core/Scene", () => ({ getScene: jest.fn() })); +jest.mock("../../core/universe", () => ({ + getByTag: jest.fn(() => []), + set: jest.fn(), + remove: jest.fn(), + getByUUID: jest.fn(), +})); +jest.mock("../../lib/easing", () => ({ tweenTo: jest.fn(() => Promise.resolve()) })); +jest.mock("../../lib/meshUtils", () => ({ + isScene: jest.fn(() => false), + serializeQuaternion: jest.fn(q => q && { x: q.x, y: q.y, z: q.z, w: q.w }), + serializeVector: jest.fn(v => v && { x: v.x, y: v.y, z: v.z }), +})); + +import * as THREE from "three"; +import Entity from "../Entity"; +import Scene from "../../core/Scene"; + +const round = n => Math.round(n * 1e4) / 1e4; + +const worldPos = entity => { + const body = entity.getBody(); + body.updateWorldMatrix(true, false); + const v = new THREE.Vector3(); + body.getWorldPosition(v); + return { x: round(v.x), y: round(v.y), z: round(v.z) }; +}; + +const localPos = entity => { + const { x, y, z } = entity.getBody().position; + return { x: round(x), y: round(y), z: round(z) }; +}; + +const makeEntity = (x, y, z) => { + const e = new Entity(); + const body = new THREE.Object3D(); + body.position.set(x, y, z); + e.setBody({ body }); + return e; +}; + +// Helper: attach `child` under `parent` keeping the engine's Entity bookkeeping +// in sync with the THREE body tree. +const attachAsChild = (sceneRoot, parent, child) => { + parent.getBody().add(child.getBody()); + parent.children.push(child); + child.setParent(parent); + sceneRoot.updateMatrixWorld(true); +}; + +describe("Entity.reparent – world transform preservation", () => { + let sceneRoot; + + beforeEach(() => { + jest.clearAllMocks(); + sceneRoot = new THREE.Scene(); + Scene.getScene.mockReturnValue(sceneRoot); + }); + + test("moving a child OUT to the scene root keeps its world position", () => { + const parent = makeEntity(5, 5, 5); + sceneRoot.add(parent.getBody()); + const child = makeEntity(5, 5, 5); // local (5,5,5) under parent → world (10,10,10) + attachAsChild(sceneRoot, parent, child); + expect(worldPos(child)).toEqual({ x: 10, y: 10, z: 10 }); + + child.reparent(null); + sceneRoot.updateMatrixWorld(true); + + // World position preserved. (Before the fix this regressed to (5,5,5), + // i.e. the stale local offset was kept as the new world position.) + expect(worldPos(child)).toEqual({ x: 10, y: 10, z: 10 }); + }); + + test("moving a root child INTO a parent keeps its world position", () => { + const parent = makeEntity(5, 5, 5); + sceneRoot.add(parent.getBody()); + const child = makeEntity(10, 10, 10); // world (10,10,10) at root + sceneRoot.add(child.getBody()); + sceneRoot.updateMatrixWorld(true); + + child.reparent(parent); + sceneRoot.updateMatrixWorld(true); + + expect(worldPos(child)).toEqual({ x: 10, y: 10, z: 10 }); + // The inspector now shows the local offset from the parent. + expect(localPos(child)).toEqual({ x: 5, y: 5, z: 5 }); + }); + + test("moving a child BETWEEN two parents keeps its world position", () => { + const parentA = makeEntity(5, 5, 5); + const parentB = makeEntity(100, 0, 0); + sceneRoot.add(parentA.getBody()); + sceneRoot.add(parentB.getBody()); + const child = makeEntity(5, 5, 5); // local under A → world (10,10,10) + attachAsChild(sceneRoot, parentA, child); + expect(worldPos(child)).toEqual({ x: 10, y: 10, z: 10 }); + + child.reparent(parentB); + sceneRoot.updateMatrixWorld(true); + + expect(worldPos(child)).toEqual({ x: 10, y: 10, z: 10 }); + expect(localPos(child)).toEqual({ x: -90, y: 10, z: 10 }); + }); + + test("keeps Entity children arrays and parent pointers in sync", () => { + const parent = makeEntity(0, 0, 0); + sceneRoot.add(parent.getBody()); + const child = makeEntity(1, 2, 3); + sceneRoot.add(child.getBody()); + + child.reparent(parent); + expect(parent.children).toContain(child); + expect(child.getParent()).toBe(parent); + + child.reparent(null); + expect(parent.children).not.toContain(child); + expect(child.getParent()).toBe(false); + }); +});