Skip to content

Remove game import from Container, default dimensions to Infinity#1348

Merged
obiot merged 6 commits intomasterfrom
refactor/container-no-game
Apr 4, 2026
Merged

Remove game import from Container, default dimensions to Infinity#1348
obiot merged 6 commits intomasterfrom
refactor/container-no-game

Conversation

@obiot
Copy link
Copy Markdown
Member

@obiot obiot commented Apr 2, 2026

Summary

  • Container constructor no longer imports or references the global game singleton
  • Default width/height changed from game.viewport lookup to Infinity (no clipping)
  • Same effective behavior since all Container creations (internal and examples) pass explicit dimensions
  • Updated JSDoc to document Infinity defaults

Test plan

  • All 1983 tests pass
  • New tests: default Infinity dimensions, position defaults, explicit dimensions, children with Infinity container, no-clip behavior

🤖 Generated with Claude Code

Container constructor no longer imports or references the global game
singleton. Default width/height changed from game.viewport dimensions
to Infinity (no clipping). This is the same effective behavior since
all actual Container creations pass explicit dimensions.

Updated JSDoc to document Infinity defaults.
Added tests: default dimensions, position defaults, explicit dimensions,
children with Infinity container, no-clip behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 2, 2026 23:47
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR decouples Container from the global game singleton by removing the game.viewport-based default sizing, and instead defaults container dimensions to Infinity to avoid implicit clipping when dimensions aren’t provided.

Changes:

  • Removed game import/usage from Container and simplified the constructor to pass through width/height directly.
  • Changed default Container width/height to Infinity and updated the constructor JSDoc accordingly.
  • Added tests covering default dimensions and some basic behaviors when dimensions are Infinity.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
packages/melonjs/src/renderable/container.js Removes dependency on game.viewport and defaults container dimensions to Infinity.
packages/melonjs/tests/container.spec.js Adds test cases for default Container dimensions and related expectations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Container: Infinity defaults, isFinite() guard in updateBounds, anchorPoint (0,0)
- ImageLayer: uses parentApp for viewport/renderer, deferred to onActivateEvent
- TMXTileMap: resolves viewport from container tree via getRootAncestor().app
- Input: pointer/pointerevent receive app via GAME_INIT instead of game singleton
- Stage: onDestroyEvent(app) now passes Application instance
- video.js: DOM event listeners moved to Application.init()
- video.renderer, video.init(), video.getParent(), device.onReady() deprecated
- Application: improved JSDoc, DOM event bridge for resize/orientation/scroll
- BitmapTextData: fix circular import (pool.ts <-> bitmaptextdata.ts)
- All me.game.* JSDoc references updated to app.*
- README hello world updated to use new Application() pattern
- Platformer and UI examples migrated to Application entry point
- New tests: container (Infinity/isFinite/clipping), ImageLayer (parentApp)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
obiot and others added 2 commits April 4, 2026 12:14
…string

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 568 to +572
const bounds = this.getBounds();

