Skip to content

Pool: retire the legacy name-keyed object pool from engine internals #1688

Description

@obiot

src/system/legacy_pool.js is the name-keyed object pool behind the public me.pool API. It predates the typed createPool helpers in src/pool.ts, and the two now overlap: geometry, colours, vectors and bounds all have typed pools, while the legacy one keys classes by string name and is what Tiled uses to instantiate objects it knows only by name.

Two engine-internal uses of it were removed in 20.7.0, both of which were latent bugs rather than working code:

  • Body#destroy had an else { pool.push(shape) } fallback that no shape could reach, because every type addShape can store has a typed pool branch above it. It had already forced two separate fixes (the Ellipse and Box3d branches exist only to dodge it).
  • Renderable#destroy handed the mask to it. mask is documented as Rect | RoundRect | Polygon | Line | Ellipse and no geometry class is registered, so this threw for every documented type. The recycling it appeared to do had never once happened, while destroy() threw for any renderable carrying a mask.

Removing the first also dissolved a module cycle: body.js was the only reason deprecated.js needed a hand-ordered import to avoid a temporal-dead-zone crash on legacy_pool's module state.

What still imports it

site use assessment
src/index.ts re-exports pool as me.pool public API, needs a deprecation path before it can move
src/system/bootstrap.ts pool.register(...) for 13 renderable classes required by the name-keyed instantiation below
src/level/tiled/TMXObjectFactory.js setPoolRegisterCallback the registration hook Tiled object parsing depends on
src/level/tiled/TMXTileMap.js pool.pull("ImageLayer", ...) Tiled names classes as strings, so something string-keyed is genuinely needed here
src/video/texture/atlas.js pool.pull("me.Sprite" / "me.NineSliceSprite", ...) looks like cycle avoidance rather than pooling: atlas.js cannot import Sprite directly without a cycle
src/renderable/container.js pool.push(child, false) correct as written, and the only internal push left

The hazard worth designing out

push(obj, throwOnError = true) throws by default for any class never pool.registered, and nothing at the call site makes that visible. Both bugs removed in 20.7.0 were instances of it, and both failed destructively: they threw partway through a destroy() that had already recycled other state, leaving a half-torn-down object that died later somewhere unrelated. container.js is the only internal caller that passes false.

At minimum, throwOnError should default to false, or push should be split into a throwing and a non-throwing form so the choice is explicit at the call site.

Suggested scope

  1. Flip or split the push default so a missed registration cannot abort a teardown.
  2. Move atlas.js off pool.pull if the cycle it appears to be dodging can be broken another way (a lazy import, or a small factory registry of its own).
  3. Decide the long-term shape of name-keyed instantiation for Tiled. It cannot simply be deleted — a .tmx names its classes as strings — but it does not have to be the same object as the recycling pool. Splitting "registry of classes by name" from "pool of recycled instances" would let the pooling half move to createPool and leave a much smaller, clearer registry behind.
  4. Only then consider deprecating the me.pool surface itself.

Items 1 and 2 are self-contained. Item 3 is a design decision that should land before 21.0.0 if me.pool is ever to be retired.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions