refactor(video): allocate uniform buffer binding points from the renderer - #1560
Merged
Conversation
…erer Binding points are a context-wide namespace: two uniform blocks sharing one silently overwrite each other, and the loser reads the winner's bytes as its own data. They were two literals in the lighting module (LIGHT2D_BINDING_POINT / LIGHT3D_BINDING_POINT), so a third block would have had to grep for a free integer and a collision would not have been caught. WebGLRenderer.reserveUniformBindingPoint() now hands them out, mirroring how texture units are claimed through TextureCache.reserveUnit(), and throws when the device's MAX_UNIFORM_BUFFER_BINDINGS is exhausted rather than binding out of range. Both lit batchers claim once and hold: init() re-runs on every context restore, and re-claiming each time would walk through the budget over a long session. The counter is initialised before the batchers are constructed — they are built in the same constructor, so a later assignment left the first claim undefined. Internal only, no behaviour change: the points handed out are 0 and 1, the same values the constants held. Point 0 stays occupied deliberately — an active uniform block with no buffer bound is INVALID_OPERATION at draw time rather than a read of zeroes, so leaving it free is not the safety net it looks like. Follow-up to #1552. Two other follow-ups from that review were investigated and rejected on measurement rather than implemented; see the PR for numbers. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JvxaXQRgQn5AoR8DNkv6Wg
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors WebGL lighting uniform-buffer binding point selection so binding points are centrally allocated by WebGLRenderer (a context-wide namespace), removing per-feature hard-coded literals while keeping runtime behavior unchanged.
Changes:
- Add
WebGLRenderer.reserveUniformBindingPoint()with exhaustion checking againstMAX_UNIFORM_BUFFER_BINDINGS. - Update
LitQuadBatcherandLitMeshBatcherto claim and retain a binding point once acrossinit()re-runs (context restore). - Remove
LIGHT2D_BINDING_POINT/LIGHT3D_BINDING_POINTconstants and add wiring tests validating allocator usage and binding point stability.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/melonjs/tests/lighting_block_wiring.spec.js | Adds coverage ensuring lit batchers’ UBO binding points come from the renderer allocator and persist across init() re-runs. |
| packages/melonjs/src/video/webgl/webgl_renderer.js | Introduces a renderer-owned binding point allocator with device-limit guarding. |
| packages/melonjs/src/video/webgl/lighting/constants.ts | Removes hard-coded uniform buffer binding point constants now superseded by renderer allocation. |
| packages/melonjs/src/video/webgl/batchers/lit_quad_batcher.js | Claims a binding point from the renderer once and reuses it on subsequent init() calls. |
| packages/melonjs/src/video/webgl/batchers/lit_mesh_batcher.js | Claims a binding point from the renderer once and reuses it on subsequent init() calls. |
💡 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.
Follow-up to #1552, from that PR's review. Internal only — no behaviour change.
What
Uniform buffer binding points were two literals in the lighting module (
LIGHT2D_BINDING_POINT,LIGHT3D_BINDING_POINT). They are a context-wide namespace, not a lighting concern: two blocks sharing a point silently overwrite each other, and the loser reads the winner's bytes as its own data. A third block would have had to grep for a free integer, and a collision would not have been caught.WebGLRenderer.reserveUniformBindingPoint()now hands them out — the same shape asTextureCache.reserveUnit()for texture units — and throws when the device'sMAX_UNIFORM_BUFFER_BINDINGSis exhausted rather than binding out of range.Both lit batchers claim once and hold it.
init()re-runs on every context restore, so re-claiming each time would walk through the budget over a long session; the test for that is mutation-verified (changing??=to=fails it).The points handed out are 0 and 1, exactly what the constants held.
Two things worth knowing:
undefined. Caught by a test, not by review.INVALID_OPERATIONat draw time, taking the whole program down. Tried it during the Lighting: uniform buffer objects for light data (raise MAX_LIGHTS, modernize the lit shaders to GLSL ES 3.00) #1552 review; it broke two adversarial tests.The two follow-ups I did NOT do
Both were investigated and rejected on measurement. Recording the numbers so the question doesn't get reopened from scratch.
Gate the per-draw light pack. Can't be done safely. The cheap gate — re-arm from
RENDER_TARGET_CHANGED, matching the depth-clear cadence — leaves a mid-frame light change stale, which is the bug #1552 fixed. The guard test (lighting_block_wiring.spec.js→ "picks up a light change between draws with no batcher switch") catches it.Collapse the SoA → AoS double pass in the packers. Measured on the lit mesh path, per draw:
~100 ns at the cap, against rewriting ~49 stride-indexed assertions in
lights.spec.jsthat currently catch real regressions. Its correctness argument — "two places must agree what light i means" — is already covered: the layout is pinned against both hand-written and driver-reported offsets.And the reason neither matters: GPU cost of the lit fragment shader, full-screen quad at 1280×720, ANGLE Metal on an Apple M4 Max —
Every light was given a screen-covering radius, so this is the worst case for brute force — and therefore the ceiling on what tiled light culling could ever recover: 0.11 ms/frame, 0.7% of a 60 fps budget. Not worth a light-list texture and a per-frame binning pass.
Caveats on those numbers: wall-clock around 60 draws with a
readPixelssync rather than the timer query (which was available),performance.now()clamped to 100 µs so ~±1% at 0.14 ms and ~±10% at 0.017 ms, and an M4 Max is near the top end — a low-end mobile GPU could be 10–30× slower, which would change the conclusion. Worth redoing on real hardware if mobile many-light scenes ever become a goal.🤖 Generated with Claude Code
https://claude.ai/code/session_01JvxaXQRgQn5AoR8DNkv6Wg