Extract _createRenderBuffer to deduplicate renderbuffer creation in GLRenderTarget#2897
Conversation
…over-abstraction Extract _createRenderBuffer and _checkFrameBufferStatus as static methods to eliminate duplicate renderbuffer creation logic, while keeping MSAA operations inline in GLRenderTarget. This avoids the unnecessary MSAAManager class abstraction that adds indirection without real benefit for a 1:1 internal relationship. Also fixes a leading-space typo in the FRAMEBUFFER_INCOMPLETE_DIMENSIONS error message. https://claude.ai/code/session_019PKH81Uz3Xv7R24kC48woN
WalkthroughCentralized renderbuffer creation by adding a private helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Augment PR SummarySummary: Refactors Changes:
Tests:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| internalFormat: GLenum, | ||
| attachment: GLenum | ||
| ): WebGLRenderbuffer { | ||
| const renderBuffer = gl.createRenderbuffer(); |
There was a problem hiding this comment.
gl.createRenderbuffer() can return null (e.g., resource exhaustion), and the subsequent bindRenderbuffer/renderbufferStorage* calls would then operate on an unbound renderbuffer and likely fail silently. Consider guarding this and throwing a clear error so failures are easier to diagnose.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rhi-webgl/src/GLRenderTarget.ts (1)
267-321:⚠️ Potential issue | 🟡 MinorAdd missing framebuffer status check to
_bindMainFBO.When
_checkFrameBufferStatuswas extracted as a static method in the recent refactor, it was only added to_bindMSAAFBO(line 361) but not to_bindMainFBO. Without this check, a non-MSAA framebuffer with format mismatches or other incompleteness issues will silently render incorrectly instead of failing fast. Add the check before unbinding:gl.bindFramebuffer(gl.FRAMEBUFFER, this._frameBuffer); // ... existing main FBO setup ... + GLRenderTarget._checkFrameBufferStatus(gl); gl.bindFramebuffer(gl.FRAMEBUFFER, null); gl.bindRenderbuffer(gl.RENDERBUFFER, null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rhi-webgl/src/GLRenderTarget.ts` around lines 267 - 321, The method _bindMainFBO is missing the framebuffer completeness check: call the same static helper used in _bindMSAAFBO — GLRenderTarget._checkFrameBufferStatus — after you've finished attaching color/depth targets and before unbinding the FBO; pass this._frameBuffer and gl (and handle a non-zero/non-OK status the same way _bindMSAAFBO does, e.g., throw or log and abort) so incomplete/non-matching framebuffers fail fast.
🧹 Nitpick comments (3)
tests/src/rhi-webgl/GLRenderTarget.test.ts (3)
139-152: Mock of_checkFrameBufferStatusappears unnecessary here.The test expects the constructor to throw during validation (mismatched color texture sizes), which happens before
_bindMainFBO/_bindMSAAFBOare reached. The mock should never be invoked. While it's harmless, removing it would make the test's intent clearer — it validates constructor-level checks, not FBO binding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/rhi-webgl/GLRenderTarget.test.ts` around lines 139 - 152, The test is mocking GLRenderTarget._checkFrameBufferStatus unnecessarily; remove the vi.spyOn(...) mock and the corresponding mockRestore() in the finally block so the test solely asserts that the RenderTarget constructor (new RenderTarget(...)) throws for mismatched color texture sizes; keep the creation of colorTexture1/colorTexture2 and the expect(...).toThrow(...) assertion unchanged and do not introduce any other mocks like _bindMainFBO or _bindMSAAFBO.
10-15: Consider addingafterAllto clean up the engine.The engine created in
beforeAllis never destroyed. While unlikely to cause issues in a test runner, explicitly callingengine.destroy()in anafterAllblock would prevent resource leaks across test suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/rhi-webgl/GLRenderTarget.test.ts` around lines 10 - 15, The test creates a WebGLEngine in beforeAll but never destroys it; add an afterAll block that calls engine.destroy() (and optionally nulls references like engine and gl) to release WebGL resources—locate the beforeAll that uses WebGLEngine.create and add afterAll { if (engine) engine.destroy(); } to clean up after tests.
72-82: Conditional test body will silently pass ifFRAMEBUFFER_INCOMPLETE_MULTISAMPLEis absent.When the test environment provides a WebGL1 context (no
FRAMEBUFFER_INCOMPLETE_MULTISAMPLEproperty), the entire test body is skipped and vitest reports it as passed, hiding the lack of coverage. Consider usingit.skipIforit.runIfto make the skip explicit in test output:- it("should handle WebGL2 multisample error if available", () => { - if ("FRAMEBUFFER_INCOMPLETE_MULTISAMPLE" in gl) { + it.runIf("FRAMEBUFFER_INCOMPLETE_MULTISAMPLE" in gl)( + "should handle WebGL2 multisample error if available", + () => { vi.spyOn(gl, "checkFramebufferStatus").mockReturnValue( (gl as WebGL2RenderingContext).FRAMEBUFFER_INCOMPLETE_MULTISAMPLE ); expect(() => { GLRenderTarget._checkFrameBufferStatus(gl); }).toThrow("The values of gl.RENDERBUFFER_SAMPLES are different"); - } - }); + } + );Note:
glmust be initialized before thedescribeblock forit.runIfto evaluate correctly at suite definition time. Sinceglis set inbeforeAll, it will beundefinedwhenit.runIfevaluates, so the currentif-guard approach may actually be the pragmatic choice here — just add a comment explaining why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/rhi-webgl/GLRenderTarget.test.ts` around lines 72 - 82, The test conditionally guards the body with "if ('FRAMEBUFFER_INCOMPLETE_MULTISAMPLE' in gl)" which silently passes the spec when a WebGL1 context is used; update the test so the skip is explicit or documented: either replace the runtime guard with a conditional test helper (e.g., it.runIf or it.skipIf) that checks the presence of FRAMEBUFFER_INCOMPLETE_MULTISAMPLE on gl at suite-definition time, or keep the runtime guard but add a clear comment explaining that gl is initialized in beforeAll so the runtime guard is required; ensure you reference the GLRenderTarget._checkFrameBufferStatus call and the FRAMEBUFFER_INCOMPLETE_MULTISAMPLE constant in your comment or conditional so future readers/tests know why the test may be skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rhi-webgl/src/GLRenderTarget.ts`:
- Around line 18-43: _gl.createRenderbuffer() can return null, so
_createRenderBuffer currently promises a WebGLRenderbuffer but may return null
and cause later NPEs; modify static _createRenderBuffer to check the result of
gl.createRenderbuffer() and if it is null throw a descriptive Error (e.g.,
"Failed to create WebGLRenderbuffer in _createRenderBuffer") before any
bind/storage calls so the function can safely keep returning WebGLRenderbuffer;
update any related logic in callers only if you prefer returning null instead of
throwing, but the preferred fix is to add the null-check+throw inside
_createRenderBuffer.
- Around line 338-346: The linter error is caused by the inline `@ts-ignore`
comment on the same line as the expression; move the `/** `@ts-ignore` */` onto
its own line immediately above the expression that reads the internal format so
the annotated expression is on the next line. In GLRenderTarget (around where
internalFormat is assigned) split the comment and the casted access into two
lines, leaving the rest of the code using internalFormat and the call to
GLRenderTarget._createRenderBuffer(_MSAAColorRenderBuffers) unchanged.
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts`:
- Around line 130-137: The test's expected error string is incorrect; update the
assertion in GLRenderTarget.test.ts so the expect(...).toThrow matches the
actual error thrown by RenderTarget (the RenderTarget constructor in
GLRenderTarget.ts when given TextureFormat.R32G32B32A32), e.g. change the
toThrow argument to the message produced by RenderTarget like "TextureFormat is
not supported:R32G32B32A32 in RenderTarget" so the test matches the real thrown
error.
---
Outside diff comments:
In `@packages/rhi-webgl/src/GLRenderTarget.ts`:
- Around line 267-321: The method _bindMainFBO is missing the framebuffer
completeness check: call the same static helper used in _bindMSAAFBO —
GLRenderTarget._checkFrameBufferStatus — after you've finished attaching
color/depth targets and before unbinding the FBO; pass this._frameBuffer and gl
(and handle a non-zero/non-OK status the same way _bindMSAAFBO does, e.g., throw
or log and abort) so incomplete/non-matching framebuffers fail fast.
---
Nitpick comments:
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts`:
- Around line 139-152: The test is mocking
GLRenderTarget._checkFrameBufferStatus unnecessarily; remove the vi.spyOn(...)
mock and the corresponding mockRestore() in the finally block so the test solely
asserts that the RenderTarget constructor (new RenderTarget(...)) throws for
mismatched color texture sizes; keep the creation of colorTexture1/colorTexture2
and the expect(...).toThrow(...) assertion unchanged and do not introduce any
other mocks like _bindMainFBO or _bindMSAAFBO.
- Around line 10-15: The test creates a WebGLEngine in beforeAll but never
destroys it; add an afterAll block that calls engine.destroy() (and optionally
nulls references like engine and gl) to release WebGL resources—locate the
beforeAll that uses WebGLEngine.create and add afterAll { if (engine)
engine.destroy(); } to clean up after tests.
- Around line 72-82: The test conditionally guards the body with "if
('FRAMEBUFFER_INCOMPLETE_MULTISAMPLE' in gl)" which silently passes the spec
when a WebGL1 context is used; update the test so the skip is explicit or
documented: either replace the runtime guard with a conditional test helper
(e.g., it.runIf or it.skipIf) that checks the presence of
FRAMEBUFFER_INCOMPLETE_MULTISAMPLE on gl at suite-definition time, or keep the
runtime guard but add a clear comment explaining that gl is initialized in
beforeAll so the runtime guard is required; ensure you reference the
GLRenderTarget._checkFrameBufferStatus call and the
FRAMEBUFFER_INCOMPLETE_MULTISAMPLE constant in your comment or conditional so
future readers/tests know why the test may be skipped.
| /** | ||
| * Create a renderbuffer, bindRenderbuffer, bindStorage and bindFramebuffer attachment. | ||
| * Automatically selects MSAA or regular storage based on target.antiAliasing. | ||
| * @internal | ||
| */ | ||
| static _createRenderBuffer( | ||
| gl: WebGLRenderingContext & WebGL2RenderingContext, | ||
| target: RenderTarget, | ||
| internalFormat: GLenum, | ||
| attachment: GLenum | ||
| ): WebGLRenderbuffer { | ||
| const renderBuffer = gl.createRenderbuffer(); | ||
| const { width, height, antiAliasing } = target; | ||
|
|
||
| gl.bindRenderbuffer(gl.RENDERBUFFER, renderBuffer); | ||
|
|
||
| if (antiAliasing > 1) { | ||
| gl.renderbufferStorageMultisample(gl.RENDERBUFFER, antiAliasing, internalFormat, width, height); | ||
| } else { | ||
| gl.renderbufferStorage(gl.RENDERBUFFER, internalFormat, width, height); | ||
| } | ||
|
|
||
| gl.framebufferRenderbuffer(gl.FRAMEBUFFER, attachment, gl.RENDERBUFFER, renderBuffer); | ||
|
|
||
| return renderBuffer; | ||
| } |
There was a problem hiding this comment.
createRenderbuffer() can return null — return type is misleading and callers may NPE.
gl.createRenderbuffer() returns WebGLRenderbuffer | null (null when context is lost or allocation fails). The method's return type promises WebGLRenderbuffer, and the returned value is later stored and used without null checks (e.g., line 315, 341, 358). If creation fails, subsequent gl.bindRenderbuffer(gl.RENDERBUFFER, null) silently unbinds, renderbufferStorage becomes a no-op or error, and the null render buffer is stored — causing hard-to-debug failures later.
Consider either guarding with an early throw or updating the return type:
🛡️ Proposed guard
static _createRenderBuffer(
gl: WebGLRenderingContext & WebGL2RenderingContext,
target: RenderTarget,
internalFormat: GLenum,
attachment: GLenum
): WebGLRenderbuffer {
const renderBuffer = gl.createRenderbuffer();
+ if (!renderBuffer) {
+ throw new Error("Failed to create renderbuffer");
+ }
const { width, height, antiAliasing } = target;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rhi-webgl/src/GLRenderTarget.ts` around lines 18 - 43,
_gl.createRenderbuffer() can return null, so _createRenderBuffer currently
promises a WebGLRenderbuffer but may return null and cause later NPEs; modify
static _createRenderBuffer to check the result of gl.createRenderbuffer() and if
it is null throw a descriptive Error (e.g., "Failed to create WebGLRenderbuffer
in _createRenderBuffer") before any bind/storage calls so the function can
safely keep returning WebGLRenderbuffer; update any related logic in callers
only if you prefer returning null instead of throwing, but the preferred fix is
to add the null-check+throw inside _createRenderBuffer.
| describe("validation", () => { | ||
| it("should reject unsupported texture format", () => { | ||
| const colorTexture = new Texture2D(engine, 512, 512); | ||
|
|
||
| expect(() => { | ||
| new RenderTarget(engine, 512, 512, colorTexture, TextureFormat.R32G32B32A32, 1); | ||
| }).toThrow("this TextureFormat is not supported"); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for both error message patterns
rg -n "this TextureFormat" --type=ts -A 2 -B 2
echo "---"
rg -n "TextureFormat is not supported" --type=ts -A 2 -B 2Repository: galacean/engine
Length of output: 3168
Error message in test does not match implementation — test will fail.
The test expects .toThrow("this TextureFormat is not supported"), but when R32G32B32A32 is passed as an unsupported depth format, GLRenderTarget.ts (line 117) throws "TextureFormat is not supported:R32G32B32A32 in RenderTarget". Vitest's substring matching will not find "this TextureFormat is not supported" in that message.
Update the test expectation to match the actual error thrown at line 117 of GLRenderTarget.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts` around lines 130 - 137, The
test's expected error string is incorrect; update the assertion in
GLRenderTarget.test.ts so the expect(...).toThrow matches the actual error
thrown by RenderTarget (the RenderTarget constructor in GLRenderTarget.ts when
given TextureFormat.R32G32B32A32), e.g. change the toThrow argument to the
message produced by RenderTarget like "TextureFormat is not
supported:R32G32B32A32 in RenderTarget" so the test matches the real thrown
error.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2897 +/- ##
==========================================
- Coverage 79.11% 78.38% -0.74%
==========================================
Files 876 870 -6
Lines 96096 95035 -1061
Branches 9702 9487 -215
==========================================
- Hits 76028 74494 -1534
- Misses 19907 20381 +474
+ Partials 161 160 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors GLRenderTarget by extracting reusable framebuffer/renderbuffer helper logic into static utility methods, and adds a dedicated test suite to validate framebuffer status error handling and MSAA/non-MSAA render target flows.
Changes:
- Extracted renderbuffer creation/attachment into
GLRenderTarget._createRenderBuffer. - Extracted framebuffer completeness checking into
GLRenderTarget._checkFrameBufferStatusand removed the instance method variant. - Added
GLRenderTargetunit tests covering framebuffer status errors, MSAA render target creation, and validation scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/src/rhi-webgl/GLRenderTarget.test.ts | Adds tests for _checkFrameBufferStatus and MSAA/non-MSAA RenderTarget creation/validation. |
| packages/rhi-webgl/src/GLRenderTarget.ts | Refactors duplicated renderbuffer/FBO status logic into static helpers and updates MSAA setup to use them. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const renderBuffer = gl.createRenderbuffer(); | ||
| const { width, height, antiAliasing } = target; | ||
|
|
||
| gl.bindRenderbuffer(gl.RENDERBUFFER, renderBuffer); | ||
|
|
There was a problem hiding this comment.
_createRenderBuffer assumes gl.createRenderbuffer() always returns a non-null renderbuffer, but the WebGL API can return null (e.g., context lost / OOM). Since the result is immediately bound/used and the function return type is WebGLRenderbuffer, this can lead to hard-to-diagnose GL errors. Consider explicitly checking for null and throwing a descriptive error (or using Logger), and update the return type/handling accordingly.
| import { GLRenderTarget } from "@galacean/engine-rhi-webgl/src/GLRenderTarget"; | ||
| import { describe, beforeAll, beforeEach, expect, it, vi, afterEach } from "vitest"; | ||
|
|
||
| describe("GLRenderTarget", () => { | ||
| let engine: WebGLEngine; | ||
| let gl: WebGLRenderingContext | WebGL2RenderingContext; | ||
|
|
||
| beforeAll(async () => { | ||
| const canvas = document.createElement("canvas"); | ||
| engine = await WebGLEngine.create({ canvas }); | ||
| // @ts-ignore | ||
| gl = engine._hardwareRenderer.gl; |
There was a problem hiding this comment.
This test imports GLRenderTarget via @galacean/engine-rhi-webgl/src/..., but the package only ships dist/**/* and types/**/* in files, so this deep import won’t work when running tests against built/published artifacts. Prefer testing via the public API (e.g., through RenderTarget and renderTarget._platformRenderTarget) or expose the needed hooks via an exported entry intended for tests.
| import { GLRenderTarget } from "@galacean/engine-rhi-webgl/src/GLRenderTarget"; | |
| import { describe, beforeAll, beforeEach, expect, it, vi, afterEach } from "vitest"; | |
| describe("GLRenderTarget", () => { | |
| let engine: WebGLEngine; | |
| let gl: WebGLRenderingContext | WebGL2RenderingContext; | |
| beforeAll(async () => { | |
| const canvas = document.createElement("canvas"); | |
| engine = await WebGLEngine.create({ canvas }); | |
| // @ts-ignore | |
| gl = engine._hardwareRenderer.gl; | |
| import { describe, beforeAll, beforeEach, expect, it, vi, afterEach } from "vitest"; | |
| describe("GLRenderTarget", () => { | |
| let engine: WebGLEngine; | |
| let gl: WebGLRenderingContext | WebGL2RenderingContext; | |
| let GLRenderTarget: any; | |
| beforeAll(async () => { | |
| const canvas = document.createElement("canvas"); | |
| engine = await WebGLEngine.create({ canvas }); | |
| // @ts-ignore | |
| gl = engine._hardwareRenderer.gl; | |
| const tempTexture = new Texture2D(engine, 1, 1); | |
| const tempRenderTarget = new RenderTarget(engine, 1, 1, tempTexture, TextureFormat.Depth16, 1); | |
| // @ts-ignore | |
| GLRenderTarget = (tempRenderTarget as any)._platformRenderTarget.constructor; |
| import { RenderTarget, Texture2D, TextureFormat } from "@galacean/engine-core"; | ||
| import { WebGLEngine } from "@galacean/engine-rhi-webgl"; | ||
| import { GLRenderTarget } from "@galacean/engine-rhi-webgl/src/GLRenderTarget"; | ||
| import { describe, beforeAll, beforeEach, expect, it, vi, afterEach } from "vitest"; |
There was a problem hiding this comment.
Unused import: beforeEach is imported from vitest but never used in this file. Consider removing it to keep the test file tidy.
| import { describe, beforeAll, beforeEach, expect, it, vi, afterEach } from "vitest"; | |
| import { describe, beforeAll, expect, it, vi, afterEach } from "vitest"; |
Match the project convention where private/internal methods have no multi-line JSDoc comments. https://claude.ai/code/session_019PKH81Uz3Xv7R24kC48woN
Align with project convention: internal methods use private access modifier instead of @internal JSDoc. https://claude.ai/code/session_019PKH81Uz3Xv7R24kC48woN
…thod Extract a private instance method _createRenderBuffer to deduplicate renderbuffer creation logic across _bindMainFBO and _bindMSAAFBO. Revert _checkFrameBuffer back to original instance method — no need to change it for testability. Tests only cover public behavior. https://claude.ai/code/session_019PKH81Uz3Xv7R24kC48woN
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/rhi-webgl/src/GLRenderTarget.ts (2)
280-281:⚠️ Potential issue | 🟡 MinorFix Prettier failure by moving
@ts-ignoreto its own line.CI is currently failing on Line 280 due to formatting.
🔧 Suggested fix
- const internalFormat = /** `@ts-ignore` */ - (this._target.getColorTexture(i)._platformTexture as GLTexture)._formatDetail.internalFormat; + /** `@ts-ignore` */ + const internalFormat = (this._target.getColorTexture(i)._platformTexture as GLTexture)._formatDetail + .internalFormat;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rhi-webgl/src/GLRenderTarget.ts` around lines 280 - 281, Prettier fails because the inline TypeScript ignore is on the same line as an expression; update the GLRenderTarget code where internalFormat is assigned (the line referencing this._target.getColorTexture(i)._platformTexture as GLTexture)._formatDetail.internalFormat) by moving the /** `@ts-ignore` */ to its own preceding line before the const internalFormat declaration (or immediately above the cast expression), ensuring the `@ts-ignore` is a standalone comment line while leaving the cast and access to _formatDetail.internalFormat unchanged.
303-319:⚠️ Potential issue | 🟠 MajorGuard
createRenderbuffer()null before binding/returning.Line 306 can return
null(e.g., context lost). Returning it as non-nullWebGLRenderbufferis unsafe and can fail later at bind/storage/attach.🛡️ Suggested fix
private _createRenderBuffer(internalFormat: GLenum, attachment: GLenum): WebGLRenderbuffer { const gl = this._gl; const { width, height, antiAliasing } = this._target; const renderBuffer = gl.createRenderbuffer(); + if (!renderBuffer) { + throw new Error("Failed to create WebGLRenderbuffer in GLRenderTarget._createRenderBuffer"); + } gl.bindRenderbuffer(gl.RENDERBUFFER, renderBuffer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rhi-webgl/src/GLRenderTarget.ts` around lines 303 - 319, The _createRenderBuffer function may receive null from gl.createRenderbuffer(), so guard the returned value before using it: in GLRenderTarget._createRenderBuffer check that renderBuffer !== null right after calling gl.createRenderbuffer(), throw or return a safe fallback (e.g., throw a descriptive Error mentioning internalFormat/attachment/target size) instead of proceeding to gl.bindRenderbuffer/gl.renderbufferStorage/gl.framebufferRenderbuffer and avoid returning null as a WebGLRenderbuffer; ensure the function's callers (and signature) handle this error path appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/rhi-webgl/src/GLRenderTarget.ts`:
- Around line 280-281: Prettier fails because the inline TypeScript ignore is on
the same line as an expression; update the GLRenderTarget code where
internalFormat is assigned (the line referencing
this._target.getColorTexture(i)._platformTexture as
GLTexture)._formatDetail.internalFormat) by moving the /** `@ts-ignore` */ to its
own preceding line before the const internalFormat declaration (or immediately
above the cast expression), ensuring the `@ts-ignore` is a standalone comment line
while leaving the cast and access to _formatDetail.internalFormat unchanged.
- Around line 303-319: The _createRenderBuffer function may receive null from
gl.createRenderbuffer(), so guard the returned value before using it: in
GLRenderTarget._createRenderBuffer check that renderBuffer !== null right after
calling gl.createRenderbuffer(), throw or return a safe fallback (e.g., throw a
descriptive Error mentioning internalFormat/attachment/target size) instead of
proceeding to
gl.bindRenderbuffer/gl.renderbufferStorage/gl.framebufferRenderbuffer and avoid
returning null as a WebGLRenderbuffer; ensure the function's callers (and
signature) handle this error path appropriately.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rhi-webgl/src/GLRenderTarget.tstests/src/rhi-webgl/GLRenderTarget.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/rhi-webgl/GLRenderTarget.test.ts
Remove 3 test cases (unsupported format, mismatched sizes, MSAA downgrade) that test pre-existing constructor validation unrelated to the _createRenderBuffer refactoring. Keep only the two lifecycle tests that exercise the refactored code paths. https://claude.ai/code/session_019PKH81Uz3Xv7R24kC48woN
GuoLei1990
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rhi-webgl/src/GLRenderTarget.ts (1)
304-320:⚠️ Potential issue | 🟡 MinorGuard against
nullfromgl.createRenderbuffer()before binding/attaching.At Line 307,
createRenderbuffer()may returnnull; Lines 309 and 317 then proceed with bind/attach while the method still returns a non-nullableWebGLRenderbuffer. This can hide context-loss/allocation failures and store invalid handles.🔧 Proposed fix
private _createRenderBuffer(internalFormat: GLenum, attachment: GLenum): WebGLRenderbuffer { const gl = this._gl; const { width, height, antiAliasing } = this._target; const renderBuffer = gl.createRenderbuffer(); + if (!renderBuffer) { + throw new Error("Failed to create WebGLRenderbuffer in GLRenderTarget._createRenderBuffer."); + } gl.bindRenderbuffer(gl.RENDERBUFFER, renderBuffer);In WebGL (and TypeScript lib.dom.d.ts), what is the return type of WebGLRenderingContext.createRenderbuffer(), and can it be null?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rhi-webgl/src/GLRenderTarget.ts` around lines 304 - 320, The _createRenderBuffer function currently assumes gl.createRenderbuffer() always returns a WebGLRenderbuffer; guard against a null return by checking the result of this._gl.createRenderbuffer() (store it in a local renderBuffer variable), and if it is null throw or return a sensible failure (e.g., throw an Error like "Failed to create renderbuffer" including context such as this._target and internalFormat) before calling bindRenderbuffer, renderbufferStorage[Multisample], or framebufferRenderbuffer; ensure the function signature/behavior still yields a non-nullable WebGLRenderbuffer by failing early on null so callers of _createRenderBuffer do not receive an invalid handle.
🧹 Nitpick comments (1)
tests/src/rhi-webgl/GLRenderTarget.test.ts (1)
14-44: Add one regression test for MSAA withdepth === null.Current lifecycle cases always pass
TextureFormat.Depth16, so the depth-absent MSAA branch (the one related to the leak fix) is still untested. Please add a case constructing an MSAARenderTargetwithout depth and assert lifecycle calls do not throw.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/rhi-webgl/GLRenderTarget.test.ts` around lines 14 - 44, Add a new test in the "lifecycle" suite that constructs an MSAA RenderTarget with no depth (pass null for the depth parameter instead of TextureFormat.Depth16) and verifies GLRenderTarget.activeRenderTarget(0) and GLRenderTarget.blitRenderTarget() do not throw; locate the existing tests that create Texture2D and RenderTarget and mirror their pattern but call new RenderTarget(engine, 512, 512, colorTexture, null, 4) and use the same `@ts-ignore` cast to get the _platformRenderTarget as GLRenderTarget, then call activeRenderTarget and blitRenderTarget inside an expect(...).not.toThrow() and finally call renderTarget.destroy().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts`:
- Around line 45-46: Remove the extra blank line immediately before the closing
`});` in GLRenderTarget.test.ts so the outer block's closing `});` is directly
adjacent to the preceding line; this fixes the Prettier/lint failure by ensuring
the final `});` (closing the outer test/describe block) has no preceding empty
line.
---
Duplicate comments:
In `@packages/rhi-webgl/src/GLRenderTarget.ts`:
- Around line 304-320: The _createRenderBuffer function currently assumes
gl.createRenderbuffer() always returns a WebGLRenderbuffer; guard against a null
return by checking the result of this._gl.createRenderbuffer() (store it in a
local renderBuffer variable), and if it is null throw or return a sensible
failure (e.g., throw an Error like "Failed to create renderbuffer" including
context such as this._target and internalFormat) before calling
bindRenderbuffer, renderbufferStorage[Multisample], or framebufferRenderbuffer;
ensure the function signature/behavior still yields a non-nullable
WebGLRenderbuffer by failing early on null so callers of _createRenderBuffer do
not receive an invalid handle.
---
Nitpick comments:
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts`:
- Around line 14-44: Add a new test in the "lifecycle" suite that constructs an
MSAA RenderTarget with no depth (pass null for the depth parameter instead of
TextureFormat.Depth16) and verifies GLRenderTarget.activeRenderTarget(0) and
GLRenderTarget.blitRenderTarget() do not throw; locate the existing tests that
create Texture2D and RenderTarget and mirror their pattern but call new
RenderTarget(engine, 512, 512, colorTexture, null, 4) and use the same
`@ts-ignore` cast to get the _platformRenderTarget as GLRenderTarget, then call
activeRenderTarget and blitRenderTarget inside an expect(...).not.toThrow() and
finally call renderTarget.destroy().
|
|
||
| }); |
There was a problem hiding this comment.
Fix lint: remove the extra blank line before the outer });.
Prettier is failing on this newline.
🔧 Proposed fix
});
-
});🧰 Tools
🪛 ESLint
[error] 45-46: Delete ⏎
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts` around lines 45 - 46, Remove the
extra blank line immediately before the closing `});` in GLRenderTarget.test.ts
so the outer block's closing `});` is directly adjacent to the preceding line;
this fixes the Prettier/lint failure by ensuring the final `});` (closing the
outer test/describe block) has no preceding empty line.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/src/rhi-webgl/GLRenderTarget.test.ts (1)
44-45:⚠️ Potential issue | 🟡 MinorRemove the extra blank line before the final
});(Prettier failure).This is still failing the formatter around Line 44-45.
🔧 Proposed fix
}); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/rhi-webgl/GLRenderTarget.test.ts` around lines 44 - 45, Remove the stray extra blank line before the final test block closure in GLRenderTarget.test.ts so Prettier passes: locate the test file's final closing token "});" (the end of the test suite) and delete the blank line immediately preceding that token so the file ends with the single line containing "});" with no extra empty line above it.
🧹 Nitpick comments (1)
tests/src/rhi-webgl/GLRenderTarget.test.ts (1)
18-19: Avoid@ts-ignorefor private access; use a narrow test-only cast helper.At Line 18 and Line 33,
@ts-ignoreis broader than needed and hides unrelated type errors. Prefer a typed helper/cast for just the accessed methods.♻️ Proposed refactor
import { describe, beforeAll, expect, it } from "vitest"; describe("GLRenderTarget", () => { let engine: WebGLEngine; + type GLRenderTargetLike = { + activeRenderTarget: (mipLevel: number) => void; + blitRenderTarget: () => void; + }; + + const getGLRenderTarget = (rt: RenderTarget): GLRenderTargetLike => + (rt as any)._platformRenderTarget as GLRenderTargetLike; beforeAll(async () => { const canvas = document.createElement("canvas"); engine = await WebGLEngine.create({ canvas }); }); @@ - // `@ts-ignore` - const glRenderTarget = renderTarget._platformRenderTarget as any; + const glRenderTarget = getGLRenderTarget(renderTarget); @@ - // `@ts-ignore` - const glRenderTarget = renderTarget._platformRenderTarget as any; + const glRenderTarget = getGLRenderTarget(renderTarget);Also applies to: 33-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/rhi-webgl/GLRenderTarget.test.ts` around lines 18 - 19, Replace the blanket "// `@ts-ignore`" by introducing a narrow test-only cast helper and use it for renderTarget._platformRenderTarget; specifically, remove the ignore and replace the assignment "const glRenderTarget = renderTarget._platformRenderTarget as any" with a call to a helper (e.g. "const glRenderTarget = asTestPlatformRenderTarget(renderTarget._platformRenderTarget)") where asTestPlatformRenderTarget is a small function in the test (or shared test utils) that casts to only the shape you need (methods/properties used in the test) instead of any; apply the same replacement for the second occurrence that currently uses "@ts-ignore" so both accesses avoid hiding unrelated type errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts`:
- Around line 8-11: Add an afterAll teardown that cleans up the engine created
in beforeAll: await the engine destruction (call await engine.destroy() — or
engine.dispose()/appropriate teardown method if destroy() is not present) and
null out the engine variable to prevent cross-test resource leakage; this
complements the existing beforeAll which calls WebGLEngine.create(...) and
references the engine variable.
---
Duplicate comments:
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts`:
- Around line 44-45: Remove the stray extra blank line before the final test
block closure in GLRenderTarget.test.ts so Prettier passes: locate the test
file's final closing token "});" (the end of the test suite) and delete the
blank line immediately preceding that token so the file ends with the single
line containing "});" with no extra empty line above it.
---
Nitpick comments:
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts`:
- Around line 18-19: Replace the blanket "// `@ts-ignore`" by introducing a narrow
test-only cast helper and use it for renderTarget._platformRenderTarget;
specifically, remove the ignore and replace the assignment "const glRenderTarget
= renderTarget._platformRenderTarget as any" with a call to a helper (e.g.
"const glRenderTarget =
asTestPlatformRenderTarget(renderTarget._platformRenderTarget)") where
asTestPlatformRenderTarget is a small function in the test (or shared test
utils) that casts to only the shape you need (methods/properties used in the
test) instead of any; apply the same replacement for the second occurrence that
currently uses "@ts-ignore" so both accesses avoid hiding unrelated type errors.
| beforeAll(async () => { | ||
| const canvas = document.createElement("canvas"); | ||
| engine = await WebGLEngine.create({ canvas }); | ||
| }); |
There was a problem hiding this comment.
Add teardown for WebGLEngine to avoid cross-test resource leakage.
At Line 8-11, engine creation is present but cleanup is missing. Add afterAll to destroy the engine/context after tests.
🧹 Proposed fix
-import { describe, beforeAll, expect, it } from "vitest";
+import { describe, beforeAll, afterAll, expect, it } from "vitest";
@@
beforeAll(async () => {
const canvas = document.createElement("canvas");
engine = await WebGLEngine.create({ canvas });
});
+
+ afterAll(() => {
+ engine.destroy();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeAll(async () => { | |
| const canvas = document.createElement("canvas"); | |
| engine = await WebGLEngine.create({ canvas }); | |
| }); | |
| import { describe, beforeAll, afterAll, expect, it } from "vitest"; | |
| beforeAll(async () => { | |
| const canvas = document.createElement("canvas"); | |
| engine = await WebGLEngine.create({ canvas }); | |
| }); | |
| afterAll(() => { | |
| engine.destroy(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/rhi-webgl/GLRenderTarget.test.ts` around lines 8 - 11, Add an
afterAll teardown that cleans up the engine created in beforeAll: await the
engine destruction (call await engine.destroy() — or
engine.dispose()/appropriate teardown method if destroy() is not present) and
null out the engine variable to prevent cross-test resource leakage; this
complements the existing beforeAll which calls WebGLEngine.create(...) and
references the engine variable.
cptbtptpbcptdtptp
changed the title
_createRenderBuffer to deduplicate renderbuffer creation in GLRenderTarget
Alternative to #2785.
Changes
Extract
_createRenderBuffer(internalFormat, attachment)to deduplicate thecreateRenderbuffer → bindRenderbuffer → renderbufferStorage → framebufferRenderbuffersequence across_bindMainFBOand_bindMSAAFBO.Bug fix
The original code created
MSAADepthRenderBufferunconditionally, but only used it inside theif (_depth !== null)guard — a resource leak when_depth === null. Now the entire creation is inside the guard.Why not #2785
MSAAManagerhas a 1:1 relationship withGLRenderTarget, duplicates host state, and still accesses internals via@ts-ignore. A private method achieves the same deduplication without extra class or state.Summary by CodeRabbit
Refactor
Tests