refactor: MSAA manager in GLRenderTarget #2785
Conversation
WalkthroughA new internal class, Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant GLRenderTarget
participant MSAAManager
participant WebGL
App->>GLRenderTarget: Create (with MSAA enabled)
GLRenderTarget->>MSAAManager: Instantiate MSAA resources
MSAAManager->>WebGL: Create MSAA framebuffer and renderbuffers
App->>GLRenderTarget: Activate for rendering
GLRenderTarget->>MSAAManager: Bind MSAA framebuffer
MSAAManager->>WebGL: Bind framebuffer
App->>GLRenderTarget: Resolve MSAA buffer
GLRenderTarget->>MSAAManager: Blit MSAA buffer to main framebuffer
MSAAManager->>WebGL: Execute blit operation
App->>GLRenderTarget: Destroy
GLRenderTarget->>MSAAManager: Destroy MSAA resources
MSAAManager->>WebGL: Delete framebuffer and renderbuffers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/rhi-webgl/src/GLRenderTarget.ts (5)
🪛 GitHub Check: lintpackages/rhi-webgl/src/GLRenderTarget.ts[failure] 166-166: [failure] 164-164: [failure] 156-156: [failure] 154-154: [failure] 118-118: [failure] 117-117: [failure] 115-115: [failure] 105-105: [failure] 98-98: [failure] 39-39: 🪛 ESLintpackages/rhi-webgl/src/GLRenderTarget.ts[error] 39-39: Delete (prettier/prettier) [error] 98-100: Delete (prettier/prettier) [error] 105-105: Delete (prettier/prettier) [error] 115-115: Insert (prettier/prettier) [error] 117-117: Delete (prettier/prettier) [error] 118-118: Replace (prettier/prettier) [error] 154-154: Delete (prettier/prettier) [error] 156-156: Delete (prettier/prettier) [error] 164-164: Delete (prettier/prettier) [error] 166-166: Delete (prettier/prettier) [error] 208-208: Delete (prettier/prettier) tests/src/rhi-webgl/GLRenderTarget.test.ts[error] 9-9: Delete (prettier/prettier) [error] 24-24: Replace (prettier/prettier) [error] 25-25: Delete (prettier/prettier) [error] 32-32: Replace (prettier/prettier) [error] 33-33: Delete (prettier/prettier) [error] 40-40: Replace (prettier/prettier) [error] 41-41: Delete (prettier/prettier) [error] 48-48: Replace (prettier/prettier) [error] 49-49: Delete (prettier/prettier) [error] 56-56: Replace (prettier/prettier) [error] 57-57: Replace (prettier/prettier) [error] 58-58: Delete (prettier/prettier) [error] 65-65: Replace (prettier/prettier) [error] 66-66: Replace (prettier/prettier) [error] 67-67: Delete (prettier/prettier) [error] 75-75: Replace (prettier/prettier) [error] 76-76: Replace (prettier/prettier) [error] 79-79: Delete (prettier/prettier) [error] 90-91: Replace (prettier/prettier) [error] 92-92: Delete (prettier/prettier) [error] 95-95: Delete (prettier/prettier) [error] 100-101: Delete (prettier/prettier) [error] 116-117: Replace (prettier/prettier) [error] 118-118: Delete (prettier/prettier) [error] 122-122: Delete (prettier/prettier) [error] 125-125: Delete (prettier/prettier) [error] 130-130: Delete (prettier/prettier) [error] 135-135: Delete (prettier/prettier) [error] 141-143: Delete (prettier/prettier) [error] 148-148: Delete (prettier/prettier) [error] 157-158: Replace (prettier/prettier) [error] 159-159: Delete (prettier/prettier) [error] 163-163: Delete (prettier/prettier) [error] 178-178: Delete (prettier/prettier) [error] 179-180: Replace (prettier/prettier) [error] 181-181: Delete (prettier/prettier) [error] 185-185: Delete (prettier/prettier) [error] 188-188: Delete (prettier/prettier) [error] 211-211: Delete (prettier/prettier) [error] 214-214: Delete (prettier/prettier) [error] 220-220: Delete (prettier/prettier) [error] 224-224: Replace (prettier/prettier) 🪛 GitHub Actions: CIpackages/rhi-webgl/src/GLRenderTarget.ts[error] 39-39: Prettier formatting error: Delete extra spaces (prettier/prettier) 🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
eyworldwide
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.6 #2785 +/- ##
===========================================
- Coverage 79.15% 78.86% -0.30%
===========================================
Files 845 844 -1
Lines 90217 90208 -9
Branches 8934 8938 +4
===========================================
- Hits 71408 71139 -269
- Misses 18669 18926 +257
- Partials 140 143 +3
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/rhi-webgl/src/GLRenderTarget.ts (1)
103-104: Consider improving private property access patternUsing
@ts-ignoreto access private properties suggests an architectural issue. Consider making these properties protected or providing appropriate accessor methods.tests/src/rhi-webgl/GLRenderTarget.test.ts (1)
141-143: Remove extra empty lines}); - - describe("Error handling and validation", () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rhi-webgl/src/GLRenderTarget.ts(4 hunks)tests/src/rhi-webgl/GLRenderTarget.test.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
tests/src/rhi-webgl/GLRenderTarget.test.ts
[error] 9-9: Delete ··
(prettier/prettier)
[error] 24-24: Replace 'checkFramebufferStatus' with "checkFramebufferStatus"
(prettier/prettier)
[error] 25-25: Delete ······
(prettier/prettier)
[error] 32-32: Replace 'checkFramebufferStatus' with "checkFramebufferStatus"
(prettier/prettier)
[error] 33-33: Delete ······
(prettier/prettier)
[error] 40-40: Replace 'checkFramebufferStatus' with "checkFramebufferStatus"
(prettier/prettier)
[error] 41-41: Delete ······
(prettier/prettier)
[error] 48-48: Replace 'checkFramebufferStatus' with "checkFramebufferStatus"
(prettier/prettier)
[error] 49-49: Delete ······
(prettier/prettier)
[error] 56-56: Replace 'checkFramebufferStatus' with "checkFramebufferStatus"
(prettier/prettier)
[error] 57-57: Replace 'isContextLost' with "isContextLost"
(prettier/prettier)
[error] 58-58: Delete ······
(prettier/prettier)
[error] 65-65: Replace 'checkFramebufferStatus' with "checkFramebufferStatus"
(prettier/prettier)
[error] 66-66: Replace 'isContextLost' with "isContextLost"
(prettier/prettier)
[error] 67-67: Delete ······
(prettier/prettier)
[error] 75-75: Replace 'FRAMEBUFFER_INCOMPLETE_MULTISAMPLE' with "FRAMEBUFFER_INCOMPLETE_MULTISAMPLE"
(prettier/prettier)
[error] 76-76: Replace 'checkFramebufferStatus' with "checkFramebufferStatus"
(prettier/prettier)
[error] 79-79: Delete ········
(prettier/prettier)
[error] 90-91: Replace 'checkFrameBufferStatus')⏎········ with "checkFrameBufferStatus")
(prettier/prettier)
[error] 92-92: Delete ······
(prettier/prettier)
[error] 95-95: Delete ········
(prettier/prettier)
[error] 100-101: Delete ⏎········
(prettier/prettier)
[error] 116-117: Replace 'checkFrameBufferStatus')⏎········ with "checkFrameBufferStatus")
(prettier/prettier)
[error] 118-118: Delete ······
(prettier/prettier)
[error] 122-122: Delete ········
(prettier/prettier)
[error] 125-125: Delete ········
(prettier/prettier)
[error] 130-130: Delete ········
(prettier/prettier)
[error] 135-135: Delete ········
(prettier/prettier)
[error] 141-143: Delete ⏎⏎
(prettier/prettier)
[error] 148-148: Delete ······
(prettier/prettier)
[error] 157-158: Replace 'checkFrameBufferStatus')⏎········ with "checkFrameBufferStatus")
(prettier/prettier)
[error] 159-159: Delete ······
(prettier/prettier)
[error] 163-163: Delete ········
(prettier/prettier)
[error] 178-178: Delete ······
(prettier/prettier)
[error] 179-180: Replace 'checkFrameBufferStatus')⏎········ with "checkFrameBufferStatus")
(prettier/prettier)
[error] 181-181: Delete ······
(prettier/prettier)
[error] 185-185: Delete ········
(prettier/prettier)
[error] 188-188: Delete ········
(prettier/prettier)
[error] 211-211: Delete ······
(prettier/prettier)
[error] 214-214: Delete ······
(prettier/prettier)
[error] 220-220: Delete ······
(prettier/prettier)
[error] 224-224: Replace · with ⏎
(prettier/prettier)
packages/rhi-webgl/src/GLRenderTarget.ts
[error] 39-39: Delete ····
(prettier/prettier)
[error] 193-193: Delete ··
(prettier/prettier)
🪛 GitHub Check: lint
packages/rhi-webgl/src/GLRenderTarget.ts
[failure] 193-193:
Delete ··
[failure] 39-39:
Delete ····
🪛 GitHub Actions: CI
packages/rhi-webgl/src/GLRenderTarget.ts
[error] 39-39: Prettier formatting error: Delete extra spaces (prettier/prettier).
🔇 Additional comments (9)
packages/rhi-webgl/src/GLRenderTarget.ts (6)
53-74: Well-implemented MSAA resolve logicThe iterative approach for handling multiple render targets with proper buffer management is correct.
79-96: Proper resource cleanup implementationThe destroy method correctly handles all WebGL resources and prevents memory leaks.
158-183: Excellent framebuffer status validation utilityThe static method provides comprehensive error handling with descriptive messages for all framebuffer states, including WebGL2-specific cases.
254-256: Clean separation of MSAA concernsThe conditional creation of MSAAManager when antiAliasing > 1 is a good architectural decision.
314-316: Proper delegation to MSAA managerThe conditional activation of MSAA framebuffer maintains backward compatibility while delegating to the manager.
337-340: Complete cleanup of MSAA resourcesThe destroy method properly handles the MSAA manager lifecycle.
tests/src/rhi-webgl/GLRenderTarget.test.ts (3)
21-85: Comprehensive test coverage for checkFrameBufferStatusExcellent test coverage for all framebuffer status codes including WebGL2-specific cases.
87-141: Well-structured MSAA integration testsThe tests effectively validate MSAA functionality by mocking WebGL validation and testing the complete lifecycle.
145-197: Thorough error handling validationExcellent coverage of error conditions including format support, size consistency, and MSAA auto-downgrade.
| this._target = target; | ||
| this._oriDrawBuffers = oriDrawBuffers; | ||
| this._frameBuffer = this._gl.createFramebuffer(); | ||
|
|
There was a problem hiding this comment.
Fix formatting issue
- this._frameBuffer = this._gl.createFramebuffer();
-
+ this._frameBuffer = this._gl.createFramebuffer();
+📝 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.
| this._frameBuffer = this._gl.createFramebuffer(); |
🧰 Tools
🪛 GitHub Check: lint
[failure] 39-39:
Delete ····
🪛 ESLint
[error] 39-39: Delete ····
(prettier/prettier)
🪛 GitHub Actions: CI
[error] 39-39: Prettier formatting error: Delete extra spaces (prettier/prettier).
🤖 Prompt for AI Agents
In packages/rhi-webgl/src/GLRenderTarget.ts at line 39, there is a formatting
issue that needs correction. Review the line for any inconsistent indentation,
spacing, or alignment problems and adjust it to match the project's coding style
guidelines for clean and consistent formatting.
|
|
||
| // MSAA manager handles all MSAA-related operations | ||
| private _msaaManager: MSAAManager | null = null; |
There was a problem hiding this comment.
Fix formatting issue
private _curFaceIndex: TextureCubeFace = undefined;
-
+
// MSAA manager handles all MSAA-related operations
private _msaaManager: MSAAManager | null = null;📝 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.
| // MSAA manager handles all MSAA-related operations | |
| private _msaaManager: MSAAManager | null = null; | |
| private _curFaceIndex: TextureCubeFace = undefined; | |
| // MSAA manager handles all MSAA-related operations | |
| private _msaaManager: MSAAManager | null = null; |
🧰 Tools
🪛 GitHub Check: lint
[failure] 193-193:
Delete ··
🪛 ESLint
[error] 193-193: Delete ··
(prettier/prettier)
🤖 Prompt for AI Agents
In packages/rhi-webgl/src/GLRenderTarget.ts around lines 193 to 195, fix the
formatting issue by ensuring consistent indentation and spacing for the comment
and the private variable declaration. Align the comment properly above the
variable declaration without extra blank lines or misaligned spaces.
| describe("GLRenderTarget", () => { | ||
| let engine: WebGLEngine; | ||
| let gl: WebGLRenderingContext | WebGL2RenderingContext; | ||
|
|
There was a problem hiding this comment.
Fix formatting issues throughout the file
There are numerous formatting issues flagged by ESLint. Please run prettier to fix all formatting inconsistencies.
#!/bin/bash
# Run prettier to fix formatting issues
npm run prettier --write tests/src/rhi-webgl/GLRenderTarget.test.ts🧰 Tools
🪛 ESLint
[error] 9-9: Delete ··
(prettier/prettier)
🤖 Prompt for AI Agents
In tests/src/rhi-webgl/GLRenderTarget.test.ts at line 9, there are multiple
formatting issues flagged by ESLint. To fix this, run the prettier formatter on
the entire file using the command "npm run prettier --write
tests/src/rhi-webgl/GLRenderTarget.test.ts" to automatically correct all
formatting inconsistencies.
| renderTarget.destroy(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add newline at end of file
});
-});
+});
+📝 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.
| }); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 224-224: Replace · with ⏎
(prettier/prettier)
🤖 Prompt for AI Agents
In tests/src/rhi-webgl/GLRenderTarget.test.ts at line 224, the file is missing a
newline at the end. Add a single newline character after the closing brace to
ensure the file ends with a newline.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/rhi-webgl/src/GLRenderTarget.ts (2)
39-39: Fix formatting issue (duplicate)This formatting issue was already flagged in previous reviews but hasn't been addressed.
- this._frameBuffer = this._gl.createFramebuffer(); - + this._frameBuffer = this._gl.createFramebuffer(); +
214-216: Fix formatting issue (duplicate)This formatting issue was already flagged in previous reviews but hasn't been addressed.
private _curFaceIndex: TextureCubeFace = undefined; - + // MSAA manager handles all MSAA-related operations private _msaaManager: MSAAManager | null = null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rhi-webgl/src/GLRenderTarget.ts(5 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/rhi-webgl/src/GLRenderTarget.ts
[failure] 171-171:
Delete ····
[failure] 169-169:
Delete ····
[failure] 161-161:
Delete ····
[failure] 159-159:
Delete ····
[failure] 118-118:
Replace this._gl,·this._target,·internalFormat,·gl.COLOR_ATTACHMENT0·+·i with ⏎········this._gl,⏎········this._target,⏎········internalFormat,⏎········gl.COLOR_ATTACHMENT0·+·i⏎······
[failure] 117-117:
Delete ······
[failure] 115-115:
Insert ⏎·······
[failure] 105-105:
Delete ·
[failure] 98-98:
Delete ⏎⏎
[failure] 39-39:
Delete ····
🪛 ESLint
packages/rhi-webgl/src/GLRenderTarget.ts
[error] 39-39: Delete ····
(prettier/prettier)
[error] 98-100: Delete ⏎⏎
(prettier/prettier)
[error] 105-105: Delete ·
(prettier/prettier)
[error] 115-115: Insert ⏎·······
(prettier/prettier)
[error] 117-117: Delete ······
(prettier/prettier)
[error] 118-118: Replace this._gl,·this._target,·internalFormat,·gl.COLOR_ATTACHMENT0·+·i with ⏎········this._gl,⏎········this._target,⏎········internalFormat,⏎········gl.COLOR_ATTACHMENT0·+·i⏎······
(prettier/prettier)
[error] 159-159: Delete ····
(prettier/prettier)
[error] 161-161: Delete ····
(prettier/prettier)
[error] 169-169: Delete ····
(prettier/prettier)
[error] 171-171: Delete ····
(prettier/prettier)
[error] 214-214: Delete ··
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e (22.x, 4/4)
- GitHub Check: codecov
- GitHub Check: e2e (22.x, 2/4)
🔇 Additional comments (9)
packages/rhi-webgl/src/GLRenderTarget.ts (9)
46-48: LGTM!The activate method correctly binds the MSAA framebuffer for rendering operations.
53-74: LGTM!The MSAA resolution logic is correctly implemented using WebGL2 framebuffer blitting. The method properly handles multiple render targets and manages draw buffers appropriately.
79-96: LGTM!The destroy method properly cleans up all WebGL resources and prevents memory leaks by nullifying references.
179-204: LGTM!The static checkFrameBufferStatus method provides comprehensive error handling for various framebuffer completeness states. This centralizes error checking logic and provides descriptive error messages.
276-276: LGTM!Proper integration with MSAAManager in the constructor when MSAA is enabled.
335-337: LGTM!Clean delegation to MSAAManager for activating MSAA rendering.
344-346: LGTM!Proper delegation to MSAAManager for resolving MSAA buffers.
358-361: LGTM!Correct cleanup of MSAAManager resources in the destroy method.
415-415: LGTM!Good use of the new static createRenderBuffer method for consistent renderbuffer creation.
|
|
||
|
|
||
| private _bindFBO(): void { | ||
| const gl = this._gl; | ||
| const isWebGL2 = this._isWebGL2; | ||
|
|
||
| /** @ts-ignore */ | ||
| const { _depth, colorTextureCount } = this._target; | ||
|
|
||
| this._blitDrawBuffers = new Array(colorTextureCount); | ||
|
|
||
| gl.bindFramebuffer(gl.FRAMEBUFFER, this._frameBuffer); | ||
|
|
||
| // prepare MRT+MSAA color RBOs | ||
| for (let i = 0; i < colorTextureCount; i++) { | ||
| this._blitDrawBuffers[i] = gl.NONE; | ||
|
|
||
| const internalFormat = /** @ts-ignore */ | ||
| (this._target.getColorTexture(i)._platformTexture as GLTexture)._formatDetail.internalFormat; | ||
|
|
||
| this._colorRenderBuffers[i] = GLRenderTarget.createRenderBuffer(this._gl, this._target, internalFormat, gl.COLOR_ATTACHMENT0 + i); | ||
| } | ||
| gl.drawBuffers(this._oriDrawBuffers); | ||
|
|
||
| // prepare MSAA depth RBO | ||
| if (_depth !== null) { | ||
| const { internalFormat, attachment } = | ||
| _depth instanceof Texture | ||
| ? /** @ts-ignore */ | ||
| (_depth._platformTexture as GLTexture)._formatDetail | ||
| : GLTexture._getRenderBufferDepthFormatDetail(_depth, gl, isWebGL2); | ||
|
|
||
| this._depthRenderBuffer = GLRenderTarget.createRenderBuffer(this._gl, this._target, internalFormat, attachment); | ||
| } | ||
|
|
||
| GLRenderTarget.checkFrameBufferStatus(gl); | ||
| gl.bindFramebuffer(gl.FRAMEBUFFER, null); | ||
| gl.bindRenderbuffer(gl.RENDERBUFFER, null); | ||
| } |
There was a problem hiding this comment.
Fix multiple formatting issues
Multiple formatting issues need to be addressed throughout this method:
-
-
private _bindFBO(): void {
const gl = this._gl;
const isWebGL2 = this._isWebGL2;
/** @ts-ignore */
- const { _depth, colorTextureCount } = this._target;
+ const { _depth, colorTextureCount } = this._target;
this._blitDrawBuffers = new Array(colorTextureCount);
gl.bindFramebuffer(gl.FRAMEBUFFER, this._frameBuffer);
// prepare MRT+MSAA color RBOs
for (let i = 0; i < colorTextureCount; i++) {
this._blitDrawBuffers[i] = gl.NONE;
- const internalFormat = /** @ts-ignore */
- (this._target.getColorTexture(i)._platformTexture as GLTexture)._formatDetail.internalFormat;
-
- this._colorRenderBuffers[i] = GLRenderTarget.createRenderBuffer(this._gl, this._target, internalFormat, gl.COLOR_ATTACHMENT0 + i);
+ const internalFormat =
+ /** @ts-ignore */
+ (this._target.getColorTexture(i)._platformTexture as GLTexture)._formatDetail.internalFormat;
+
+ this._colorRenderBuffers[i] = GLRenderTarget.createRenderBuffer(
+ this._gl,
+ this._target,
+ internalFormat,
+ gl.COLOR_ATTACHMENT0 + i
+ );
}📝 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.
| private _bindFBO(): void { | |
| const gl = this._gl; | |
| const isWebGL2 = this._isWebGL2; | |
| /** @ts-ignore */ | |
| const { _depth, colorTextureCount } = this._target; | |
| this._blitDrawBuffers = new Array(colorTextureCount); | |
| gl.bindFramebuffer(gl.FRAMEBUFFER, this._frameBuffer); | |
| // prepare MRT+MSAA color RBOs | |
| for (let i = 0; i < colorTextureCount; i++) { | |
| this._blitDrawBuffers[i] = gl.NONE; | |
| const internalFormat = /** @ts-ignore */ | |
| (this._target.getColorTexture(i)._platformTexture as GLTexture)._formatDetail.internalFormat; | |
| this._colorRenderBuffers[i] = GLRenderTarget.createRenderBuffer(this._gl, this._target, internalFormat, gl.COLOR_ATTACHMENT0 + i); | |
| } | |
| gl.drawBuffers(this._oriDrawBuffers); | |
| // prepare MSAA depth RBO | |
| if (_depth !== null) { | |
| const { internalFormat, attachment } = | |
| _depth instanceof Texture | |
| ? /** @ts-ignore */ | |
| (_depth._platformTexture as GLTexture)._formatDetail | |
| : GLTexture._getRenderBufferDepthFormatDetail(_depth, gl, isWebGL2); | |
| this._depthRenderBuffer = GLRenderTarget.createRenderBuffer(this._gl, this._target, internalFormat, attachment); | |
| } | |
| GLRenderTarget.checkFrameBufferStatus(gl); | |
| gl.bindFramebuffer(gl.FRAMEBUFFER, null); | |
| gl.bindRenderbuffer(gl.RENDERBUFFER, null); | |
| } | |
| private _bindFBO(): void { | |
| const gl = this._gl; | |
| const isWebGL2 = this._isWebGL2; | |
| /** @ts-ignore */ | |
| const { _depth, colorTextureCount } = this._target; | |
| this._blitDrawBuffers = new Array(colorTextureCount); | |
| gl.bindFramebuffer(gl.FRAMEBUFFER, this._frameBuffer); | |
| // prepare MRT+MSAA color RBOs | |
| for (let i = 0; i < colorTextureCount; i++) { | |
| this._blitDrawBuffers[i] = gl.NONE; | |
| const internalFormat = | |
| /** @ts-ignore */ | |
| (this._target.getColorTexture(i)._platformTexture as GLTexture)._formatDetail.internalFormat; | |
| this._colorRenderBuffers[i] = GLRenderTarget.createRenderBuffer( | |
| this._gl, | |
| this._target, | |
| internalFormat, | |
| gl.COLOR_ATTACHMENT0 + i | |
| ); | |
| } | |
| gl.drawBuffers(this._oriDrawBuffers); | |
| // prepare MSAA depth RBO | |
| if (_depth !== null) { | |
| const { internalFormat, attachment } = | |
| _depth instanceof Texture | |
| ? /** @ts-ignore */ | |
| (_depth._platformTexture as GLTexture)._formatDetail | |
| : GLTexture._getRenderBufferDepthFormatDetail(_depth, gl, isWebGL2); | |
| this._depthRenderBuffer = GLRenderTarget.createRenderBuffer(this._gl, this._target, internalFormat, attachment); | |
| } | |
| GLRenderTarget.checkFrameBufferStatus(gl); | |
| gl.bindFramebuffer(gl.FRAMEBUFFER, null); | |
| gl.bindRenderbuffer(gl.RENDERBUFFER, null); | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[failure] 118-118:
Replace this._gl,·this._target,·internalFormat,·gl.COLOR_ATTACHMENT0·+·i with ⏎········this._gl,⏎········this._target,⏎········internalFormat,⏎········gl.COLOR_ATTACHMENT0·+·i⏎······
[failure] 117-117:
Delete ······
[failure] 115-115:
Insert ⏎·······
[failure] 105-105:
Delete ·
[failure] 98-98:
Delete ⏎⏎
🪛 ESLint
[error] 98-100: Delete ⏎⏎
(prettier/prettier)
[error] 105-105: Delete ·
(prettier/prettier)
[error] 115-115: Insert ⏎·······
(prettier/prettier)
[error] 117-117: Delete ······
(prettier/prettier)
[error] 118-118: Replace this._gl,·this._target,·internalFormat,·gl.COLOR_ATTACHMENT0·+·i with ⏎········this._gl,⏎········this._target,⏎········internalFormat,⏎········gl.COLOR_ATTACHMENT0·+·i⏎······
(prettier/prettier)
🤖 Prompt for AI Agents
In packages/rhi-webgl/src/GLRenderTarget.ts between lines 98 and 136, the
_bindFBO method has multiple formatting issues such as inconsistent indentation,
spacing, and line breaks. Fix these by ensuring consistent indentation (usually
2 or 4 spaces), proper spacing around operators and after commas, aligning
multiline expressions clearly, and separating logical blocks with blank lines
for readability. Maintain consistent comment formatting and ensure the code
style matches the project's conventions.
| 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) { | ||
| // Use MSAA storage | ||
| gl.renderbufferStorageMultisample(gl.RENDERBUFFER, antiAliasing, internalFormat, width, height); | ||
| } else { | ||
| // Use regular storage | ||
| gl.renderbufferStorage(gl.RENDERBUFFER, internalFormat, width, height); | ||
| } | ||
|
|
||
| gl.framebufferRenderbuffer(gl.FRAMEBUFFER, attachment, gl.RENDERBUFFER, renderBuffer); | ||
|
|
||
| return renderBuffer; | ||
| } |
There was a problem hiding this comment.
Fix formatting issues in createRenderBuffer method
Multiple indentation issues need to be corrected:
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) {
// Use MSAA storage
gl.renderbufferStorageMultisample(gl.RENDERBUFFER, antiAliasing, internalFormat, width, height);
} else {
// Use regular storage
gl.renderbufferStorage(gl.RENDERBUFFER, internalFormat, width, height);
}
-
+
gl.framebufferRenderbuffer(gl.FRAMEBUFFER, attachment, gl.RENDERBUFFER, renderBuffer);
-
+
return renderBuffer;
}📝 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.
| 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) { | |
| // Use MSAA storage | |
| gl.renderbufferStorageMultisample(gl.RENDERBUFFER, antiAliasing, internalFormat, width, height); | |
| } else { | |
| // Use regular storage | |
| gl.renderbufferStorage(gl.RENDERBUFFER, internalFormat, width, height); | |
| } | |
| gl.framebufferRenderbuffer(gl.FRAMEBUFFER, attachment, gl.RENDERBUFFER, renderBuffer); | |
| return renderBuffer; | |
| } | |
| 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) { | |
| // Use MSAA storage | |
| gl.renderbufferStorageMultisample(gl.RENDERBUFFER, antiAliasing, internalFormat, width, height); | |
| } else { | |
| // Use regular storage | |
| gl.renderbufferStorage(gl.RENDERBUFFER, internalFormat, width, height); | |
| } | |
| gl.framebufferRenderbuffer(gl.FRAMEBUFFER, attachment, gl.RENDERBUFFER, renderBuffer); | |
| return renderBuffer; | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[failure] 171-171:
Delete ····
[failure] 169-169:
Delete ····
[failure] 161-161:
Delete ····
[failure] 159-159:
Delete ····
🪛 ESLint
[error] 159-159: Delete ····
(prettier/prettier)
[error] 161-161: Delete ····
(prettier/prettier)
[error] 169-169: Delete ····
(prettier/prettier)
[error] 171-171: Delete ····
(prettier/prettier)
🤖 Prompt for AI Agents
In packages/rhi-webgl/src/GLRenderTarget.ts between lines 151 and 173, the
createRenderBuffer method has inconsistent indentation. Fix the formatting by
ensuring all lines inside the method are properly indented with consistent
spacing, aligning the code blocks under their respective control structures and
method signature for better readability.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactor: Internal architecture improvement for GLRenderTarget with MSAA logic separation
What is the current behavior? (You can also link to an open issue here)
Current behavior:
GLRenderTargetclass has too many responsibilities, containing extensive MSAA-related logic.What is the new behavior (if this is a feature change)?
New behavior:
Separation of Concerns:
MSAAManagerclass to handle MSAA-related logic exclusivelyNew Static Utility Method:
GLRenderTarget.checkFrameBufferStatus()static method instead of instance methodImproved Architecture:
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
❌ No breaking changes
RenderTargetusage patterns are fully compatibleOther information:
🧪 Test Coverage:
GLRenderTarget.test.tstest suite (15 test cases)RenderTarget.test.ts) continue to passSummary by CodeRabbit
Refactor
Tests