Skip to content

Commit 7d3314f

Browse files
felixtrzmeta-codesync[bot]
authored andcommitted
fix(core): auto-clone GLTF scene in AssetManager.getGLTF
Summary: `AssetManager.getGLTF(key)` previously returned the cached `GLTF` directly, so adding `gltf.scene` to multiple parents silently re-parented the single shared `Object3D` — only the last entity actually displayed the asset, with no error or warning. Reusing one key across entities (weapons, plants, robots, vehicles) required a hand-rolled `cloneAsset` helper in every project. Make `getGLTF` return a fresh clone by default. `scene` and `scenes` are cloned via `SkeletonUtils.clone` (correctly handles `SkinnedMesh`/`Bone` rebinding); geometries, materials, animations, cameras, asset, parser, and userData remain shared by reference for memory efficiency. `AnimationClip`s bind by name to bones in the cloned skeleton, so animations continue to play on the clone. Pass `{ shared: true }` to retrieve the cached instance directly (escape hatch for advanced / framework code that intentionally mutates the canonical instance). `loadGLTF` is unchanged — it remains the loader / cache populator and returns the canonical instance; `getGLTF` is the instance-getter. JSDoc on both methods now points users at the right entry point. Drop the now-redundant manual `.scene.clone()` calls in `examples/environment-raycast/src/raycast-plant.ts`. Update the starter template / agent guidance docs (`PROJECT_AGENTS.md`, `PROJECT_CLAUDE.md`, `iwsdk-planner` SKILL) to mention the new default and the `{ shared: true }` opt-out. Add a behavior-change entry to `packages/core/CHANGELOG.md` under "Unreleased". Reviewed By: zjm-meta Differential Revision: D104883297 fbshipit-source-id: b005b81a8c24d3f0b1f3c96320281f3a7aff1087
1 parent 28f3901 commit 7d3314f

8 files changed

Lines changed: 257 additions & 11 deletions

File tree

