Merge pull request #1548 from decentraland/chore/sync-main-to-authserver

chore: sync main to authserver

Author: pravusjif

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

@@ -101,6 +101,17 @@ export type IEntityContainer = {
   updateUsedEntity(entity: Entity): boolean
 }
 
+/**
+ * True when `entity`'s NUMBER falls in the renderer-reserved range, at any version.
+ *
+ * Masks rather than calling `EntityUtils.fromEntityId`, which allocates a tuple per call to
+ * read one number; this runs on every inbound CRDT message. `entity & MAX_U16` already lands
+ * in [0, 65535], so the `>>> 0` fromEntityId applies is a no-op here.
+ */
+function isReservedEntity(entity: Entity, reservedStaticEntities: number): boolean {
+  return (entity & MAX_U16) < reservedStaticEntities
+}
+
 /**
  * @public
  */
@@ -143,7 +154,11 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
     }
 
     for (const [number, version] of removedEntities.getMap()) {
-      if (version < MAX_U16) {
+      // Never recycle a renderer-reserved number: the renderer reissues those slots as
+      // toEntityId(number, version + 1) too, from the same stored version, so it would hand
+      // the scene an id belonging to a live remote player. Belt-and-braces — every writer
+      // into `removedEntities` refuses reserved numbers, so this cannot fire today.
+      if (number >= reservedStaticEntities && version < MAX_U16) {
         const entity = EntityUtils.toEntityId(number, version + 1)
         // If the entity is not being used, we can re-use it
         // If the entity was removed in this tick, we're not counting for the usedEntities, but we have it in the toRemoveEntityArray
@@ -158,7 +173,7 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
   }
 
   function removeEntity(entity: Entity) {
-    if (entity < reservedStaticEntities) return false
+    if (isReservedEntity(entity, reservedStaticEntities)) return false
 
     if (usedEntities.has(entity)) {
       usedEntities.delete(entity)
@@ -185,6 +200,12 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
   }
 
   function updateRemovedEntity(entity: Entity) {
+    // Called for EVERY inbound DELETE_ENTITY, including the renderer's tombstones for
+    // departed remote players — so this is the door reserved numbers would otherwise enter
+    // the free list through. They need no tombstone: getEntityState reports them Reserved
+    // before it consults `removedEntities`, so `Removed` is unreachable for them anyway.
+    if (isReservedEntity(entity, reservedStaticEntities)) return false
+
     const [n, v] = EntityUtils.fromEntityId(entity)
 
     // Update the removed entities map
@@ -199,6 +220,11 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
   }
 
   function updateUsedEntity(entity: Entity) {
+    // Same invariant as updateRemovedEntity. Unreachable from the CRDT path today
+    // (getEntityState returns `Reserved`, never `Unknown`), but the `v > 0` branch below
+    // would seed `removedEntities` with a reserved number.
+    if (isReservedEntity(entity, reservedStaticEntities)) return false
+
     const [n, v] = EntityUtils.fromEntityId(entity)
 
     // if the entity was removed then abort fast
@@ -216,11 +242,14 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
   }
 
   function getEntityState(entity: Entity): EntityState {
-    const [n, v] = EntityUtils.fromEntityId(entity)
-    if (n < reservedStaticEntities) {
+    // Same guard as the three above, so `Engine.removeEntity` — which classifies via
+    // getEntityState — cannot disagree with whether this container releases the id.
+    if (isReservedEntity(entity, reservedStaticEntities)) {
       return EntityState.Reserved
     }
 
+    const [n, v] = EntityUtils.fromEntityId(entity)
+
     if (usedEntities.has(entity)) {
       return EntityState.UsedEntity
     }

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

@@ -8,7 +8,7 @@ import { ByteBuffer } from '../serialization/ByteBuffer'
 import { crdtSceneSystem, OnChangeFunction } from '../systems/crdt'
 import { ComponentDefinition } from './component'
 import { createComponentDefinitionFromSchema } from './lww-element-set-component-definition'
-import { Entity, createEntityContainer } from './entity'
+import { Entity, EntityState, EntityUtils, createEntityContainer } from './entity'
 import { ReadonlyComponentSchema } from './readonly'
 import { SystemItem, SystemContainer, SystemFn, SYSTEMS_REGULAR_PRIORITY } from './systems'
 import type {
@@ -29,6 +29,9 @@ export * from './readonly'
 export * from './types'
 export { Entity, ByteBuffer, SystemItem, OnChangeFunction }
 
+/** RootEntity 0, PlayerEntity 1, CameraEntity 2 — see the engineInstance fields below. */
+const NAMED_STATIC_ENTITIES = 3
+
 function preEngine(options?: IEngineOptions): PreEngine {
   const entityContainer = options?.entityContainer ?? createEntityContainer()
   const componentsDefinition = new Map<number, ComponentDefinition<unknown>>()
@@ -49,6 +52,20 @@ function preEngine(options?: IEngineOptions): PreEngine {
     return entity
   }
   function removeEntity(entity: Entity) {
+    // The renderer streams the avatar range and drops the scene's deletes there, so purging
+    // locally is permanent: a one-shot component like PlayerIdentityData is never re-sent, and
+    // the entity is left as a moving Transform with no identity. The three named static
+    // entities are reserved too, but the renderer DOES apply scene deletes on them — that is
+    // how InputModifier.deleteFrom(engine.PlayerEntity) clears an input lock — so they must
+    // still be purged. Asking the container keeps this in step with whether it releases the
+    // id, including for a custom container with a different reserved range.
+    const [entityNumber] = EntityUtils.fromEntityId(entity)
+    const isAvatarEntity =
+      entityNumber >= NAMED_STATIC_ENTITIES && entityContainer.getEntityState(entity) === EntityState.Reserved
+
+    const released = entityContainer.removeEntity(entity)
+    if (isAvatarEntity) return released
+
     for (const [, component] of componentsDefinition) {
       // TODO: hack for the moment.
       // We still need the NetworkEntity to forward this message to the SyncTransport.
@@ -57,7 +74,7 @@ function preEngine(options?: IEngineOptions): PreEngine {
       component.entityDeleted(entity, true)
     }
 
-    return entityContainer.removeEntity(entity)
+    return released
   }
 
   function removeEntityWithChildren(entity: Entity) {
@@ -337,7 +354,12 @@ export function Engine(options?: IEngineOptions): IEngine {
     PlayerEntity: 1 as Entity,
     CameraEntity: 2 as Entity,
 
-    getEntityState: partialEngine.entityContainer.getEntityState,
+    // Delegated, not aliased. A detached `entityContainer.getEntityState` reference binds
+    // `this` to the engine, so a custom IEntityContainer that uses `this` silently reports the
+    // wrong state through the public API while the engine itself, which calls the container
+    // directly, reports the right one. Harmless for the built-in closure-based container, but
+    // removeEntity now classifies through getEntityState, so the two must never diverge.
+    getEntityState: (entity: Entity) => partialEngine.entityContainer.getEntityState(entity),
     addTransport: crdtSystem.addTransport,
 
     entityContainer: partialEngine.entityContainer

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

@@ -92,12 +92,19 @@ export interface IEngine {
    * @public
    * Remove all components of an entity
    * @param entity - entity
+   * @returns whether the entity id was released for reuse. Ids in the renderer-reserved range
+   *   are never released, at any version. Components are still purged for
+   *   RootEntity/PlayerEntity/CameraEntity, but not for the avatar range.
    */
-  removeEntity(entity: Entity): void
+  removeEntity(entity: Entity): boolean
 
   /**
    * Remove all components of each entity in the tree made with Transform parenting
    * @param entity - the root entity of the tree
+   *
+   * May complete only partially and does not report it: nodes in the renderer-reserved range
+   * are refused by `removeEntity`, so a reserved node anywhere in the tree survives while its
+   * descendants are removed — leaving its `Transform.parent` pointing at a removed entity.
    */
   removeEntityWithChildren(entity: Entity): void
 

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

@@ -1512,7 +1512,7 @@ export interface IEngine {
     registerComponentDefinition<T>(componentName: string, componentDefinition: ComponentDefinition<T>): ComponentDefinition<T>;
     // (undocumented)
     removeComponentDefinition(componentId: number | string): void;
-    removeEntity(entity: Entity): void;
+    removeEntity(entity: Entity): boolean;
     removeEntityWithChildren(entity: Entity): void;
     removeSystem(selector: string | SystemFn): boolean;
     readonly RootEntity: Entity;