Skip to content

Commit d8e093e

Browse files
fix(ecs): take the renderer-streamed bound from the container, not the default
Review follow-up (P2). Engine.removeEntity called isRendererStreamedNumber with the module default of RESERVED_STATIC_ENTITIES, so the purge decision and the id-release decision could disagree whenever a custom container is injected via the public IEngineOptions.entityContainer seam. They are two halves of one question and have to use the same bound. Confirmed reachable, and it breaks in BOTH directions — the more damaging one being the opposite of what you might expect: createEntityContainer({ reservedStaticEntities: 64 }), entity #100 v0 container released the id? true components purged? false getEntitiesWith(C) after removal: 1 entry, still yields 100 = {"v":111} The id goes back into the recycling pool while the entity stays visible to component queries with its data intact — engine.removeEntity returns true and the entity is not removed. A caller cannot detect that. The other direction is the narrower one: with reservedStaticEntities: 1024, entity #700 is renderer-owned per the container (release refused) yet the purge ran anyway, reopening for 512..1024 exactly the corruption this branch fixes. Fix: IEntityContainer exposes the bound it enforces, and Engine.removeEntity uses it. The member is OPTIONAL, so a third-party IEntityContainer stays source-compatible; when absent the engine falls back to the module default, which is correct for any container that also uses it. Public API grows by one line: + readonly reservedStaticEntities?: number; Tests cover both directions and fail 3/21 without the change. Note the named static entities stay at numbers 0-2 regardless of the bound — root, player and camera are fixed by the protocol, not by the reserved range — and a bound below 3 degenerates safely to "always purge", matching a container that reserves nothing. Snapshots regenerated. Full suite: 160 suites, 1223 tests.
1 parent ee5c3c9 commit d8e093e

16 files changed

Lines changed: 99 additions & 22 deletions

packages/@dcl/ecs/src/engine/entity.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,18 @@ export type IEntityContainer = {
9999
releaseRemovedEntities(): Entity[]
100100
updateRemovedEntity(entity: Entity): boolean
101101
updateUsedEntity(entity: Entity): boolean
102+
103+
/**
104+
* The exclusive upper bound of the renderer-reserved entity NUMBER range this container
105+
* enforces — i.e. the `reservedStaticEntities` it was built with.
106+
*
107+
* `Engine.removeEntity` needs it to decide whether to purge an entity's components, and
108+
* that decision has to agree with whether this container will release the id. Optional so
109+
* a custom `IEntityContainer` passed via `IEngineOptions.entityContainer` stays
110+
* source-compatible; when absent the engine falls back to `RESERVED_STATIC_ENTITIES`, which
111+
* only matches if the container also uses the default.
112+
*/
113+
readonly reservedStaticEntities?: number
102114
}
103115

