Skip to content

Commit 9b06596

Browse files
fix(ecs): delegate engine.getEntityState instead of aliasing the container's
Two findings from the review of this branch that belong with it, because the branch is what made them matter. engine/index.ts exposed `getEntityState: partialEngine.entityContainer.getEntityState` — a detached method reference, so calling `engine.getEntityState(e)` binds `this` to the engine object rather than the container. Harmless for the built-in container, whose implementation is a closure, but a custom IEntityContainer that uses `this` silently reports the WRONG state through the public API while the engine itself reports the right one, because removeEntity calls the container directly. That divergence was cosmetic before; this branch made removeEntity classify through getEntityState, so the public API would now contradict the engine's own behaviour. Delegated instead of aliased. Pinned with a class-based container that reads `this.bound`. Negative control: restoring the aliased form fails 1 of 29. Also documents on IEngine.removeEntityWithChildren that it can complete only partially and does not report it. `removeEntity` refuses renderer-reserved nodes, so a reserved node anywhere in a Transform tree survives while its descendants are removed, leaving its `Transform.parent` pointing at a removed entity. That is reachable only if a scene parents a reserved entity under a scene entity, and fixing it properly means either skipping such subtrees or returning a result — a public API change this PR should not carry. Documented rather than silently left as a surprise. Deliberately NOT fixed here: systems/crdt/index.ts purges every component for any inbound DELETE_ENTITY with no reserved-range guard, which is reachable from the network transport and lets a peer erase a live remote player's identity and transform from a scene, which the scene then forwards onward. An unconditional guard there would be wrong — the renderer's tombstone for a departed player MUST purge — so the fix is transport-gating, a separate change to the CRDT receive semantics that deserves its own review. Filing separately. Public API surface unchanged: still one line for this branch. Full suite: 160 suites, 1231 tests.
1 parent 452acee commit 9b06596

15 files changed

