Fix remaining framebuffer binding-cache bypasses (`_bind`/`_unBind` outside `Context.bindFramebuffer`)
- Dominant language
- JavaScript
- Stars
- 15.7k
- Forks
- 3.9k
- Avg merge
- 4d 6h
- Merged PRs (30d)
- 34
Description
### What happened?
This stems from https://github.com/CesiumGS/cesium/issues/13661 which was fixed in https://github.com/CesiumGS/cesium/pull/13662.
Please read up on that issue and PR to get full context first.
## Background
#13662 fixed a one-frame black flash caused by the `Framebuffer` constructor issuing `gl.bindFramebuffer` calls without updating `Context._currentFramebuffer`. `Context.bindFramebuffer` skips the GL call when the target matches its cache, so any code that changes the real GL binding behind the cache's back makes subsequent draws land on the wrong framebuffer until the next real rebind (or `endFrame`).
#13662 patched only the constructor. The same bypass pattern is still live in several places that can, in theory, run mid-frame:
- `Framebuffer.js` `status` getter binds `this`, unbinds to `null`, cache untouched. Read during updates by `OIT.js`, `ShadowMap.js`, and `FramebufferManager.js`
- `Texture.js` `loadFramebufferSource` (used by `Texture.fromFramebuffer`). Binds `source.framebuffer`, unbinds to `null`.
- `CubeMap.js` `copyFace` constructs + binds a scratch framebuffer.
- `TextureAtlas.js` does an atlas resize during billboard/label update. This follows the same construct + bind/unbind pattern.
Each is a variant of the #13662 bug: harmless until some caller sequence puts a cached bind before it and draws after it in the same frame.
## Proposed fix
Rather than patching each site with `context._currentFramebuffer = undefined` (repeating the cross-class reach noted in #13662), we could make `Context` the sole writer of framebuffer binding state. Arguably, it should probably be this way already...
1. Expose the binding through the context, e.g. `context.bindFramebuffer(framebuffer)` / a scoped helper like `context.withFramebuffer(fb, fn)` that saves and restores both the GL binding and the cache.
2. Convert the sites above (and the `Framebuffer` constructor) to use it; `_bind`/`_unBind` become private implementation details of `Context` or are removed.
3. Keep the #13662 spec and add equivalents: reading `status`, `Texture.fromFramebuffer`, `CubeMap.copyFace`, and an atlas resize must all leave `context._currentFramebuffer` consistent with the GL binding.
## Notes
- `Context.endFrame` already resets the cache each frame, which is why these have mostly gone unnoticed — the desync window is at most the remainder of one frame.
- Refs: #13662 (constructor fix + root-cause analysis).
### Reproduction steps
N/A - theoretical from code analysis. https://github.com/CesiumGS/cesium/issues/13661 is the one we actually observed, now fixed.
In theory, we could write some unit tests to prove these bypass flaws.
### Sandcastle example
_No response_
### Environment
Browser: Firefox / Chrome / all
CesiumJS Version: Latest (main / 1.144)
Operating System: macOS / all
### AI acknowledgment
- [x] I used AI to generate this issue report.
- [x] (If the above is checked) I have reviewed the AI-generated content before submitting.
Contributor guide
Research direction
Read issues #13661 and #13662 first for the framebuffer cache context, then inspect Context, Framebuffer.js, Texture.js, CubeMap.js, and TextureAtlas.js. Trace the listed _bind/_unBind paths and the existing #13662 spec. Done means the affected operations leave Context._currentFramebuffer consistent with the GL binding and equivalent coverage exists for status, Texture.fromFramebuffer, CubeMap.copyFace, and atlas resize.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- computer-graphics
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100