fix(clipping): post-merge Copilot review on #1435#1437
Merged
Conversation
Three small follow-ups that landed in Copilot review after the squash merge of #1435: - Bounds.addFrame: short-circuit the 4-corner walk when the matrix is identity. The renderer's clipRect/enableScissor unconditionally pass `currentTransform` and don't filter identity themselves, so they were running 4× `Matrix3d.apply` per scissor change for the common identity case. Existing matrix-passing callers (Renderable, Spine plugin) already gate on `isIdentity()` so they're unaffected. - examples/Clipping: the right-side wrapper was positioned at its visual center (`pos = (RIGHT_CENTER_X, RIGHT_CENTER_Y)`) with a child clip offset by `(-W/2, -H/2)`. Container's anchorPoint is always `(0, 0)`, so its bounds (and viewport-cull rect) didn't match what was actually drawn — could disappear near edges or on resize. Wrapper now sits at top-left with `pos = (RIGHT_X, RIGHT_Y)` and the inner clip at `(0, 0)`; the per-frame pulse scales around the visual center via `translate(cx, cy) → scale → translate(-cx, -cy)` on `currentTransform`. Bounds and visual now agree. - bounds.spec.ts: add a sentinel test that spies on `m.apply` and asserts the identity short-circuit avoids the corner walk. Guards against a future refactor that drops the `isIdentity()` check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up adjustments to the clipping work from #1435, focused on reducing overhead in the common “identity transform” scissor path, aligning the clipping gallery example’s bounds with what’s visually drawn, and adding a regression/sentinel test to prevent the optimization from being accidentally removed.
Changes:
- Add an identity fast path to
Bounds.addFrame()to avoid the 4-cornerm.applywalk for no-op transforms. - Update WebGL scissor code comments to reflect that
Bounds.addFrame()now provides the identity short-circuit used byclipRect/enableScissor. - Fix the clipping example’s right-side wrapper positioning so container bounds/culling match the rendered output, while keeping the pulse centered via translate→scale→translate-back on
currentTransform. - Add a sentinel unit test that verifies the identity fast path avoids calling
Matrix3d.apply.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/melonjs/src/physics/bounds.ts | Adds an identity/no-transform early-return path in addFrame() to avoid unnecessary per-corner transform work. |
| packages/melonjs/src/video/webgl/webgl_renderer.js | Updates documentation/comments to reflect that scissor AABB derivation benefits from Bounds.addFrame’s identity short-circuit. |
| packages/melonjs/tests/bounds.spec.ts | Adds a regression/sentinel test that asserts addFrame doesn’t call apply for an identity matrix. |
| packages/examples/src/examples/clipping/ExampleClipping.tsx | Adjusts example layout/transform composition so bounds and culling align with what is drawn while preserving the pulsing-center behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three small follow-ups to PR #1435 (which landed by squash-merge before these review comments were posted):
Bounds.addFramenow short-circuits the 4-corner walk when the matrix is identity. The renderer'sclipRect/enableScissorunconditionally passcurrentTransformand don't filter identity themselves, so they were running 4×Matrix3d.applyper scissor change in the common identity case. Existing matrix-passing callers (Renderable.updateBounds, the spine plugin) already gate onisIdentity()so they're unaffected.pos = (RIGHT_CENTER_X, RIGHT_CENTER_Y)) with a child clip offset by(-W/2, -H/2). Container'sanchorPointis always(0, 0), so its bounds (and viewport-cull rect) didn't match what was actually drawn — could disappear near canvas edges or on resize. Wrapper now sits at top-left and the per-frame pulse scales around the visual center viatranslate(cx, cy) → scale → translate(-cx, -cy)oncurrentTransform. Bounds and visual now agree.bounds.spec.tsadds a sentinel test that spies onm.applyand asserts the identity short-circuit avoids the corner walk — guards against a future refactor that drops theisIdentity()check.Test plan
pnpm vitest run— 2933 tests pass.pnpm lint— 0 errors.Clippingexample renders identically to before; right-side pulse still breathes around its visual center.🤖 Generated with Claude Code