Lines changed: 101 additions & 31 deletions

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,12 @@ export function Engine(options?: IEngineOptions): IEngine {
354354
PlayerEntity: 1 as Entity,
355355
CameraEntity: 2 as Entity,
356356

357-
getEntityState: partialEngine.entityContainer.getEntityState,
357+
// Delegated, not aliased. A detached `entityContainer.getEntityState` reference binds
358+
// `this` to the engine, so a custom IEntityContainer that uses `this` silently reports the
359+
// wrong state through the public API while the engine itself, which calls the container
360+
// directly, reports the right one. Harmless for the built-in closure-based container, but
361+
// removeEntity now classifies through getEntityState, so the two must never diverge.
362+
getEntityState: (entity: Entity) => partialEngine.entityContainer.getEntityState(entity),
358363
addTransport: crdtSystem.addTransport,
359364

360365
entityContainer: partialEngine.entityContainer

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,10 @@ export interface IEngine {
101101
/**
102102
* Remove all components of each entity in the tree made with Transform parenting
103103
* @param entity - the root entity of the tree
104+
*
105+
* May complete only partially and does not report it: nodes in the renderer-reserved range
106+
* are refused by `removeEntity`, so a reserved node anywhere in the tree survives while its
107+
* descendants are removed — leaving its `Transform.parent` pointing at a removed entity.
104108
*/
105109
removeEntityWithChildren(entity: Entity): void
106110

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

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { Engine } from '../../packages/@dcl/ecs/src/engine'
22
import {
33
Entity,
4+
EntityState,
45
EntityUtils,
56
IEntityContainer,
67
RESERVED_STATIC_ENTITIES,
@@ -272,6 +273,66 @@ describe('Reserved entity range ownership', () => {
272273
// The renderer applies scene component ops on the three named static entities — that is how
273274
// InputModifier.deleteFrom(engine.PlayerEntity) clears an input lock — so a removal must
274275
// still purge them, even though their ids are reserved and never released.
276+
// engine.getEntityState must delegate to the container rather than alias its method. An
277+
// aliased reference binds `this` to the engine, so a container that uses `this` reports the
278+
// wrong state through the public API while the engine — which calls the container directly —
279+
// reports the right one. removeEntity now classifies via getEntityState, so a divergence
280+
// there means the public API contradicts the engine's own behaviour.
281+
describe('when a custom container depends on `this`', () => {
282+
const CUSTOM_BOUND = 1000
283+
284+
let engine: ReturnType<typeof Engine>
285+
let rendererOwned: Entity
286+
287+
beforeEach(() => {
288+
class ThisDependentContainer {
289+
readonly bound = CUSTOM_BOUND
290+
private readonly used = new Set<number>()
291+
private counter = CUSTOM_BOUND
292+
generateEntity(): Entity {
293+
const entity = EntityUtils.toEntityId(this.counter++, 0)
294+
this.used.add(entity as number)
295+
return entity
296+
}
297+
removeEntity(entity: Entity): boolean {
298+
if (EntityUtils.fromEntityId(entity)[0] < this.bound) return false
299+
this.used.delete(entity as number)
300+
return true
301+
}
302+
getEntityState(entity: Entity): EntityState {
303+
if (EntityUtils.fromEntityId(entity)[0] < this.bound) return EntityState.Reserved
304+
return this.used.has(entity as number) ? EntityState.UsedEntity : EntityState.Unknown
305+
}
306+
getExistingEntities(): Set<Entity> {
307+
return new Set([...this.used] as Entity[])
308+
}
309+
releaseRemovedEntities(): Entity[] {
310+
return []
311+
}
312+
updateRemovedEntity(): boolean {
313+
return false
314+
}
315+
updateUsedEntity(): boolean {
316+
return false
317+
}
318+
}
319+
engine = Engine({ onChangeFunction: () => {}, entityContainer: new ThisDependentContainer() })
320+
rendererOwned = EntityUtils.toEntityId(700, 0)
321+
})
322+
323+
it('should report the state the container reports, not undefined-compared garbage', () => {
324+
expect(engine.getEntityState(rendererOwned)).toBe(EntityState.Reserved)
325+
})
326+
327+
it('should agree with what removeEntity actually does', () => {
328+
expect(engine.removeEntity(rendererOwned)).toBe(false)
329+
})
330+
331+
it('should still classify a container-owned entity as releasable', () => {
332+
expect(engine.removeEntity(engine.addEntity())).toBe(true)
333+
})
334+
})
335+
275336
describe('when a scene removes a named static entity', () => {
276337
let engine: ReturnType<typeof Engine>
277338
let InputLock: ReturnType<ReturnType<typeof Engine>['defineComponent']>

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

Lines changed: 3 additions & 3 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=621k bytes
22
THE BUNDLE HAS SOURCEMAPS
33
(start empty vm 0.21.0-3680274614.commit-1808aa1)
44
OPCODES ~= 0k
@@ -10,7 +10,7 @@ EVAL test/snapshots/development-bundles/static-scene.test.js
1010
REQUIRE: ~system/EngineApi
1111
REQUIRE: ~system/Runtime
1212
OPCODES ~= 74k
13-
MALLOC_COUNT = 16875
13+
MALLOC_COUNT = 16880
1414
ALIVE_OBJS_DELTA ~= 3.31k
1515
CALL onStart()
1616
main.crdt: PUT_COMPONENT e=0x200 c=1 t=0 data={"position":{"x":5.880000114440918,"y":2.7916901111602783,"z":7.380000114440918},"rotation":{"x":0,"y":0,"z":0,"w":1},"scale":{"x":1,"y":1,"z":1},"parent":0}
@@ -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.18k bytes
58+
MEMORY_USAGE_COUNT ~= 1548.92k bytes

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

Lines changed: 4 additions & 4 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.5k bytes
22
THE BUNDLE HAS SOURCEMAPS
33
(start empty vm 0.21.0-3680274614.commit-1808aa1)
44
OPCODES ~= 0k
@@ -10,8 +10,8 @@ EVAL test/snapshots/development-bundles/testing-fw.test.js
1010
REQUIRE: ~system/EngineApi
1111
REQUIRE: ~system/Runtime
1212
OPCODES ~= 84k
13-
MALLOC_COUNT = 17427
14-
ALIVE_OBJS_DELTA ~= 3.46k
13+
MALLOC_COUNT = 17435
14+
ALIVE_OBJS_DELTA ~= 3.47k
1515
CALL onStart()
1616
LOG: ["Adding one to position.y=0"]
1717
Renderer: PUT_COMPONENT e=0x0 c=1 t=1 data={"position":{"x":1,"y":0,"z":0},"rotation":{"x":0,"y":0,"z":0,"w":1},"scale":{"x":9,"y":9,"z":9},"parent":0}
@@ -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.88k bytes
64+
MEMORY_USAGE_COUNT ~= 1554.69k bytes

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
SCENE_COMPILED_JS_SIZE_PROD=621.1k bytes
1+
SCENE_COMPILED_JS_SIZE_PROD=621.5k bytes
22
THE BUNDLE HAS SOURCEMAPS
33
(start empty vm 0.21.0-3680274614.commit-1808aa1)
44
OPCODES ~= 0k
@@ -10,8 +10,8 @@ EVAL test/snapshots/development-bundles/two-way-crdt.test.js
1010
REQUIRE: ~system/EngineApi
1111
REQUIRE: ~system/Runtime
1212
OPCODES ~= 84k
13-
MALLOC_COUNT = 17427
14-
ALIVE_OBJS_DELTA ~= 3.46k
13+
MALLOC_COUNT = 17435
14+
ALIVE_OBJS_DELTA ~= 3.47k
1515
CALL onStart()
1616
LOG: ["Adding one to position.y=0"]
1717
Renderer: PUT_COMPONENT e=0x0 c=1 t=1 data={"position":{"x":1,"y":0,"z":0},"rotation":{"x":0,"y":0,"z":0,"w":1},"scale":{"x":9,"y":9,"z":9},"parent":0}
@@ -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.88k bytes
64+
MEMORY_USAGE_COUNT ~= 1554.69k bytes

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ EVAL test/snapshots/production-bundles/append-value-crdt.js
88
REQUIRE: ~system/EngineApi
99
REQUIRE: ~system/Runtime
1010
OPCODES ~= 88k
11-
MALLOC_COUNT = 15751
11+
MALLOC_COUNT = 15759
1212
ALIVE_OBJS_DELTA ~= 3.53k
1313
CALL onStart()
1414
Renderer: APPEND_VALUE e=0x200 c=1063 t=0 data={"button":0,"hit":{"position":{"x":1,"y":2,"z":3},"globalOrigin":{"x":1,"y":2,"z":3},"direction":{"x":1,"y":2,"z":3},"normalHit":{"x":1,"y":2,"z":3},"length":10,"meshName":"mesh","entityId":512},"state":1,"timestamp":1,"analog":5,"tickNumber":0}
@@ -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.76k bytes
57+
MEMORY_USAGE_COUNT ~= 1139.07k bytes

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ EVAL test/snapshots/production-bundles/billboard.js
88
REQUIRE: ~system/EngineApi
99
REQUIRE: ~system/Runtime
1010
OPCODES ~= 89k
11-
MALLOC_COUNT = 18260
11+
MALLOC_COUNT = 18265
1212
ALIVE_OBJS_DELTA ~= 4.00k
1313
CALL onStart()
1414
OPCODES ~= 0k
@@ -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.67k bytes
79+
MEMORY_USAGE_COUNT ~= 1324.91k bytes

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ EVAL test/snapshots/production-bundles/cube-deleted.js
88
REQUIRE: ~system/EngineApi
99
REQUIRE: ~system/Runtime
1010
OPCODES ~= 78k
11-
MALLOC_COUNT = 14871
11+
MALLOC_COUNT = 14876
1212
ALIVE_OBJS_DELTA ~= 3.30k
1313
CALL onStart()
1414
OPCODES ~= 0k
@@ -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 ~= 1100.90k bytes
44+
MEMORY_USAGE_COUNT ~= 1101.14k bytes

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ EVAL test/snapshots/production-bundles/cube.js
88
REQUIRE: ~system/EngineApi
99
REQUIRE: ~system/Runtime
1010
OPCODES ~= 78k
11-
MALLOC_COUNT = 14844
11+
MALLOC_COUNT = 14849
1212
ALIVE_OBJS_DELTA ~= 3.29k
1313
CALL onStart()
1414
OPCODES ~= 0k
@@ -31,4 +31,4 @@ CALL onUpdate(0.1)
3131
OPCODES ~= 3k
3232
MALLOC_COUNT = 0
3333
ALIVE_OBJS_DELTA ~= 0.00k
34-
MEMORY_USAGE_COUNT ~= 1090.66k bytes
34+
MEMORY_USAGE_COUNT ~= 1090.90k bytes

0 commit comments

Comments
 (0)