104116
/**
@@ -293,6 +305,7 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
293305
}
294306

295307
return {
308+
reservedStaticEntities,
296309
generateEntity,
297310
removeEntity,
298311
getExistingEntities(): Set<Entity> {

packages/@dcl/ecs/src/engine/index.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,13 @@ function preEngine(options?: IEngineOptions): PreEngine {
6363
// range, so deletes on RootEntity/PlayerEntity/CameraEntity DO reach it and are applied.
6464
// Skipping those would silently break a working removal — the frame is byte-identical to
6565
// InputModifier.deleteFrom(engine.PlayerEntity).
66-
if (!isRendererStreamedNumber(entity)) {
66+
// Bound taken from the CONTAINER, not the module default, so the purge decision and the
67+
// id-release decision cannot disagree. They are two halves of one question, and a custom
68+
// container injected via IEngineOptions.entityContainer may enforce a different range: with
69+
// a larger one the purge would run on entities the container treats as renderer-owned, and
70+
// with a smaller one it would be skipped for scene-owned entities whose ids ARE released —
71+
// leaving a "removed" entity still visible to getEntitiesWith with its data intact.
72+
if (!isRendererStreamedNumber(entity, entityContainer.reservedStaticEntities)) {
6773
for (const [, component] of componentsDefinition) {
6874
// TODO: hack for the moment.
6975
// We still need the NetworkEntity to forward this message to the SyncTransport.

packages/@dcl/playground-assets/etc/playground-assets.api.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1487,6 +1487,7 @@ export type IEntityContainer = {
14871487
releaseRemovedEntities(): Entity[];
14881488
updateRemovedEntity(entity: Entity): boolean;
14891489
updateUsedEntity(entity: Entity): boolean;
1490+
readonly reservedStaticEntities?: number;
14901491
};
14911492

14921493
// @public (undocumented)

test/ecs/reserved-entity-range.spec.ts

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,8 +104,7 @@ describe('Reserved entity range ownership', () => {
104104
// createEntityContainer reads the define at call time, so assigning the global first
105105
// reproduces the build faithfully: counter starts at 746 while only 230 composite
106106
// entities are ever marked used, leaving a permanent deficit of 4.
107-
;(globalThis as unknown as { DCL_MAX_COMPOSITE_ENTITY: number }).DCL_MAX_COMPOSITE_ENTITY =
108-
MAX_COMPOSITE_ENTITY
107+
;(globalThis as unknown as { DCL_MAX_COMPOSITE_ENTITY: number }).DCL_MAX_COMPOSITE_ENTITY = MAX_COMPOSITE_ENTITY
109108
entityContainer = createEntityContainer()
110109
for (let i = 0; i < COMPOSITE_ENTITY_COUNT; i++) {
111110
entityContainer.updateUsedEntity(EntityUtils.toEntityId(RESERVED_STATIC_ENTITIES + i, 0))
@@ -191,6 +190,64 @@ describe('Reserved entity range ownership', () => {
191190
})
192191
})
193192

193+
// IEngineOptions.entityContainer is a public injection seam, so Engine.removeEntity must take
194+
// the reserved bound from the CONTAINER rather than the module default. Using the default
195+
// breaks in both directions, and neither is detectable by a caller.
196+
describe('when a custom entity container enforces a different reserved bound', () => {
197+
let engine: ReturnType<typeof Engine>
198+
let Owned: ReturnType<ReturnType<typeof Engine>['defineComponent']>
199+
200+
describe('and the bound is LOWER than the default, so the id IS released', () => {
201+
let sceneEntity: Entity
202+
203+
beforeEach(() => {
204+
engine = Engine({ entityContainer: createEntityContainer({ reservedStaticEntities: 64 }) })
205+
Owned = engine.defineComponent('test::owned', { value: Schemas.Int })
206+
// Number 100 is scene-owned under a bound of 64, but sits inside the default range.
207+
sceneEntity = EntityUtils.toEntityId(100, 0)
208+
Owned.create(sceneEntity, { value: 111 })
209+
})
210+
211+
it('should release the id, confirming the container considers it scene-owned', () => {
212+
expect(engine.removeEntity(sceneEntity)).toBe(true)
213+
})
214+
215+
it('should purge its components, so a released id cannot keep them', () => {
216+
engine.removeEntity(sceneEntity)
217+
218+
expect(Owned.getOrNull(sceneEntity)).toBeNull()
219+
})
220+
221+
it('should stop yielding it from getEntitiesWith', () => {
222+
engine.removeEntity(sceneEntity)
223+
224+
expect(Array.from(engine.getEntitiesWith(Owned))).toHaveLength(0)
225+
})
226+
})
227+
228+
describe('and the bound is HIGHER than the default, so the id is NOT released', () => {
229+
let rendererOwned: Entity
230+
231+
beforeEach(() => {
232+
engine = Engine({ entityContainer: createEntityContainer({ reservedStaticEntities: 1024 }) })
233+
Owned = engine.defineComponent('test::owned', { value: Schemas.Int })
234+
// Number 700 is renderer-owned under a bound of 1024, but outside the default range.
235+
rendererOwned = EntityUtils.toEntityId(700, 0)
236+
Owned.create(rendererOwned, { value: 222 })
237+
})
238+
239+
it('should refuse to release the id', () => {
240+
expect(engine.removeEntity(rendererOwned)).toBe(false)
241+
})
242+
243+
it('should NOT purge its components, since the container treats it as renderer-owned', () => {
244+
engine.removeEntity(rendererOwned)
245+
246+
expect(Owned.getOrNull(rendererOwned)).not.toBeNull()
247+
})
248+
})
249+
})
250+
194251
describe('when a scene removes an entity it genuinely owns', () => {
195252
let engine: ReturnType<typeof Engine>
196253
let SceneOwned: ReturnType<ReturnType<typeof Engine>['defineComponent']>

test/snapshots/development-bundles/static-scene.test.ts.crdt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
SCENE_COMPILED_JS_SIZE_PROD=620.5k bytes
1+
SCENE_COMPILED_JS_SIZE_PROD=620.6k bytes
22
THE BUNDLE HAS SOURCEMAPS
33
(start empty vm 0.21.0-3680274614.commit-1808aa1)
44
OPCODES ~= 0k
@@ -55,4 +55,4 @@ CALL onUpdate(0.1)
5555
OPCODES ~= 5k
5656
MALLOC_COUNT = -5
5757
ALIVE_OBJS_DELTA ~= 0.00k
58-
MEMORY_USAGE_COUNT ~= 1548.16k bytes
58+
MEMORY_USAGE_COUNT ~= 1548.28k bytes

test/snapshots/development-bundles/testing-fw.test.ts.crdt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
SCENE_COMPILED_JS_SIZE_PROD=621k bytes
1+
SCENE_COMPILED_JS_SIZE_PROD=621.1k bytes
22
THE BUNDLE HAS SOURCEMAPS
33
(start empty vm 0.21.0-3680274614.commit-1808aa1)
44
OPCODES ~= 0k
@@ -61,4 +61,4 @@ CALL onUpdate(0.1)
6161
OPCODES ~= 6k
6262
MALLOC_COUNT = -53
6363
ALIVE_OBJS_DELTA ~= -0.01k
64-
MEMORY_USAGE_COUNT ~= 1553.86k bytes
64+
MEMORY_USAGE_COUNT ~= 1553.98k bytes

test/snapshots/development-bundles/two-way-crdt.test.ts.crdt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
SCENE_COMPILED_JS_SIZE_PROD=621k bytes
1+
SCENE_COMPILED_JS_SIZE_PROD=621.1k bytes
22
THE BUNDLE HAS SOURCEMAPS
33
(start empty vm 0.21.0-3680274614.commit-1808aa1)
44
OPCODES ~= 0k
@@ -61,4 +61,4 @@ CALL onUpdate(0.1)
6161
OPCODES ~= 6k
6262
MALLOC_COUNT = -53
6363
ALIVE_OBJS_DELTA ~= -0.01k
64-
MEMORY_USAGE_COUNT ~= 1553.86k bytes
64+
MEMORY_USAGE_COUNT ~= 1553.98k bytes

test/snapshots/production-bundles/append-value-crdt.ts.crdt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,4 +54,4 @@ CALL onUpdate(0.1)
5454
OPCODES ~= 15k
5555
MALLOC_COUNT = 31
5656
ALIVE_OBJS_DELTA ~= 0.01k
57-
MEMORY_USAGE_COUNT ~= 1138.88k bytes
57+
MEMORY_USAGE_COUNT ~= 1138.97k bytes

test/snapshots/production-bundles/billboard.ts.crdt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,4 +76,4 @@ CALL onUpdate(0.1)
7676
OPCODES ~= 12k
7777
MALLOC_COUNT = 0
7878
ALIVE_OBJS_DELTA ~= 0.00k
79-
MEMORY_USAGE_COUNT ~= 1324.78k bytes
79+
MEMORY_USAGE_COUNT ~= 1324.87k bytes

test/snapshots/production-bundles/cube-deleted.ts.crdt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,4 +41,4 @@ CALL onUpdate(0.1)
4141
OPCODES ~= 7k
4242
MALLOC_COUNT = 1
4343
ALIVE_OBJS_DELTA ~= 0.00k
44-
MEMORY_USAGE_COUNT ~= 1101.02k bytes
44+
MEMORY_USAGE_COUNT ~= 1101.11k bytes

0 commit comments

Comments
 (0)