Refactor Tween to use event-based lifecycle instead of world container#1365
Refactor Tween to use event-based lifecycle instead of world container#1365
Conversation
#1347) - Tween no longer adds itself to game.world — uses TICK, STATE_PAUSE, STATE_RESUME, GAME_AFTER_UPDATE, GAME_RESET events instead - Removes game.world dependency and container-compat flags (isRenderable) - isPersistent controls whether GAME_RESET stops the tween - updateWhenPaused controls whether the tween pauses with the game - Removed dead code in setProperties (always-false typeof check) - setProperties calls _unsubscribe to prevent leaked listeners on pool reuse - Updated JSDoc for Tween, Easing, and Interpolation with cross-references Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactors Tween to be a logic-only object driven by the engine event system rather than participating in the scene graph via game.world, and updates related docs/tests to match the new pooling/event approach.
Changes:
Tweennow subscribes/unsubscribes to lifecycle events (TICK, pause/resume, reset, after-update) onstart()/stop()instead ofaddChild/removeChildNow.- Removes container-compat behavior (
isRenderable) and adds internal running/paused tracking for event-driven updates. - Updates pooling-related tests/imports and expands JSDoc for Tween/Easing/Interpolation; adds a changelog breaking-change note.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/melonjs/tests/pool.test.ts | Updates test import to use the system pool implementation. |
| packages/melonjs/tests/bitmapfontdata.spec.ts | Switches bitmap font test to use bitmapTextDataPool directly. |
| packages/melonjs/src/tweens/tween.ts | Converts Tween lifecycle from world-container updates to event-based subscription model; updates docs. |
| packages/melonjs/src/tweens/interpolation.ts | Adds JSDoc describing interpolation functions and usage with Tween. |
| packages/melonjs/src/tweens/easing.ts | Rewrites JSDoc to clarify easing families and Tween usage. |
| packages/melonjs/CHANGELOG.md | Documents the Tween lifecycle breaking change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… _startTime Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers constructor, to(), start/stop, update interpolation, callbacks, delay, repeat, yoyo, easing, chaining, setProperties reset, isPersistent, updateWhenPaused, static accessors, array interpolation, relative values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Duration 0 no longer produces Infinity/NaN in elapsed calculation - Non-numeric string properties (e.g. "world") are skipped instead of being corrupted to NaN by parseFloat - Added 13 edge case tests (60 total tween tests) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- start() now calls _unsubscribe() first to prevent listener leaks on restart - Added afterEach cleanup in tween and bitmapfontdata tests - Updated changelog to include GAME_AFTER_UPDATE in event list - Pool instances properly released in bitmapfontdata tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… fix - Stop all tweens created during tests to prevent listener leaks - to() options use !== undefined instead of truthy checks (allows duration:0, yoyo:false, delay:0) - start(time) syncs _tweenTimeTracker to provided time (fixes flaky tests with start(0)) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (this._isRunning) { | ||
| this._isRunning = false; | ||
| this._isPaused = false; | ||
| // eslint-disable-next-line @typescript-eslint/unbound-method | ||
| off(TICK, this._onTick, this); |
There was a problem hiding this comment.
Calling off(...) inside _unsubscribe() can run while the corresponding event is being emitted (e.g. _unsubscribe() is triggered from update() during a TICK handler, or via stop() during GAME_RESET). The current EventEmitter.emit() iterates the live listener array with for...of and removeListener() uses splice(), so removing the current listener during iteration can cause subsequent listeners to be skipped for that event.
To avoid missed TICK/GAME_RESET handlers when multiple tweens (or other listeners) are registered, defer the actual off(...) calls until after the current call stack (e.g. via utils/function.defer) or otherwise ensure listener removal doesn’t mutate the array being iterated (e.g. event emitter iterates over a copy).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _subscribe() { | ||
| if (!this._isRunning) { | ||
| this._isRunning = true; | ||
| // eslint-disable-next-line @typescript-eslint/unbound-method | ||
| on(TICK, this._onTick, this); | ||
| // eslint-disable-next-line @typescript-eslint/unbound-method |
There was a problem hiding this comment.
The new event-based lifecycle (TICK/pause/resume/reset subscriptions) is core to this refactor, but the new tests mostly exercise update() directly and don’t validate that event wiring behaves correctly (pause stops updates unless updateWhenPaused, GAME_RESET stops unless isPersistent, stop()/completion actually detaches listeners). Adding unit tests that use event.emit(...) to drive these events would help prevent regressions and confirm listener cleanup.
| emit<E extends keyof Events>(event: E, ...args: Parameters<Events[E]>) { | ||
| const listeners = this.eventListeners[event]; | ||
| if (listeners) { | ||
| for (const entry of listeners) { | ||
| // iterate over a copy so that removeListener during emit is safe | ||
| const snapshot = listeners.slice(); | ||
| for (const entry of snapshot) { | ||
| entry.fn.apply(entry.ctx, args); |
There was a problem hiding this comment.
emit() now allocates a new array (listeners.slice()) for every emission. Since hot-path events like TICK fire every frame, this can create avoidable allocations/GC pressure. Consider an alternative that keeps iteration safe without per-emit copying (e.g., index-based loop with a mutation-safe strategy, or only snapshotting when a mutation is detected).
| import { beforeEach, describe, expect, it } from "vitest"; | ||
| import { getPool } from "../src/pool.ts"; | ||
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | ||
| import { bitmapTextDataPool } from "../src/renderable/text/bitmaptextdata.ts"; |
There was a problem hiding this comment.
This test imports a source file with an explicit .ts extension, whereas other tests consistently import from ../src/... using .js (for ESM parity) or no extension. Using .ts here can be brittle depending on the test runner/module resolution settings; consider matching the existing convention (omit the extension or use the .js path if available).
| import { bitmapTextDataPool } from "../src/renderable/text/bitmaptextdata.ts"; | |
| import { bitmapTextDataPool } from "../src/renderable/text/bitmaptextdata.js"; |
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "$schema": "https://turbo.build/schema.json", | |||
| "$schema": "https://v2-9-5.turborepo.dev/schema.json", | |||
There was a problem hiding this comment.
The $schema URL was changed from the canonical https://turbo.build/schema.json to a versioned v2-9-5.turborepo.dev host. If that host is unavailable (offline/dev env, future deprecation), editor validation/autocomplete for turbo.json may break. Prefer the official schema URL unless there’s a specific tooling requirement to pin to a versioned schema (and document that rationale).
| "$schema": "https://v2-9-5.turborepo.dev/schema.json", | |
| "$schema": "https://turbo.build/schema.json", |
Closes #1347
Summary
game.world— subscribes to game loop events (TICK,STATE_PAUSE,STATE_RESUME,GAME_AFTER_UPDATE,GAME_RESET) onstart()and unsubscribes onstop()or completiongame.worlddependency — no moreaddChild/removeChildNowisRenderable)isPersistentcontrols whetherGAME_RESETauto-stops the tweenupdateWhenPausedcontrols whether the tween pauses with the game statesetProperties(always-falsetypeofcheck)setPropertiescalls_unsubscribe()to prevent leaked event listeners on pool reuseBreaking Changes
game.world.children— code that iterated the world container to find tweens will no longer see themisRenderableproperty removed from TweenTest plan
🤖 Generated with Claude Code