examples/environment-raycast/src/raycast-plant.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ export class RaycastPlantSystem extends createSystem({
3737
return;
3838
}
3939

40-
this.previewPlant = plantGltf.scene.clone();
40+
this.previewPlant = plantGltf.scene;
4141
this.scene.add(this.previewPlant);
4242

4343
// Create an entity with EnvironmentRaycastTarget to track raycast hits
@@ -75,7 +75,7 @@ export class RaycastPlantSystem extends createSystem({
7575
return;
7676
}
7777

78-
const newPlant = plantGltf.scene.clone();
78+
const newPlant = plantGltf.scene;
7979
newPlant.position.copy(position);
8080
newPlant.quaternion.copy(quaternion);
8181

packages/core/CHANGELOG.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,18 @@
11
# @iwsdk/core
22

3+
## Unreleased
4+
5+
### Behavior Changes
6+
7+
- `AssetManager.getGLTF(key)` (and `GLTFAssetLoader.getGLTF(key)`) now
8+
return a fresh clone of the cached `GLTF` by default. `scene` and
9+
`scenes` are cloned via `SkeletonUtils.clone` (correct for
10+
`SkinnedMesh`/`Bone` hierarchies); geometries, materials, and
11+
`animations` remain shared by reference. This fixes silent re-parenting
12+
when the same key is used to spawn multiple entities (T270858760).
13+
Pass `{ shared: true }` to opt back into the previous shared-instance
14+
behavior.
15+
316
## 0.4.0
417

518
### Minor Changes

packages/core/src/asset/asset-manager.ts

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,12 @@ import type { World } from '../ecs/index.js';
1010
import { LoadingManager, Texture, WebGLRenderer } from '../runtime/index.js';
1111
import { CacheManager } from './cache-manager.js';
1212
import { AudioAssetLoader } from './loaders/audio-loader.js';
13-
import { GLTFAssetLoader } from './loaders/gltf-loader.js';
13+
import { GetGLTFOptions, GLTFAssetLoader } from './loaders/gltf-loader.js';
1414
import { HDRTextureAssetLoader } from './loaders/hdr-texture-loader.js';
1515
import { TextureAssetLoader } from './loaders/texture-loader.js';
1616

17+
export type { GetGLTFOptions } from './loaders/gltf-loader.js';
18+
1719
/**
1820
* Asset types supported by the {@link AssetManager}.
1921
* @category Assets
@@ -120,7 +122,14 @@ export class AssetManager {
120122
}
121123
}
122124

123-
/** Load a GLTF by URL; optionally register a logical key. */
125+
/**
126+
* Load a GLTF by URL; optionally register a logical key.
127+
*
128+
* @remarks
129+
* Resolves with the cached `GLTF` directly. Use {@link AssetManager.getGLTF}
130+
* after the load resolves to retrieve a clone suitable for placing into
131+
* multiple entities.
132+
*/
124133
static loadGLTF(url: string, key?: string): Promise<GLTF> {
125134
return GLTFAssetLoader.loadGLTF(url, key);
126135
}
@@ -173,8 +182,16 @@ export class AssetManager {
173182
return HDRTextureAssetLoader.loadHDRTexture(url);
174183
}
175184

176-
/** Get a cached GLTF by logical key. */
177-
static getGLTF(key: string): GLTF | null {
178-
return GLTFAssetLoader.getGLTF(key);
185+
/**
186+
* Get a cached GLTF by logical key.
187+
*
188+
* @remarks
189+
* Returns a fresh clone by default (`scene`/`scenes` are new `Object3D`
190+
* trees; geometries, materials, animations stay shared), so the same key
191+
* may be safely used for multiple entities. Pass `{ shared: true }` to
192+
* return the cached instance directly.
193+
*/
194+
static getGLTF(key: string, options?: GetGLTFOptions): GLTF | null {
195+
return GLTFAssetLoader.getGLTF(key, options);
179196
}
180197
}

packages/core/src/asset/loaders/gltf-loader.ts

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,26 @@
88
import { DRACOLoader } from 'three/examples/jsm/loaders/DRACOLoader.js';
99
import { GLTF, GLTFLoader } from 'three/examples/jsm/loaders/GLTFLoader.js';
1010
import { KTX2Loader } from 'three/examples/jsm/loaders/KTX2Loader.js';
11+
import { clone as cloneSkinned } from 'three/examples/jsm/utils/SkeletonUtils.js';
1112
import {
13+
Group,
1214
LoadingManager,
1315
REVISION,
1416
WebGLRenderer,
1517
} from '../../runtime/index.js';
1618
import { CacheManager } from '../cache-manager.js';
1719

20+
/** Options for retrieving a cached GLTF. @category Assets */
21+
export interface GetGLTFOptions {
22+
/**
23+
* If true, return the cached GLTF directly (shared across calls).
24+
* Default `false`: returns a fresh clone whose `scene`/`scenes` are new
25+
* `Object3D` trees, allowing the same key to be used for multiple entities.
26+
* Geometries, materials, and animation clips remain shared by reference.
27+
*/
28+
shared?: boolean;
29+
}
30+
1831
const THREE_PATH = `https://unpkg.com/three@0.${REVISION}.0`;
1932

2033
/**
@@ -59,7 +72,14 @@ export class GLTFAssetLoader {
5972
.setKTX2Loader(this.ktx2Loader);
6073
}
6174

62-
/** Load a GLTF by URL, caching the result; optionally register a logical key. */
75+
/**
76+
* Load a GLTF by URL, caching the result; optionally register a logical key.
77+
*
78+
* @remarks
79+
* Resolves with the cached `GLTF` instance directly. To retrieve a clone
80+
* suitable for placing into multiple entities, call {@link GLTFAssetLoader.getGLTF}
81+
* (or `AssetManager.getGLTF`) by key after the load resolves.
82+
*/
6383
static loadGLTF(url: string, key?: string): Promise<GLTF> {
6484
// Always use URL as cache key for consistent caching
6585
if (CacheManager.hasPromise(url)) {
@@ -96,8 +116,34 @@ export class GLTFAssetLoader {
96116
}
97117
}
98118

99-
/** Get a cached GLTF by logical key. */
100-
static getGLTF(key: string): GLTF | null {
101-
return (CacheManager.getAssetByKey(key) as GLTF) || null;
119+
/**
120+
* Get a cached GLTF by logical key.
121+
*
122+
* @remarks
123+
* By default, returns a fresh clone: `scene` and `scenes` are new
124+
* `Object3D` trees (correctly handling `SkinnedMesh`/`Bone` hierarchies),
125+
* while geometries, materials, `animations`, `cameras`, `asset`,
126+
* `parser`, and `userData` remain shared by reference. This makes it
127+
* safe to call `getGLTF(key)` once per entity — adding the result's
128+
* `scene` to multiple parents will not silently re-parent a single
129+
* shared object.
130+
*
131+
* Pass `{ shared: true }` to return the cached `GLTF` directly (the
132+
* pre-0.4.x behavior), e.g. for framework code that intentionally
133+
* mutates the canonical instance.
134+
*/
135+
static getGLTF(key: string, options: GetGLTFOptions = {}): GLTF | null {
136+
const cached = CacheManager.getAssetByKey(key) as GLTF | undefined;
137+
if (!cached) {
138+
return null;
139+
}
140+
if (options.shared) {
141+
return cached;
142+
}
143+
return {
144+
...cached,
145+
scene: cloneSkinned(cached.scene) as Group,
146+
scenes: cached.scenes.map((s) => cloneSkinned(s) as Group),
147+
};
102148
}
103149
}
Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
/**
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
import { GLTF } from 'three/examples/jsm/loaders/GLTFLoader.js';
9+
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
10+
import { CacheManager } from '../../src/asset/cache-manager.js';
11+
import { GLTFAssetLoader } from '../../src/asset/loaders/gltf-loader.js';
12+
import {
13+
AnimationClip,
14+
Bone,
15+
BufferAttribute,
16+
BufferGeometry,
17+
Group,
18+
Mesh,
19+
MeshStandardMaterial,
20+
Skeleton,
21+
SkinnedMesh,
22+
SphereGeometry,
23+
} from '../../src/runtime/three.js';
24+
25+
function makeFakeGLTF(): GLTF {
26+
const geometry = new SphereGeometry();
27+
const material = new MeshStandardMaterial();
28+
const mesh = new Mesh(geometry, material);
29+
mesh.name = 'TestMesh';
30+
31+
const scene = new Group();
32+
scene.name = 'TestScene';
33+
scene.add(mesh);
34+
35+
const animations: AnimationClip[] = [];
36+
return {
37+
scene,
38+
scenes: [scene],
39+
animations,
40+
cameras: [],
41+
asset: { version: '2.0' },
42+
parser: undefined as unknown as GLTF['parser'],
43+
userData: {},
44+
};
45+
}
46+
47+
describe('GLTFAssetLoader.getGLTF', () => {
48+
beforeEach(() => {
49+
CacheManager.clear();
50+
const gltf = makeFakeGLTF();
51+
CacheManager.setKeyToUrl('test', 'https://example.invalid/test.glb');
52+
CacheManager.setAsset('https://example.invalid/test.glb', gltf);
53+
});
54+
55+
afterEach(() => {
56+
CacheManager.clear();
57+
});
58+
59+
it('returns null for unknown keys', () => {
60+
expect(GLTFAssetLoader.getGLTF('missing')).toBeNull();
61+
});
62+
63+
it('returns a fresh scene clone on every call by default', () => {
64+
const a = GLTFAssetLoader.getGLTF('test');
65+
const b = GLTFAssetLoader.getGLTF('test');
66+
expect(a).not.toBeNull();
67+
expect(b).not.toBeNull();
68+
expect(a!.scene).not.toBe(b!.scene);
69+
expect(a!.scenes[0]).not.toBe(b!.scenes[0]);
70+
});
71+
72+
it('shares geometries, materials, and animations across clones', () => {
73+
const a = GLTFAssetLoader.getGLTF('test')!;
74+
const b = GLTFAssetLoader.getGLTF('test')!;
75+
const meshA = a.scene.children[0] as Mesh;
76+
const meshB = b.scene.children[0] as Mesh;
77+
expect(meshA.geometry).toBe(meshB.geometry);
78+
expect(meshA.material).toBe(meshB.material);
79+
expect(a.animations).toBe(b.animations);
80+
});
81+
82+
it('returns the cached instance when { shared: true }', () => {
83+
const a = GLTFAssetLoader.getGLTF('test', { shared: true });
84+
const b = GLTFAssetLoader.getGLTF('test', { shared: true });
85+
expect(a).not.toBeNull();
86+
expect(a).toBe(b);
87+
expect(a!.scene).toBe(b!.scene);
88+
});
89+
90+
it('returns scenes with no parent so multi-entity placement does not steal ownership', () => {
91+
const parentA = new Group();
92+
const parentB = new Group();
93+
94+
const a = GLTFAssetLoader.getGLTF('test')!;
95+
parentA.add(a.scene);
96+
97+
const b = GLTFAssetLoader.getGLTF('test')!;
98+
parentB.add(b.scene);
99+
100+
expect(a.scene.parent).toBe(parentA);
101+
expect(b.scene.parent).toBe(parentB);
102+
expect(parentA.children).toContain(a.scene);
103+
expect(parentB.children).toContain(b.scene);
104+
});
105+
106+
it('rebinds SkinnedMesh skeletons so each clone has its own bones', () => {
107+
const root = new Group();
108+
root.name = 'SkinnedRoot';
109+
110+
const bone = new Bone();
111+
bone.name = 'Bone0';
112+
root.add(bone);
113+
114+
const geometry = new BufferGeometry();
115+
geometry.setAttribute(
116+
'skinIndex',
117+
new BufferAttribute(new Uint16Array(4), 4),
118+
);
119+
geometry.setAttribute(
120+
'skinWeight',
121+
new BufferAttribute(new Float32Array(4), 4),
122+
);
123+
const skinnedMesh = new SkinnedMesh(geometry, new MeshStandardMaterial());
124+
skinnedMesh.name = 'SkinnedTestMesh';
125+
skinnedMesh.bind(new Skeleton([bone]));
126+
root.add(skinnedMesh);
127+
128+
CacheManager.setAsset('https://example.invalid/test.glb', {
129+
scene: root,
130+
scenes: [root],
131+
animations: [],
132+
cameras: [],
133+
asset: { version: '2.0' },
134+
parser: undefined as unknown as GLTF['parser'],
135+
userData: {},
136+
});
137+
138+
const a = GLTFAssetLoader.getGLTF('test')!;
139+
const b = GLTFAssetLoader.getGLTF('test')!;
140+
141+
const meshA = a.scene.getObjectByName('SkinnedTestMesh') as SkinnedMesh;
142+
const meshB = b.scene.getObjectByName('SkinnedTestMesh') as SkinnedMesh;
143+
const boneA = a.scene.getObjectByName('Bone0') as Bone;
144+
const boneB = b.scene.getObjectByName('Bone0') as Bone;
145+
146+
expect(meshA).toBeInstanceOf(SkinnedMesh);
147+
expect(meshB).toBeInstanceOf(SkinnedMesh);
148+
expect(meshA).not.toBe(meshB);
149+
150+
expect(boneA).not.toBe(boneB);
151+
expect(boneA.parent).toBe(a.scene);
152+
expect(boneB.parent).toBe(b.scene);
153+
154+
expect(meshA.skeleton).not.toBe(meshB.skeleton);
155+
expect(meshA.skeleton.bones[0]).toBe(boneA);
156+
expect(meshB.skeleton.bones[0]).toBe(boneB);
157+
});
158+
});

packages/starter-assets/PROJECT_AGENTS.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,9 @@ const world = await World.create(container, {
192192
assets: { myModel: { url: '/models/scene.glb', type: AssetType.GLTF } },
193193
});
194194
const gltf = AssetManager.getGLTF('myModel');
195+
// `getGLTF` returns a fresh clone of `scene`/`scenes` per call, so it's
196+
// safe to use the same key for multiple entities. Pass `{ shared: true }`
197+
// if you intentionally want the cached instance.
195198

196199
// ✅ GOOD - For runtime loading
197200
await AssetManager.loadGLTF(url, 'myModel');

packages/starter-assets/PROJECT_CLAUDE.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,9 @@ const world = await World.create(container, {
192192
assets: { myModel: { url: '/models/scene.glb', type: AssetType.GLTF } },
193193
});
194194
const gltf = AssetManager.getGLTF('myModel');
195+
// `getGLTF` returns a fresh clone of `scene`/`scenes` per call, so it's
196+
// safe to use the same key for multiple entities. Pass `{ shared: true }`
197+
// if you intentionally want the cached instance.
195198

196199
// ✅ GOOD - For runtime loading
197200
await AssetManager.loadGLTF(url, 'myModel');

packages/starter-assets/claude-injections/skills/iwsdk-planner/SKILL.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -564,6 +564,9 @@ const world = await World.create(container, {
564564

565565
// Access preloaded assets
566566
const model = AssetManager.getGLTF('myModel');
567+
// `getGLTF` returns a fresh clone of `scene`/`scenes` per call, so the
568+
// same key safely backs multiple entities. Use `{ shared: true }` for
569+
// the cached instance.
567570
const texture = AssetManager.getTexture('myTexture');
568571
const audio = AssetManager.getAudio('mySound');
569572
```
@@ -1253,6 +1256,9 @@ const world = await World.create(container, {
12531256

12541257
// Retrieve preloaded assets (synchronous — already loaded)
12551258
const gltf = AssetManager.getGLTF('myModel');
1259+
// `getGLTF` returns a fresh clone of `scene`/`scenes` per call, so the
1260+
// same key safely backs multiple entities. Pass `{ shared: true }` to
1261+
// get the cached instance.
12561262
const texture = AssetManager.getTexture('myTexture');
12571263
const audio = AssetManager.getAudio('mySound');
12581264
```

0 commit comments

Comments
 (0)