// call parent method
super.updateBounds(absolute);
if (this.isFinite()) {
// call parent method only when container has finite dimensions
super.updateBounds(absolute);
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Infinity-sized containers, updateBounds() skips super.updateBounds() but does not clear/reset the existing bounds before aggregating child bounds. Since non-finite bounds are initialized with max = Infinity, subsequent bounds.addBounds(childBounds) will keep the container bounds infinite and won’t become child-derived. Clear/reset bounds (or start from the first child bounds with clear=true) when !this.isFinite() and enableChildBoundsUpdate is enabled.

Copilot uses AI. Check for mistakes.
Comment on lines 124 to +129
// called when the layer is added to the game world or a container
onActivateEvent() {
// register to the viewport change notification
const viewport = this.parentApp.viewport;
// set the initial size to match the viewport
this.resize(viewport.width, viewport.height);
this.createPattern();
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImageLayer.onActivateEvent() dereferences this.parentApp immediately. Container.addChild() invokes onActivateEvent even when the container tree is not attached to a root/world container, so parentApp may be unavailable and this will throw. Add a guard (throw a clear error or defer initialization until attached) before using parentApp.viewport/renderer.

Copilot uses AI. Check for mistakes.
Comment on lines 149 to 152
if (pointerEventTarget === null || pointerEventTarget === undefined) {
// default pointer event target
pointerEventTarget = game.renderer.getCanvas();
pointerEventTarget = _app.renderer.getCanvas();
}
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enablePointerEvent() uses _app.renderer.getCanvas() as the default event target, but _app is only assigned via the GAME_INIT event. If bindPointer() / registerPointerEvent() is called before an Application is constructed, this will crash. Add a defensive check and throw a clear initialization error when _app is undefined.

Copilot uses AI. Check for mistakes.
Comment on lines +315 to +319
// bridge DOM events to the melonJS event system
globalThis.addEventListener("resize", (e) => {
emit(WINDOW_ONRESIZE, e);
});
globalThis.addEventListener("orientationchange", (e) => {
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor registers global resize/orientationchange/scroll listeners via globalThis.addEventListener(...), but destroy() does not remove them. Creating/destroying multiple Application instances will leak listeners and duplicate emitted events. Store handler references and unregister them in destroy().

Copilot uses AI. Check for mistakes.
Comment on lines 416 to 418
const app = container.getRootAncestor().app;

function _setBounds(width, height) {
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TMXTileMap.addTo() resolves const app = container.getRootAncestor().app; unconditionally. If container is not attached to a root/world container, getRootAncestor() can return undefined and this will throw even when setViewportBounds is false. Move app resolution inside the setViewportBounds === true branch and add a defensive check when no root/app is available.

Suggested change
const app = container.getRootAncestor().app;
function _setBounds(width, height) {
function _setBounds(width, height) {
const rootAncestor = container.getRootAncestor();
const app = rootAncestor && rootAncestor.app;
if (!app || !app.viewport) {
return;
}

Copilot uses AI. Check for mistakes.
obiot and others added 2 commits April 4, 2026 12:21
- Container: clear bounds before child aggregation for infinite containers
- ImageLayer: throw if parentApp unavailable in onActivateEvent
- Input: throw if Application not initialized when enabling pointer events
- Application: store DOM event handlers, remove them in destroy()
- TMXTileMap: move app resolution inside setViewportBounds branch

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 4, 2026 06:08
@obiot obiot merged commit 6472226 into master Apr 4, 2026
8 checks passed
@obiot obiot deleted the refactor/container-no-game branch April 4, 2026 06:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

globalThis.addEventListener("resize", this._onResize);
globalThis.addEventListener("orientationchange", this._onOrientationChange);
if (device.screenOrientation) {
globalThis.screen.orientation.onchange = this._onOrientationChange;
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Application.init() assigns screen.orientation.onchange directly and destroy() unconditionally clears it. This can clobber another Application instance’s handler (or any consumer handler) when multiple apps exist or when an app is destroyed out of order. Prefer screen.orientation.addEventListener('change', ...)/removeEventListener, or only clear onchange if it still equals this instance’s handler (restore previous handler otherwise).

Suggested change
globalThis.screen.orientation.onchange = this._onOrientationChange;
globalThis.screen.orientation.addEventListener(
"change",
this._onOrientationChange,
);

Copilot uses AI. Check for mistakes.
this._onOrientationChange!,
);
globalThis.removeEventListener("scroll", this._onScroll!);
if (device.screenOrientation) {
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroy() sets screen.orientation.onchange = null without checking whether another Application (or user code) has since installed a different handler. This can break orientation handling in multi-app scenarios. Only clear it if the current value matches this instance’s handler, or switch to add/removeEventListener for ScreenOrientation change events.

Suggested change
if (device.screenOrientation) {
if (
device.screenOrientation &&
globalThis.screen.orientation.onchange === this._onOrientationChange
) {

Copilot uses AI. Check for mistakes.
@@ -123,15 +119,23 @@ export default class ImageLayer extends Sprite {
this.repeatY = true;
break;
}
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repeat setter no longer triggers a resize / pattern rebuild. If repeat is changed at runtime after activation, width/height and _pattern can become inconsistent until a viewport resize happens. Consider updating size (this.resize(viewport.width, viewport.height)) and recreating the pattern when parentApp is available and the layer is active.

Suggested change
}
}
if (this.parentApp && this.parentApp.viewport) {
const viewport = this.parentApp.viewport;
this.resize(viewport.width, viewport.height);
this.createPattern();
}

Copilot uses AI. Check for mistakes.
Comment on lines +48 to +50
* In this case, anchorPoint is treated as (0, 0) since there is no meaningful
* center for an infinite area. Bounds are then derived entirely from children
* when {@link Container#enableChildBoundsUpdate} is enabled.
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc/comment says the container anchorPoint is “always” (0,0) / “treated as” (0,0), but nothing prevents users from changing anchorPoint after construction (which can reintroduce Infinity * anchor issues). Consider clarifying this as a default (not an invariant), or enforcing it (e.g. override setter / clamp to 0 when width/height are non-finite).

Suggested change
* In this case, anchorPoint is treated as (0, 0) since there is no meaningful
* center for an infinite area. Bounds are then derived entirely from children
* when {@link Container#enableChildBoundsUpdate} is enabled.
* In this case, the default anchorPoint should remain at (0, 0), since there is
* no meaningful center for an infinite area. Bounds are then derived entirely
* from children when {@link Container#enableChildBoundsUpdate} is enabled.

Copilot uses AI. Check for mistakes.
Comment on lines +169 to +175
const app = new Application(1218, 562, {
parent: "screen",
scale: "auto",
backgroundColor: "#202020",
});

// set a gray background color
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this example, backgroundColor passed to new Application(...) only affects the parent element CSS background (per ApplicationSettings docs), while app.world.backgroundColor.parseCSS(...) controls the rendered clear color. Having both (and the comment “set a gray background color”) is confusing—consider either removing the option or adjusting the comments to reflect the distinction.

Suggested change
const app = new Application(1218, 562, {
parent: "screen",
scale: "auto",
backgroundColor: "#202020",
});
// set a gray background color
// `backgroundColor` here sets the parent element CSS background
const app = new Application(1218, 562, {
parent: "screen",
scale: "auto",
backgroundColor: "#202020",
});
// set the rendered world clear color to gray

Copilot uses AI. Check for mistakes.
Comment on lines 28 to +33
/**
* Initialize the melonJS library.
* This is called automatically in two cases:
* - On DOMContentLoaded, unless {@link skipAutoInit} is set to true
* - By {@link Application.init} when creating a new game instance
*
* This is called automatically by the {@link Application} constructor.
* Multiple calls are safe — boot() is idempotent.
* @see {@link skipAutoInit}
* When using {@link Application} directly, calling boot() manually is not needed.
* @see {@link Application}
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated JSDoc for boot() says it’s “called automatically by the Application constructor”, but the entrypoint still auto-calls boot() on DOMContentLoaded when skipAutoInit is false (see src/index.ts). Consider updating the doc to reflect both auto-init paths (DOMContentLoaded + Application.init/constructor) so users relying on skipAutoInit aren’t misled.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants