Remove depthTest setting, improve container sorting, clean up renderer#1377
Remove depthTest setting, improve container sorting, clean up renderer#1377
Conversation
GPU depth sorting is incompatible with 2D alpha blending — the depth buffer and painter's algorithm cannot coexist for transparent sprites. The "z-buffer" option never worked correctly and has been removed. Changes: - Remove depthTest application setting, DepthTest type, and z-buffer option from settings, defaults, header, and renderer initialization - Simplify WebGL renderer: depth test disabled by default, only enabled temporarily inside drawMesh() for 3D mesh rendering - Remove depth buffer from clear() (drawMesh clears it locally) - Move customShader property to base Renderer class so Renderable preDraw/postDraw no longer checks renderer type via renderer.gl - Container: cache sort comparator via sortOn getter/setter - Container: simplify sort comparators, remove legacy null guards - Remove depthTest: "z-buffer" from platformer example - Add tests for sort comparators and sortOn caching Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR removes the depthTest application setting (WebGL depth sorting for 2D) and simplifies WebGL renderer state handling, while also optimizing Container child sorting by caching the comparator and simplifying comparator implementations.
Changes:
- Remove
depthTestfrom application settings/defaults and WebGL renderer initialization/clearing behavior. - Optimize
Containersorting via cached comparator (sortOngetter/setter) and simplified comparator functions. - Move
customShaderto the baseRendererto avoid renderer-type checks inRenderable.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/melonjs/tests/container.spec.js | Adds unit tests for container comparators and sortOn comparator caching. |
| packages/melonjs/src/video/webgl/webgl_renderer.js | Disables depth testing for 2D by default; updates clear behavior; simplifies depth enable/restore in drawMesh. |
| packages/melonjs/src/video/renderer.js | Declares customShader on the base renderer. |
| packages/melonjs/src/renderable/renderable.js | Removes renderer-type guard when setting/resetting customShader. |
| packages/melonjs/src/renderable/container.js | Introduces cached comparator via sortOn accessor; simplifies sort comparators. |
| packages/melonjs/src/application/settings.ts | Removes DepthTest type and depthTest from ApplicationSettings. |
| packages/melonjs/src/application/header.ts | Removes depth test info from the console header. |
| packages/melonjs/src/application/defaultApplicationSettings.ts | Removes the depthTest default setting. |
| packages/melonjs/src/application/application.ts | Removes depthTest normalization and related world.autoSort toggling logic. |
| packages/melonjs/CHANGELOG.md | Documents breaking removal of depthTest and related renderer/container changes. |
| packages/examples/src/examples/platformer/createGame.ts | Removes depthTest: "z-buffer" from the platformer example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Throw an error if sortOn is set to an unsupported value (only "x", "y", "z" are valid). Prevents undefined comparator from falling back to Array.sort's default lexicographic ordering. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tweens use event-based lifecycle since #1365 and are no longer added as container children. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- addChild() and addChildAt() now throw if child is not a Renderable - Remove redundant isRenderable checks throughout Container (update, draw, updateBounds) since addChild guarantees all children are Renderable instances - Remove dead "non renderable" else branch in update loop - Remove Tween from addChild JSDoc (no longer added to containers) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Renderable/Container cleanup: - Remove redundant typeof checks for ancestor, updateBounds, pos, getAbsolutePosition (guaranteed by addChild instanceof guard) - Simplify mask, _absPos, _bounds, _tint cleanup to truthiness checks - Simplify parentApp and isFloating ancestor checks - Remove currentTransform undefined check in Container.reset() - Remove unused Color and Bounds imports Matrix optimization: - Reorder isIdentity() comparisons in Matrix2d and Matrix3d to check translation components first (most commonly non-zero), enabling early exit for non-identity matrices in the hot path 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 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if (!this._parentApp && this.ancestor) { | ||
| // the `app` property is only defined in the world "root" container | ||
| this._parentApp = this.ancestor.getRootAncestor().app; |
There was a problem hiding this comment.
Renderable.parentApp now assumes this.ancestor.getRootAncestor() exists whenever this.ancestor is set. This is not always true: Entity assigns this.children[0].ancestor = this (see entity.js), so calling parentApp on an Entity's child renderable will throw at runtime. Please restore a safe guard (e.g., check for getRootAncestor before calling, or walk up .ancestor until reaching a Container/root that has .app).
| this._parentApp = this.ancestor.getRootAncestor().app; | |
| if (typeof this.ancestor.getRootAncestor === "function") { | |
| const rootAncestor = this.ancestor.getRootAncestor(); | |
| this._parentApp = rootAncestor && rootAncestor.app; | |
| } else { | |
| let ancestor = this.ancestor; | |
| while (ancestor) { | |
| if (ancestor.app) { | |
| this._parentApp = ancestor.app; | |
| break; | |
| } | |
| ancestor = ancestor.ancestor; | |
| } | |
| } |
| // allocated a GUID value (use child.id as based index if defined) | ||
| child.GUID = createGUID(child.id); | ||
| } | ||
| child.GUID = createGUID(child.id); |
There was a problem hiding this comment.
addChild() now always calls createGUID(child.id). Several renderables (e.g. Entity) default id to an empty string, and createGUID() does arithmetic on the index parameter (GUID_index += index), so passing a non-number can coerce GUID_index into a string and break GUID generation. Consider only passing child.id when it is a finite number, otherwise call createGUID() without an argument (or normalize with Number(child.id) and a fallback).
| child.GUID = createGUID(child.id); | |
| child.GUID = Number.isFinite(child.id) | |
| ? createGUID(child.id) | |
| : createGUID(); |
Summary
depthTestapplication setting — GPU depth sorting is incompatible with 2D alpha blending. The"z-buffer"option never worked correctly for 2D sprites (see Enable GPU depth sorting for 2D sprite rendering in WebGL #1370 for full analysis). Depth testing remains available internally for 3D mesh rendering only.sortOngetter/setter, simplify comparators by removing legacy null guardscustomShaderto baseRendererclass soRenderableno longer checks renderer typeChanges
Removed
DepthTesttype anddepthTestsetting fromApplicationSettingsdepthTestfromdefaultApplicationSettingsclear()(onlydrawMeshuses it, clears locally)depthTest: "z-buffer"from platformer exampleImproved
Container.sortOnis now a getter/setter that caches the comparator — avoids"_sort" + sortOn.toUpperCase()lookup on each sort_sortZ,_sortReverseZ,_sortX,_sortY— removed null guards, use||short-circuitCleaned up
customShadermoved to baseRendererclassRenderable.preDraw/postDrawno longer checkstypeof renderer.gl !== "undefined"drawMeshsimplified: always restores depth to disabled state (no conditional)Test plan
_sortZ,_sortReverseZ,_sortX,_sortY) andsortOncaching🤖 Generated with Claude Code