Support FXAA and linear space color blend #2605
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis update introduces a new anti-aliasing feature to the rendering pipeline by adding an Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Camera
participant Engine
participant BasicRenderPipeline
participant PostProcessManager
participant FinalPass
App->>Camera: Set antiAliasing = FXAA
App->>Engine: Initialize Engine
Engine->>BasicRenderPipeline: Instantiate with FinalPass
BasicRenderPipeline->>PostProcessManager: Render post-process with output target
BasicRenderPipeline->>Camera: Check _needFinalPass()
alt _needFinalPass() == true
BasicRenderPipeline->>FinalPass: Configure and render final pass
FinalPass->>Camera: Check antiAliasing property
alt antiAliasing == FXAA
FinalPass->>FinalPass: Apply sRGB conversion
FinalPass->>FinalPass: Apply FXAA shader
else
FinalPass->>FinalPass: Apply sRGB conversion only
end
FinalPass->>BasicRenderPipeline: Return processed frame
BasicRenderPipeline->>Camera: Blit final output if needed
else
BasicRenderPipeline->>PostProcessManager: Render post-process directly to camera target
end
Suggested labels
Suggested reviewers
Poem
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:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/case/postProcess-fxaa.ts (1)
17-36: Eliminate unuseddirtTextureor integrate it
Currently,dirtTextureis destructured from the resource array but never used. Consider removing it or applying it if intended:- const [_, __, dirtTexture] = resArray; + const [_, __] = resArray;packages/core/src/postProcess/PostProcessFinalPass.ts (1)
33-44: Potential overhead from toggling FXAA macro each frame
Enabling and disabling shader macros inonRendercan incur extra overhead from potential shader variant switches. If performance becomes a concern, consider caching or reducing toggles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/core/src/postProcess/shaders/FXAA/FXAA3_11.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/FinalPost.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/PostCommon.glslis excluded by!**/*.glsl
📒 Files selected for processing (8)
e2e/case/postProcess-fxaa.ts(1 hunks)packages/core/src/Camera.ts(2 hunks)packages/core/src/Engine.ts(2 hunks)packages/core/src/enums/AntiAliasing.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/postProcess/PostProcessFinalPass.ts(1 hunks)packages/core/src/postProcess/PostProcessPass.ts(1 hunks)packages/core/src/postProcess/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/core/src/Engine.ts (2)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
PostProcessFinalPass(15-45)packages/core/src/postProcess/index.ts (1)
PostProcessFinalPass(19-19)
packages/core/src/postProcess/PostProcessFinalPass.ts (5)
packages/core/src/postProcess/index.ts (3)
PostProcessFinalPass(19-19)PostProcessPass(17-17)PostProcessPassEvent(17-17)packages/core/src/index.ts (3)
Engine(2-2)Camera(16-16)AntiAliasing(77-77)packages/core/src/Engine.ts (1)
Engine(52-693)packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-185)packages/core/src/texture/RenderTarget.ts (1)
RenderTarget(11-245)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (10)
packages/core/src/postProcess/index.ts (1)
19-19: Good addition of the PostProcessFinalPass export.The export follows the existing pattern and makes the new post-processing class available for use throughout the application.
packages/core/src/enums/AntiAliasing.ts (1)
1-9: Well-structured anti-aliasing enum with proper documentation.The enum provides clear options for anti-aliasing configuration with proper JSDoc comments explaining each option's purpose. This follows the project's established patterns for enum definitions.
packages/core/src/index.ts (1)
77-77: Appropriate export of the AntiAliasing enum.The export makes the new AntiAliasing enum available throughout the module and is placed appropriately among other enum exports.
packages/core/src/Camera.ts (2)
20-20: Good import addition for AntiAliasing enum.The import is correctly placed with related imports and follows the established pattern in the file.
98-104: Well-documented antiAliasing property added to Camera class.The property extends the camera's functionality by allowing per-camera anti-aliasing configuration using the newly defined AntiAliasing enum. The property has proper JSDoc comments and a sensible default value.
packages/core/src/postProcess/PostProcessPass.ts (1)
14-17: Use ofNumber.MAX_SAFE_INTEGERfor the final pass
AssigningFinaltoNumber.MAX_SAFE_INTEGEReffectively ensures this pass executes last. This approach appears consistent with the intended purpose.packages/core/src/Engine.ts (2)
33-33: New import ofPostProcessFinalPass
The import statement correctly references the new final pass class, no issues.
275-277: Final pass appended after the uber pass
Instantiating and addingPostProcessFinalPassensures it runs at the end of the post-processing chain.packages/core/src/postProcess/PostProcessFinalPass.ts (2)
15-19: New final pass class
This class correctly extendsPostProcessPassand initializes a final material. Implementation looks solid.
47-52: Shader injection and creation
AppendingPostCommonandFXAA3_11toShaderLiband invokingShader.createfor the final post shader follows a coherent design.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/postProcess/PostProcessFinalPass.ts (2)
43-53: Optionally skip final pass when FXAA is not used.If this pass is specifically for FXAA (and does not handle any other effects), you can skip it to save performance when FXAA is disabled:
override isValid(): boolean { if (!this.isActive) { return false; } + const camera = this._engine.sceneManager.activeCamera; + const enableFXAA = camera?.antiAliasing === AntiAliasing.FastApproximateAntiAliasing; + return enableFXAA; }
57-63: Add test coverage for the final pass.No tests were found for this newly added class. Consider adding a unit or integration test to ensure the final pass behavior is correct, especially under different anti-aliasing and HDR settings.
I can provide a sample test snippet if you’d like. Just let me know!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/core/src/postProcess/shaders/FXAA/FXAA3_11.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/FinalPost.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/PostCommon.glslis excluded by!**/*.glsl
📒 Files selected for processing (3)
packages/core/src/Camera.ts(2 hunks)packages/core/src/Engine.ts(2 hunks)packages/core/src/postProcess/PostProcessFinalPass.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/Engine.ts
- packages/core/src/Camera.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (3)
packages/core/src/postProcess/index.ts (3)
PostProcessFinalPass(19-19)PostProcessPass(17-17)PostProcessPassEvent(17-17)packages/core/src/Engine.ts (1)
Engine(52-701)packages/core/src/index.ts (3)
Engine(2-2)Camera(16-16)AntiAliasing(77-77)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
26-28: Good job disabling depth test and write.This addresses a previous concern about ensuring the final blit operation is not affected by depth testing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
33-38: Check camera anti-aliasing inisValid()to skip unnecessary overhead.Currently,
isValid()only checksisActive, but the pass will still be queued even if the camera is not using FXAA. As previously suggested, consider returningfalsefromisValid()ifcamera.antiAliasing !== AntiAliasing.FastApproximateAntiAliasing. That way, the engine won’t include this pass at all when FXAA is disabled, improving performance.
🧹 Nitpick comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
57-61: Add tests to ensure correct shader setup.Registering
FXAA3_11withShaderLiband creating the final shader is a crucial step for FXAA. Consider adding automated tests to validate:
- Macros are properly enabled/disabled based on camera settings.
- The final blit is performed as intended.
Would you like help outlining unit or integration tests for this final pass?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/postProcess/PostProcessFinalPass.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (3)
packages/core/src/postProcess/index.ts (3)
PostProcessFinalPass(19-19)PostProcessPass(17-17)PostProcessPassEvent(17-17)packages/core/src/Engine.ts (1)
Engine(52-701)packages/core/src/index.ts (3)
Engine(2-2)Camera(16-16)AntiAliasing(77-77)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
25-29: Good job disabling depth test and write.By setting
enabled = falseandwriteEnabled = falsefor the depth state, you’re effectively ensuring this final blit operation doesn’t interfere with depth. This aligns with the prior feedback to force the blit onto the camera’s render target without depth conflicts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/core/src/postProcess/PostProcessFinalPass.ts (2)
33-39: 🛠️ Refactor suggestionOverride
isActivemethod to prevent undesired state modifications.The
isActiveproperty fromPostProcessPasscan still be read and written by users, potentially causing undesired behavior. You should override theisActivemethod if you want this pass to always be valid.+ /** + * @override + * @returns Always returns true since this pass should always be active + */ + get isActive(): boolean { + return true; + } + + /** + * @override + * @hidden + */ + set isActive(value: boolean) { + // Do nothing to prevent disabling this pass + } /** @inheritdoc */ override isValid(): boolean { if (!this.isActive) { return false; } return PostProcessFinalPass._camera?.antiAliasing === AntiAliasing.FastApproximateAntiAliasing; }
41-52: 🛠️ Refactor suggestionOptimize render logic by removing redundant FXAA macro enabling.
The current implementation enables the FXAA macro in
onRender()regardless of the camera's anti-aliasing settings, even thoughisValid()already checks forAntiAliasing.FastApproximateAntiAliasing. This is inefficient and could lead to unexpected behavior.Additionally, the method doesn't capture the camera parameter to update the static
_cameraproperty, which is used inisValid().override onRender(camera: Camera, srcTexture: Texture2D, destTarget: RenderTarget): void { + // Update static camera reference + PostProcessFinalPass._camera = camera; + const material = this._finalMaterial; - material.shaderData.enableMacro(PostProcessFinalPass._fxaaEnabledMacro); + // Only enable FXAA if the camera has it enabled + if (camera.antiAliasing === AntiAliasing.FastApproximateAntiAliasing) { + material.shaderData.enableMacro(PostProcessFinalPass._fxaaEnabledMacro); + } else { + material.shaderData.disableMacro(PostProcessFinalPass._fxaaEnabledMacro); + } + if (camera.enableHDR) { material.shaderData.enableMacro(PostProcessFinalPass._hdrInputMacro); } else { material.shaderData.disableMacro(PostProcessFinalPass._hdrInputMacro); } Blitter.blitTexture(camera.engine, srcTexture, destTarget, 0, camera.viewport, material); }
🧹 Nitpick comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
55-59: Consider error handling for shader creation.The static shader initialization at the end of the file has no error handling. If the shader creation fails, there would be no indication of the failure, which could make debugging difficult.
-Object.assign(ShaderLib, { - FXAA3_11 -}); +try { + Object.assign(ShaderLib, { + FXAA3_11 + }); + + Shader.create(PostProcessFinalPass.FINAL_SHADER_NAME, blitVs, FinalPost); +} catch (error) { + console.error("Failed to initialize PostProcessFinalPass shaders:", error); +} - -Shader.create(PostProcessFinalPass.FINAL_SHADER_NAME, blitVs, FinalPost);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/postProcess/PostProcessFinalPass.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (3)
packages/core/src/postProcess/index.ts (3)
PostProcessFinalPass(19-19)PostProcessPass(17-17)PostProcessPassEvent(17-17)packages/core/src/index.ts (3)
Camera(16-16)Engine(2-2)AntiAliasing(77-77)packages/core/src/Engine.ts (1)
Engine(52-701)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
26-29: Good job implementing the depth state settings.The implementation correctly disables depth test and depth write, which forces blitting to the camera's render target.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
35-39: Unify FXAA checks inisValidwithonRenderto avoid redundancy.
Currently,isValidprevents the pass from running unless FXAA is enabled via the static_camera. YetonRenderagain checks if_camerahasFastApproximateAntiAliasingbefore setting shader macros. You can remove the second check or unify everything inisValidto streamline logic and avoid repeated conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/postProcess/PostProcessFinalPass.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (3)
packages/core/src/postProcess/index.ts (3)
PostProcessFinalPass(19-19)PostProcessPass(17-17)PostProcessPassEvent(17-17)packages/core/src/index.ts (3)
Camera(16-16)Engine(2-2)AntiAliasing(77-77)packages/core/src/Engine.ts (1)
Engine(52-701)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
61-65: Neat integration of shader code.
RegisteringFXAA3_11intoShaderLiband creating the final shader is a clear and straightforward approach. This setup ensures the FXAA shader is properly recognized and available for the pass.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/postProcess/PostProcessFinalPass.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (3)
packages/core/src/postProcess/index.ts (3)
PostProcessFinalPass(19-19)PostProcessPass(17-17)PostProcessPassEvent(17-17)packages/core/src/index.ts (3)
Camera(16-16)Engine(2-2)AntiAliasing(77-77)packages/core/src/Engine.ts (1)
Engine(52-701)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
27-29: Depth testing correctly disabled.Thank you for disabling depth testing and depth writing. This fully addresses the previous recommendation to ensure a clean blit operation without unintended depth interference.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/core/src/postProcess/PostProcessFinalPass.ts (4)
15-17: Preserve clarity by adding doc comments for static macro properties.These macros (
_fxaaEnabledMacroand_hdrInputMacro) are central to the post-processing configuration. A short comment explaining their intended usage (e.g., enabling FXAA, toggling HDR input) can help future maintainers.
42-44: Avoid storing thecamerareference unless needed across frames.Assigning
this._cameraeach time may cause unexpected behavior in multi-camera scenarios if the engine reuses the same pass instance. Confirm whether this pass is per-camera or shared. If shared, store camera data in local variables withinonRender()to avoid accidental cross-camera usage.
47-57: Macrotoggling might be repeated across multiple frames.Enabling and disabling macros each frame can be slightly inefficient if these toggles don’t change often. Consider caching the previous state of
enableFXAA/enableHDRto skip toggling if the value remains the same.
67-67: Consider surfacing shader compile errors.If
Shader.createfails or logs warnings, it may silently break the final pass. Consider adding simple error-handling or debug logs to quickly identify issues in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/postProcess/PostProcessFinalPass.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (3)
packages/core/src/postProcess/PostProcessFinalPass.ts (3)
21-31: Acknowledgment: Depth test & write disabled as requested.Thanks for honoring the previous feedback to disable depth test and write in the final pass. This ensures the pass always properly overwrites the final render target without depth interference.
34-40: Potential confusion whenisValid()is called before_cameraassignment.
isValid()checks_camera?.antiAliasingbut_camerais only assigned insideonRender(). IfisValid()is called earlier in the pipeline (beforeonRender()),_camerawill beundefinedand the pass will always returnfalse.
If that’s intentional, please confirm; otherwise, assign_camerasooner or reconsider howisValid()retrieves the active camera.
63-65: Ensure that newly added shaders are compatible with existing pipeline.While extending
ShaderLibis straightforward, confirm thatFXAA3_11doesn’t conflict with other passes. For instance, ifPostCommonorUberPassis used in other parts of the pipeline, ensure they don’t inadvertently override or disable this new shader code.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/postProcess/PostProcessFinalPass.ts (1)
21-31: Constructor implementation is appropriate, but best to follow existing feedback.The constructor properly sets up the post-processing pass with the correct event and material configuration. The depth state is disabled, which is good for a screen-space pass.
As mentioned in a previous review comment, you should make sure depth test and depth write are disabled to force blitting to the camera render target.
🧹 Nitpick comments (2)
packages/core/src/postProcess/PostProcessFinalPass.ts (2)
43-53: Consider optimizing shader macro management.The code toggles shader macros during rendering, which is functional but could potentially be optimized. Consider setting macros only when the camera's anti-aliasing or HDR settings change, rather than on every render call.
- if (enableFXAA) { - material.shaderData.enableMacro(PostProcessFinalPass._fxaaEnabledMacro); - if (camera.enableHDR) { - material.shaderData.enableMacro(PostProcessFinalPass._hdrInputMacro); - } else { - material.shaderData.disableMacro(PostProcessFinalPass._hdrInputMacro); - } - } else { - material.shaderData.disableMacro(PostProcessFinalPass._fxaaEnabledMacro); - material.shaderData.disableMacro(PostProcessFinalPass._hdrInputMacro); - } + // Only update macros if settings have changed + if (this._lastFXAAEnabled !== enableFXAA || this._lastHDREnabled !== camera.enableHDR) { + if (enableFXAA) { + material.shaderData.enableMacro(PostProcessFinalPass._fxaaEnabledMacro); + if (camera.enableHDR) { + material.shaderData.enableMacro(PostProcessFinalPass._hdrInputMacro); + } else { + material.shaderData.disableMacro(PostProcessFinalPass._hdrInputMacro); + } + } else { + material.shaderData.disableMacro(PostProcessFinalPass._fxaaEnabledMacro); + material.shaderData.disableMacro(PostProcessFinalPass._hdrInputMacro); + } + this._lastFXAAEnabled = enableFXAA; + this._lastHDREnabled = camera.enableHDR; + }To implement this optimization, you'd need to add two more private properties:
private _lastFXAAEnabled: boolean = false; private _lastHDREnabled: boolean = false;
1-64: Consider adding overrideable methods for extensibility.The current implementation is solid for FXAA, but you might want to make it more extensible for future post-processing effects by adding protected methods that subclasses could override.
For example, you could add:
protected configureMaterial(material: Material, camera: Camera): void { const enableFXAA = camera?.antiAliasing === AntiAliasing.FastApproximateAntiAliasing; if (enableFXAA) { material.shaderData.enableMacro(PostProcessFinalPass._fxaaEnabledMacro); if (camera.enableHDR) { material.shaderData.enableMacro(PostProcessFinalPass._hdrInputMacro); } else { material.shaderData.disableMacro(PostProcessFinalPass._hdrInputMacro); } } else { material.shaderData.disableMacro(PostProcessFinalPass._fxaaEnabledMacro); material.shaderData.disableMacro(PostProcessFinalPass._hdrInputMacro); } }Then update onRender to use it:
override onRender(camera: Camera, srcTexture: Texture2D, destTarget: RenderTarget): void { this._camera = camera; this.configureMaterial(this._finalMaterial, camera); Blitter.blitTexture(camera.engine, srcTexture, destTarget, 0, camera.viewport, this._finalMaterial); }This would make it easier for subclasses to extend the functionality without having to override the entire onRender method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/Camera.ts(2 hunks)packages/core/src/postProcess/PostProcessFinalPass.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/Camera.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
🔇 Additional comments (6)
packages/core/src/postProcess/PostProcessFinalPass.ts (6)
1-13: Imports look well-organized and complete.The file imports all necessary dependencies for implementing a final post-processing pass with FXAA support. The modular approach to shader imports (separating FXAA3_11 and FinalPost) is good for maintainability.
14-20: Class properties are well-structured.The static shader macros and private properties are properly defined. The
_cameraproperty is now correctly implemented as a private instance property instead of static, addressing previous review concerns.
33-36: TheisValidmethod now correctly returnstrue.This implementation ensures the pass is always active, which makes sense for a final post-processing pass. This addresses previous concerns about the method potentially not returning a value.
38-41: Good initialization of the camera property.The camera is now correctly assigned at the beginning of the
onRendermethod, ensuring it's properly set before being used.
55-57: The blitting operation looks correct.Using Blitter.blitTexture is appropriate for copying the processed texture to the destination target. Including the camera viewport ensures proper rendering dimensions.
59-63: Shader registration looks good.The FXAA shader is correctly registered in the ShaderLib, and the final shader is created with appropriate vertex and fragment shaders.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
49-66: 🛠️ Refactor suggestionRelease
_finalPassresources indestroy()
FinalPassallocates its own material, mesh and render target.
BasicRenderPipeline.destroy()currently forgets to free them, leading to
GPU leaks when cameras are recreated.destroy(): void { this._cullingResults.destroy(); + this._finalPass?.release(); this._camera = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (20)
packages/core/src/postProcess/shaders/Bloom/BloomBlurH.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Bloom/BloomBlurV.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Bloom/BloomPrefilter.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Bloom/BloomUpsample.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/FXAA/FXAA3_11.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/FinalPost.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/UberPost.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/BlitScreen.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/SkyProcedural.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/background-texture.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/blinn-phong.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/particle.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/pbr-specular.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/pbr.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/skybox.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/sprite.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/text.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/unlit.fs.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glslis excluded by!**/*.glslpackages/ui/src/shader/uiDefault.fs.glslis excluded by!**/*.glsl
📒 Files selected for processing (7)
e2e/case/camera-opaque-texture.ts(0 hunks)e2e/case/material-shaderReplacement.ts(1 hunks)packages/core/src/Camera.ts(4 hunks)packages/core/src/Engine.ts(1 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(5 hunks)packages/core/src/postProcess/FinalPass.ts(1 hunks)packages/core/src/postProcess/PostProcessManager.ts(2 hunks)
💤 Files with no reviewable changes (1)
- e2e/case/camera-opaque-texture.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/Engine.ts
- packages/core/src/Camera.ts
- packages/core/src/postProcess/FinalPass.ts
🧰 Additional context used
🪛 ESLint
packages/core/src/postProcess/PostProcessManager.ts
[error] 204-204: Delete ·
(prettier/prettier)
[error] 207-207: Delete ·
(prettier/prettier)
[error] 214-215: Delete ⏎
(prettier/prettier)
🪛 GitHub Check: lint
packages/core/src/postProcess/PostProcessManager.ts
[failure] 214-214:
Delete ⏎
[failure] 207-207:
Delete ·
[failure] 204-204:
Delete ·
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e (22.x)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: codecov
🔇 Additional comments (2)
e2e/case/material-shaderReplacement.ts (1)
151-154: Ensure color-space correctness after removingoutputSRGBCorrectionThe shader now writes the sampled texel directly to
gl_FragColor.
That is correct if (and only if) the newFinalPassis always invoked whenever this shader is rendered, because the sRGB conversion has been moved there.
If the camera is configured so that_needFinalPass()returnsfalse, the rendered colors will stay in linear space and appear darker.Please double-check with a quick render test (FXAA off + LDR render-target) or search for usages of
setReplacementShader("UVCheckShader")to confirm that every path still schedules a final pass. If you find cases where the final pass is skipped, re-introduce the correction locally or force the camera to enable the final pass.packages/core/src/postProcess/PostProcessManager.ts (1)
221-237: Validate MSAA sample count & color-space flags
_getOutputRenderTarget()hard-codesmsaaSamples = 1.
If the camera itself is multisampled, this down-sampling may introduce
visible edge aliasing before FXAA is applied, and wastes the original
hardware-MSAA resolve. Please verify whether:
- We intentionally want a single-sample texture as FXAA input, or
- We should pass
camera.msaaSamplesinstead.Likewise, the
isSRGBColorSpaceflag is derived via!camera._isTargetFormatHDR().
Ensure this matches the engine’s SRGB/HDR semantics for all targets.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/src/core/Camera.test.ts (1)
119-119: Add space before the inline comment.The test case updates correctly reflect the new behavior where
independentCanvasEnabledis nowtrueinitially due to the sRGB pass introduced as part of the FXAA support. However, there's a minor formatting issue - you need to add a space betweentrue);and the comment.- expect(camera.independentCanvasEnabled).to.eq(true);// Because sRGB pass + expect(camera.independentCanvasEnabled).to.eq(true); // Because sRGB pass🧰 Tools
🪛 ESLint
[error] 119-119: Insert
·(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/core/Camera.test.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
tests/src/core/Camera.test.ts
[error] 119-119: Insert ·
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
- GitHub Check: build (22.x, windows-latest)
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Tests