Support post process #2129
Support post process #2129
Conversation
# Conflicts: # packages/core/src/RenderPipeline/BasicRenderPipeline.ts # packages/core/src/RenderPipeline/RenderContext.ts
zhuxudong
added
rendering
WalkthroughThe recent changes introduce a new Changes
Sequence Diagram(s)Bloom Effect Control FlowsequenceDiagram
participant Scene
participant BloomEffect
participant Shader
participant Renderer
Scene->>+BloomEffect: Enable BloomEffect
BloomEffect->>Shader: Load bloom shaders
BloomEffect->>Renderer: Prepare to render with bloom
Renderer->>Shader: Apply prefilter
Renderer->>Shader: Apply horizontal blur
Renderer->>Shader: Apply vertical blur
Renderer->>Shader: Apply upsample
Renderer->>Shader: Apply final composition
Shader->>+Scene: Rendered scene with bloom effect
Poem
Tip AI model upgrade
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Outside diff range and nitpick comments (16)
packages/math/src/MathUtil.ts (1)
Line range hint
4-70: Consider refactoring theMathUtilclass.The class contains only static members. Prefer using simple functions instead of classes with only static members.
/** * Common utility methods for math operations. */ const MathUtil = { zeroTolerance: 1e-6, radToDegreeFactor: 180 / Math.PI, degreeToRadFactor: Math.PI / 180, clamp(v: number, min: number, max: number): number { return Math.max(min, Math.min(max, v)); }, equals(a: number, b: number): boolean { return Math.abs(a - b) <= MathUtil.zeroTolerance; }, isPowerOf2(v: number): boolean { return (v & (v - 1)) === 0; }, radianToDegree(r: number): number { return r * MathUtil.radToDegreeFactor; }, degreeToRadian(d: number): number { return d * MathUtil.degreeToRadFactor; }, lerp(start: number, end: number, t: number): number { return start + (end - start) * t; } }; export default MathUtil;Tools
Biome
[error] 4-71: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
e2e/case/postProcess-bloom-tonemap.ts (1)
1-4: Ensure the file header comments are accurate and complete.The file header comments are clear and provide useful information about the file's purpose and category.
Consider adding more details if necessary.
packages/core/src/RenderPipeline/PipelineUtils.ts (1)
Line range hint
15-220: Avoid classes that contain only static members.Prefer using simple functions instead of classes with only static members.
tests/src/core/Camera.test.ts (1)
97-97: Add a description for the new test.To maintain consistency and clarity, add a brief description of what is being tested for
camera.independentCanvasEnabled.+ // Test independentCanvasEnabled expect(camera.independentCanvasEnabled).to.eq(false);packages/core/src/shadow/CascadedShadowCasterPass.ts (2)
122-124: Consider adding comments for new parameters.Adding comments for
TextureWrapMode.ClampandTextureFilterMode.Bilinearwill improve code readability and maintainability.1, TextureWrapMode.Clamp, // Clamp texture wrapping TextureFilterMode.Bilinear // Bilinear texture filtering
137-139: Consider adding comments for new parameters.Adding comments for
TextureWrapMode.ClampandTextureFilterMode.Bilinearwill improve code readability and maintainability.1, TextureWrapMode.Clamp, // Clamp texture wrapping TextureFilterMode.Bilinear // Bilinear texture filteringpackages/core/src/Scene.ts (3)
14-14: Add a comment for the new import.Adding a comment for the
PostProcessManagerimport will improve code readability and maintainability.import { PostProcessManager } from "./postProcess"; // Post processing manager
67-68: Add a comment for the new property.Adding a comment for the
_postProcessManagerproperty will improve code readability and maintainability./** @internal */ _postProcessManager = new PostProcessManager(this); // Manages post-processing effects
269-274: Add a description for the new getter method.To maintain consistency and clarity, add a brief description of what the
postProcessManagergetter method does./** * Post Process manager. * @returns The post-process manager for the scene. */ get postProcessManager(): PostProcessManager { return this._postProcessManager; }packages/core/src/postProcess/effects/BloomEffect.ts (4)
1-10: Ensure consistent import order.For better readability and maintainability, consider grouping and ordering imports consistently, e.g., by module and then alphabetically.
15-24: Consider adding descriptions to enum values.Adding descriptions to each enum value can improve code readability and maintainability.
/** * This controls the size of the bloom texture. */ export enum BloomDownScaleMode { /** * Use this to select half size as the starting resolution. */ Half, /** * Use this to select quarter size as the starting resolution. */ Quarter }
26-30: Consider adding comments for private static properties.Adding comments for private static properties can improve code readability and maintainability.
export class BloomEffect extends PostProcessEffect { static readonly SHADER_NAME = "postProcessEffect-bloom"; // High-quality filtering macro private static _hqMacro: ShaderMacro = ShaderMacro.getByName("BLOOM_HQ"); // Dirt texture macro private static _dirtMacro: ShaderMacro = ShaderMacro.getByName("BLOOM_DIRT");
42-46: Consider initializing private properties in the constructor.For better readability and maintainability, consider initializing private properties in the constructor.
private _material: Material; private _threshold: number = 0.9; private _scatter: number = 0.7; private _highQualityFiltering = false;packages/core/src/postProcess/effects/TonemappingEffect.ts (3)
1-8: Ensure consistent import order.For better readability and maintainability, consider grouping and ordering imports consistently, e.g., by module and then alphabetically.
13-32: Consider adding descriptions to enum values.Adding descriptions to each enum value can improve code readability and maintainability.
/** * Options to select a tonemapping algorithm to use. */ export enum TonemappingMode { /** * Use this option if you do not want to apply tonemapping */ None, /** * Neutral tonemapper * @remarks Use this option if you only want range-remapping with minimal impact on color hue and saturation. */ Neutral, /** * ACES Filmic reference tonemapper (custom approximation) * @remarks * Use this option to apply a close approximation of the reference ACES tonemapper for a more filmic look. * It is more contrasted than Neutral and has an effect on actual color hue and saturation. */ ACES }
37-38: Consider adding comments for private properties.Adding comments for private properties can improve code readability and maintainability.
private _mode: TonemappingMode; private _material: Material;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
e2e/fixtures/originImage/PostProcess_postProcess-bloom-tonemap.jpgis excluded by!**/*.jpg
Files selected for processing (25)
- e2e/case/postProcess-bloom-tonemap.ts (1 hunks)
- e2e/config.ts (1 hunks)
- packages/core/src/BasicResources.ts (2 hunks)
- packages/core/src/Camera.ts (10 hunks)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (5 hunks)
- packages/core/src/RenderPipeline/DepthOnlyPass.ts (2 hunks)
- packages/core/src/RenderPipeline/OpaqueTexturePass.ts (2 hunks)
- packages/core/src/RenderPipeline/PipelinePass.ts (1 hunks)
- packages/core/src/RenderPipeline/PipelineUtils.ts (9 hunks)
- packages/core/src/RenderPipeline/RenderContext.ts (2 hunks)
- packages/core/src/RenderPipeline/enums/RenderBufferStoreAction.ts (1 hunks)
- packages/core/src/RenderPipeline/index.ts (1 hunks)
- packages/core/src/Scene.ts (3 hunks)
- packages/core/src/index.ts (1 hunks)
- packages/core/src/postProcess/PostProcessEffect.ts (1 hunks)
- packages/core/src/postProcess/PostProcessManager.ts (1 hunks)
- packages/core/src/postProcess/PostProcessPass.ts (1 hunks)
- packages/core/src/postProcess/effects/BloomEffect.ts (1 hunks)
- packages/core/src/postProcess/effects/TonemappingEffect.ts (1 hunks)
- packages/core/src/postProcess/effects/index.ts (1 hunks)
- packages/core/src/postProcess/index.ts (1 hunks)
- packages/core/src/shaderlib/pbr/pbr_helper.glsl (1 hunks)
- packages/core/src/shadow/CascadedShadowCasterPass.ts (4 hunks)
- packages/math/src/MathUtil.ts (1 hunks)
- tests/src/core/Camera.test.ts (1 hunks)
Files skipped from review due to trivial changes (4)
- packages/core/src/RenderPipeline/PipelinePass.ts
- packages/core/src/RenderPipeline/enums/RenderBufferStoreAction.ts
- packages/core/src/postProcess/effects/index.ts
- packages/core/src/postProcess/index.ts
Additional context used
Biome
packages/math/src/MathUtil.ts
[error] 4-71: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/core/src/postProcess/PostProcessManager.ts
[error] 19-19: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 26-26: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 33-33: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 33-33: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 34-34: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 34-34: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 52-52: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 71-71: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 76-76: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/core/src/RenderPipeline/PipelineUtils.ts
[error] 15-220: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/core/src/shadow/CascadedShadowCasterPass.ts
[error] 85-85: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
Additional comments not posted (51)
packages/core/src/RenderPipeline/index.ts (1)
3-3: Export statement addition looks good.The new export for
RenderBufferStoreActionaligns with the project's structure.packages/core/src/postProcess/PostProcessEffect.ts (2)
1-2: Import statements look good.The necessary imports for
RenderContext,RenderTarget, andTexture2Dare correctly added.
4-21: Abstract classPostProcessEffectlooks well-structured.The class contains a private property
_enabled, a constructor, and an abstract methodonRender. This structure is appropriate for an abstract class meant to define post-processing effects.However, ensure that all subclasses implement the
onRendermethod correctly.packages/core/src/RenderPipeline/OpaqueTexturePass.ts (2)
32-42: MethodonConfiglooks good.The method correctly configures the render target based on the camera's opaque texture downsampling settings.
48-49: MethodonRenderlooks good.The method correctly blits the texture and sets the camera's shader data.
packages/core/src/RenderPipeline/DepthOnlyPass.ts (2)
Line range hint
28-42: MethodonConfiglooks good.The method correctly configures the render target based on the camera's pixel viewport and depth texture settings.
49-53: MethodonRenderlooks good.The method correctly activates the render target, clears it, and renders the opaque and alpha test queues.
packages/math/src/MathUtil.ts (1)
61-70: Methodlerplooks good.The method correctly performs linear interpolation between two values.
packages/core/src/index.ts (1)
65-66: Ensure that thepostProcessmodule is correctly implemented and tested.The export statement looks good, but make sure the
postProcessmodule is properly implemented and covered by tests.e2e/case/postProcess-bloom-tonemap.ts (4)
5-20: Imports are well-organized and appropriate.The imports cover all necessary modules and classes for the post-processing setup.
23-25: Verify the canvas element exists and is correctly referenced.Ensure that the canvas element with the ID "canvas" exists in the HTML and is correctly referenced.
64-65: Ensure post-processing and HDR settings are correctly applied.The settings for enabling post-processing and HDR look correct. Validate that they produce the expected visual effects.
67-75: Verify the proper initialization and order of post-processing effects.The initialization and addition of post-processing effects seem correct. Ensure the order of effects is as intended.
packages/core/src/RenderPipeline/RenderContext.ts (2)
6-6: Ensure theRenderTargetimport is necessary and used correctly.The import statement for
RenderTargetis appropriate. Verify its usage within the file.
37-37: Validate the addition ofcolorTargetproperty.The
colorTargetproperty is added correctly. Ensure it is used appropriately within theRenderContextclass and other related classes.e2e/config.ts (1)
184-189: Ensure thePostProcessconfiguration is consistent with other categories.The addition of the
PostProcesscategory withbloomAndTonemaplooks consistent. Ensure it aligns with the project's configuration standards.packages/core/src/postProcess/PostProcessPass.ts (11)
1-8: Imports are well-organized and appropriate.The imports cover all necessary modules and classes for the
PostProcessPasssetup.
9-12: Initialization of private properties is correct.The private properties
_isActiveand_effectsare initialized correctly.
13-15: Ensure thenameproperty is used consistently.The
nameproperty is defined correctly. Ensure it is used consistently within the class and other related classes.
16-25: Getters and setters forisActiveare correct.The getters and setters for the
isActiveproperty are implemented correctly.
27-33: Getter foreffectsis correct.The getter for the
effectsproperty is implemented correctly.
34-42: Constructor is implemented correctly.The constructor initializes the
PostProcessPasswith the engine and optional name.
44-57: Ensure theonRendermethod handles post-processing effects correctly.The
onRendermethod appears to handle post-processing effects correctly. Ensure the logic is as intended and performs well.
59-73: Getter forgetEffectis correct.The getter for the
getEffectmethod is implemented correctly.
75-91: Getter forgetEffectsis correct.The getter for the
getEffectsmethod is implemented correctly.
93-130: MethodaddEffectis implemented correctly.The
addEffectmethod is implemented correctly. Ensure the logic handles edge cases appropriately.
132-142: MethodremoveEffectis implemented correctly.The
removeEffectmethod is implemented correctly. Ensure the logic handles edge cases appropriately.packages/core/src/BasicResources.ts (3)
33-34: Verify the correctness of the vertex coordinates.The vertex coordinates for the right-bottom and left-top vertices have been changed. Ensure these new coordinates are correct and expected for the intended rendering.
38-40: Verify the correctness of the flipY vertex coordinates.The vertex coordinates for the right-bottom and left-top vertices in the
flipYVerticesarray have been changed. Ensure these new coordinates are correct and expected for the intended rendering.
81-81: Confirm the change in mesh topology.The mesh topology has been changed from
TriangleStriptoTriangles. Ensure this change is intentional and does not affect the rendering pipeline adversely.packages/core/src/postProcess/PostProcessManager.ts (5)
79-81: Ensure the_passesproperty is used correctly.The
_passesproperty is initialized as aSafeLoopArray. Verify that this property is used correctly in the rest of the class.
115-139: Ensure proper handling ofindexOrPassparameter.The
addPassmethod handles adding a post-process pass at a specified index or appending it to the end. Verify that the method correctly handles both cases and ensures the pass belongs to the current engine.
156-189: Verify the_rendermethod logic.The
_rendermethod handles the rendering of post-processing passes. Ensure that the method correctly manages the rendering context and post-processing passes.
190-190: Ensure proper release of swap render targets.The
_rendermethod releases the swap render targets if post-processing is not enabled. Verify that this logic is correct and does not introduce any resource leaks.
10-13: Avoid usingthisin a static context.Using
thisin a static context can be confusing. Prefer using the class name instead.- private static _transformRT: RenderTarget[] = []; - private static _rtIdentifier = 0; - private static _srcRenderTarget: RenderTarget; - private static _destRenderTarget: RenderTarget; + private static PostProcessManager._transformRT: RenderTarget[] = []; + private static PostProcessManager._rtIdentifier = 0; + private static PostProcessManager._srcRenderTarget: RenderTarget; + private static PostProcessManager._destRenderTarget: RenderTarget;Likely invalid or redundant comment.
packages/core/src/shaderlib/pbr/pbr_helper.glsl (1)
14-14: Ensure the clamping logic is correct.The calculation in the
getAARoughnessFactorfunction has been modified to clamp the value to a maximum of 1.0. Verify that this change is correct and does not affect the rendering quality adversely.packages/core/src/RenderPipeline/PipelineUtils.ts (4)
18-22: Ensure the new properties are used correctly.The properties
_blitTexelSizePropertyand_texelSizehave been added. Verify that these properties are used correctly in the methods of the class.
Line range hint
34-67: Verify therecreateTextureIfNeededmethod logic.The
recreateTextureIfNeededmethod has been updated to includetextureWrapModeandtextureFilterModeparameters. Ensure that the method correctly handles these parameters and recreates the texture if needed.
Line range hint
80-138: Verify therecreateRenderTargetIfNeededmethod logic.The
recreateRenderTargetIfNeededmethod has been updated to includetextureWrapModeandtextureFilterModeparameters. Ensure that the method correctly handles these parameters and recreates the render target if needed.
Line range hint
149-218: Verify theblitTexturemethod logic.The
blitTexturemethod has been updated to includematerial,passIndex, andrenderBufferStoreActionparameters. Ensure that the method correctly handles these parameters and performs the blit operation as expected.packages/core/src/RenderPipeline/BasicRenderPipeline.ts (5)
118-124: Verify the internal color target handling.The internal color target is recreated if needed with the specified parameters. Ensure that this logic is correct and does not introduce any rendering issues.
153-157: Verify the color target and viewport handling.The
_drawRenderPassmethod handles the color target and viewport. Ensure that this logic is correct and does not introduce any rendering issues.
200-203: Verify the post-process rendering logic.The post-process manager's
_rendermethod is called to handle post-processing effects. Ensure that this logic is correct and integrates well with the rest of the rendering pipeline.
208-216: Verify the final blit operation.The final blit operation copies the
internalColorTargettocamera.renderTarget. Ensure that this logic is correct and does not introduce any rendering issues.
210-216: Verify the background texture rendering logic.The
_drawBackgroundTexturemethod handles rendering the background texture. Ensure that this logic is correct and does not introduce any rendering issues.packages/core/src/Scene.ts (1)
272-274: LGTM!The
postProcessManagergetter method is correctly implemented.packages/core/src/Camera.ts (5)
122-123: Verify the usage of new private properties_enableHDRand_enablePostProcess.Ensure these properties are used correctly within the class and are properly initialized.
Verification successful
The new private properties
_enableHDRand_enablePostProcessare used correctly within theCameraclass.
_enableHDRis accessed via getter and setter methods and used in a conditional expression._enablePostProcessis accessed via getter and setter methods and used in a conditional expression.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new private properties `_enableHDR` and `_enablePostProcess`. # Test: Search for the usage of `_enableHDR` and `_enablePostProcess` within the `Camera` class. rg --type-add 'ts:*.ts' --type ts '_enableHDR|_enablePostProcess' packages/core/src/Camera.tsLength of output: 424
730-732: Verify the correctness of_getInternalColorTextureFormatmethod.Ensure the method correctly returns the appropriate texture format based on the
_enableHDRproperty.Verification successful
To verify the correctness of the
_getInternalColorTextureFormatmethod, we need to check the following:
- The definition and usage of the
_enableHDRproperty within theCameraclass.- The import and definition of
TextureFormat.Let's proceed with generating the necessary shell scripts to gather this information.
The
_getInternalColorTextureFormatmethod is correctly implemented.
- The
_enableHDRproperty is defined within theCameraclass.- The
TextureFormatis correctly imported and used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `_getInternalColorTextureFormat` method. # Test: Ensure the method correctly returns the appropriate texture format based on the `_enableHDR` property. rg --type-add 'ts:*.ts' --type ts '_getInternalColorTextureFormat' packages/core/src/Camera.tsLength of output: 146
Script:
#!/bin/bash # Verify the definition and usage of `_enableHDR` within Camera class. ast-grep --lang typescript --pattern 'class Camera { $$$ private _enableHDR: $_; $$$ }' packages/core/src/Camera.ts # Verify the import of `TextureFormat`. rg 'TextureFormat' packages/core/src/Camera.tsLength of output: 496
369-385: Verify the correctness ofenableHDRgetter and setter.Ensure the getter and setter methods correctly handle the
_enableHDRproperty and include proper checks for hardware support.Verification successful
The
enableHDRgetter and setter methods are correctly implemented.
- The
_enableHDRproperty is properly initialized.- The setter method includes the necessary hardware support check using
GLCapabilityType.textureHalfFloat.- The
Logger.warnmethod is used appropriately to log warnings.- The
_checkMainCanvasAntialiasWastemethod is invoked correctly in the setter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `enableHDR` getter and setter. # Test: Ensure the getter and setter methods correctly handle the `_enableHDR` property and include proper checks for hardware support. rg --type-add 'ts:*.ts' --type ts 'enableHDR' packages/core/src/Camera.tsLength of output: 451
Script:
#!/bin/bash # Verify the initialization of the _enableHDR property rg --type-add 'ts:*.ts' --type ts '_enableHDR' packages/core/src/Camera.ts # Verify the usage of GLCapabilityType.textureHalfFloat rg --type-add 'ts:*.ts' --type ts 'GLCapabilityType.textureHalfFloat' packages/core/src/Camera.ts # Verify the Logger.warn method rg --type-add 'ts:*.ts' --type ts 'Logger.warn' packages/core/src/Camera.ts # Verify the _checkMainCanvasAntialiasWaste method rg --type-add 'ts:*.ts' --type ts '_checkMainCanvasAntialiasWaste' packages/core/src/Camera.tsLength of output: 915
387-400: Verify the correctness ofenablePostProcessgetter and setter.Ensure the getter and setter methods correctly handle the
_enablePostProcessproperty.Verification successful
The
enablePostProcessgetter and setter are correctly implemented.The
_checkMainCanvasAntialiasWastemethod correctly handles the necessary conditions whenenablePostProcessis set.
- Implementation of
_checkMainCanvasAntialiasWastemethod is verified and correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `enablePostProcess` getter and setter. # Test: Ensure the getter and setter methods correctly handle the `_enablePostProcess` property. rg --type-add 'ts:*.ts' --type ts 'enablePostProcess' packages/core/src/Camera.tsLength of output: 488
Script:
#!/bin/bash # Description: Verify the definition and implementation of the `_checkMainCanvasAntialiasWaste` method. # Test: Ensure the `_checkMainCanvasAntialiasWaste` method is defined and correctly handles the changes when `enablePostProcess` is set. rg --type-add 'ts:*.ts' --type ts '_checkMainCanvasAntialiasWaste' packages/core/src/Camera.tsLength of output: 323
Script:
#!/bin/bash # Description: Retrieve the implementation details of the `_checkMainCanvasAntialiasWaste` method. # Test: Ensure the `_checkMainCanvasAntialiasWaste` method correctly handles the changes when `enablePostProcess` is set. ast-grep --lang typescript --pattern $'class $_ { private _checkMainCanvasAntialiasWaste(): void { $$$ } }' packages/core/src/Camera.tsLength of output: 144
Script:
#!/bin/bash # Description: Retrieve the implementation details of the `_checkMainCanvasAntialiasWaste` method using rg. # Test: Ensure the `_checkMainCanvasAntialiasWaste` method correctly handles the changes when `enablePostProcess` is set. rg --type-add 'ts:*.ts' --type ts 'private _checkMainCanvasAntialiasWaste' -A 20 packages/core/src/Camera.tsLength of output: 477
166-174: Verify the logic ofindependentCanvasEnabledgetter.Ensure the logic correctly handles the new
_enableHDRand_enablePostProcessproperties.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/postProcess/effects/BloomEffect.ts (1 hunks)
- packages/core/src/shadow/CascadedShadowCasterPass.ts (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/shadow/CascadedShadowCasterPass.ts
Additional comments not posted (10)
packages/core/src/postProcess/effects/BloomEffect.ts (10)
1-25: LGTM!The imports and enum declaration are correct.
27-41: LGTM!The class definition and static properties are correctly implemented.
43-80: LGTM!The
thresholdproperty and its getter and setter methods are correctly implemented with necessary validation.
82-98: LGTM!The
scatterproperty and its getter and setter methods are correctly implemented with necessary validation.
100-111: LGTM!The
intensityproperty and its getter and setter methods are correctly implemented with necessary validation.
113-125: LGTM!The
tintproperty and its getter and setter methods are correctly implemented with necessary color updates.
127-145: LGTM!The
highQualityFilteringproperty and its getter and setter methods are correctly implemented with necessary macro updates.
147-161: LGTM!The
dirtTextureproperty and its getter and setter methods are correctly implemented with necessary macro updates.
163-172: LGTM!The
dirtIntensityproperty and its getter and setter methods are correctly implemented with necessary shader data updates.
220-625: LGTM!The remaining methods and shader fragments are correctly implemented with necessary shader logic.
| constructor(engine: Engine) { | ||
| super(engine); | ||
|
|
||
| const material = new Material(engine, Shader.find(BloomEffect.SHADER_NAME)); | ||
| const depthState = material.renderState.depthState; | ||
|
|
||
| depthState.enabled = false; | ||
| depthState.writeEnabled = false; | ||
|
|
||
| const shaderData = material.shaderData; | ||
| shaderData.setVector4(BloomEffect._bloomParams, new Vector4()); | ||
| shaderData.setVector4(BloomEffect._dirtTilingOffsetProp, new Vector4()); | ||
| shaderData.setVector4(BloomEffect._lowMipTexelSizeProp, new Vector4()); | ||
| shaderData.setColor(BloomEffect._tintProp, new Color(1, 1, 1, 1)); | ||
|
|
||
| this._material = material; | ||
| this.threshold = 0.9; | ||
| this.scatter = 0.7; | ||
| this.intensity = 1; | ||
| this.dirtIntensity = 1; | ||
| } |
There was a problem hiding this comment.
Consider initializing material properties in a separate method.
For better readability and maintainability, consider initializing material properties in a separate method.
constructor(engine: Engine) {
super(engine);
this._material = this._createMaterial(engine);
this.threshold = 0.9;
this.scatter = 0.7;
this.intensity = 1;
this.dirtIntensity = 1;
}
private _createMaterial(engine: Engine): Material {
const material = new Material(engine, Shader.find(BloomEffect.SHADER_NAME));
const depthState = material.renderState.depthState;
depthState.enabled = false;
depthState.writeEnabled = false;
const shaderData = material.shaderData;
shaderData.setVector4(BloomEffect._bloomParams, new Vector4());
shaderData.setVector4(BloomEffect._dirtTilingOffsetProp, new Vector4());
shaderData.setVector4(BloomEffect._lowMipTexelSizeProp, new Vector4());
shaderData.setColor(BloomEffect._tintProp, new Color(1, 1, 1, 1));
return material;
}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.
| constructor(engine: Engine) { | |
| super(engine); | |
| const material = new Material(engine, Shader.find(BloomEffect.SHADER_NAME)); | |
| const depthState = material.renderState.depthState; | |
| depthState.enabled = false; | |
| depthState.writeEnabled = false; | |
| const shaderData = material.shaderData; | |
| shaderData.setVector4(BloomEffect._bloomParams, new Vector4()); | |
| shaderData.setVector4(BloomEffect._dirtTilingOffsetProp, new Vector4()); | |
| shaderData.setVector4(BloomEffect._lowMipTexelSizeProp, new Vector4()); | |
| shaderData.setColor(BloomEffect._tintProp, new Color(1, 1, 1, 1)); | |
| this._material = material; | |
| this.threshold = 0.9; | |
| this.scatter = 0.7; | |
| this.intensity = 1; | |
| this.dirtIntensity = 1; | |
| } | |
| constructor(engine: Engine) { | |
| super(engine); | |
| this._material = this._createMaterial(engine); | |
| this.threshold = 0.9; | |
| this.scatter = 0.7; | |
| this.intensity = 1; | |
| this.dirtIntensity = 1; | |
| } | |
| private _createMaterial(engine: Engine): Material { | |
| const material = new Material(engine, Shader.find(BloomEffect.SHADER_NAME)); | |
| const depthState = material.renderState.depthState; | |
| depthState.enabled = false; | |
| depthState.writeEnabled = false; | |
| const shaderData = material.shaderData; | |
| shaderData.setVector4(BloomEffect._bloomParams, new Vector4()); | |
| shaderData.setVector4(BloomEffect._dirtTilingOffsetProp, new Vector4()); | |
| shaderData.setVector4(BloomEffect._lowMipTexelSizeProp, new Vector4()); | |
| shaderData.setColor(BloomEffect._tintProp, new Color(1, 1, 1, 1)); | |
| return material; | |
| } |
| override onRender(context: RenderContext, srcTexture: Texture2D, destRenderTarget: RenderTarget): void { | ||
| const engine = this.engine; | ||
| const camera = context.camera; | ||
| const material = this._material; | ||
| const downRes = this.downScale === BloomDownScaleMode.Half ? 1 : 2; | ||
| const pixelViewport = camera.pixelViewport; | ||
| const tw = pixelViewport.width >> downRes; | ||
| const th = pixelViewport.height >> downRes; | ||
|
|
||
| // Determine the iteration count | ||
| const mipCount = this._calculateMipCount(tw, th); | ||
|
|
||
| // Prefilter | ||
| this._prefilter(camera, srcTexture, tw, th, mipCount); | ||
| // Down sample - gaussian pyramid | ||
| this._downsample(mipCount); | ||
| // Up sample (bilinear by default, HQ filtering does bicubic instead | ||
| this._upsample(mipCount); | ||
| // Setup bloom on uber | ||
| this._setupBloom(camera); | ||
|
|
||
| PipelineUtils.blitTexture(engine, srcTexture, destRenderTarget, undefined, undefined, material, 4); | ||
| } |
There was a problem hiding this comment.
Consider splitting the onRender method into smaller methods.
The onRender method is quite large and can be split into smaller methods for better readability and maintainability.
override onRender(context: RenderContext, srcTexture: Texture2D, destRenderTarget: RenderTarget): void {
const engine = this.engine;
const camera = context.camera;
const material = this._material;
const shaderData = material.shaderData;
const downRes = this.downScale === BloomDownScaleMode.Half ? 1 : 2;
const pixelViewport = camera.pixelViewport;
const tw = pixelViewport.width >> downRes;
const th = pixelViewport.height >> downRes;
const mipCount = this._calculateMipCount(tw, th);
this._prefilter(engine, camera, srcTexture, tw, th, mipCount);
this._downsample(engine, material, mipCount);
this._upsample(engine, camera, material, mipCount);
this._setupBloom(shaderData, camera);
PipelineUtils.blitTexture(engine, srcTexture, destRenderTarget, undefined, undefined, material, 4);
}
private _calculateMipCount(tw: number, th: number): number {
const maxSize = Math.max(tw, th);
const iterations = Math.floor(Math.log2(maxSize) - 1);
return Math.min(Math.max(iterations, 1), this._maxIterations);
}
private _prefilter(engine: Engine, camera: Camera, srcTexture: Texture2D, tw: number, th: number, mipCount: number): void {
let mipWidth = tw,
mipHeight = th;
for (let i = 0; i < mipCount; i++) {
this._mipUpRT[i] = PipelineUtils.recreateRenderTargetIfNeeded(
engine,
this._mipUpRT[i],
mipWidth,
mipHeight,
camera._getInternalColorTextureFormat(),
null,
false,
false,
camera.msaaSamples,
TextureWrapMode.Clamp,
TextureFilterMode.Bilinear
);
this._mipDownRT[i] = PipelineUtils.recreateRenderTargetIfNeeded(
engine,
this._mipDownRT[i],
mipWidth,
mipHeight,
camera._getInternalColorTextureFormat(),
null,
false,
false,
camera.msaaSamples,
TextureWrapMode.Clamp,
TextureFilterMode.Bilinear
);
mipWidth = Math.max(1, Math.floor(mipWidth / 2));
mipHeight = Math.max(1, Math.floor(mipHeight / 2));
}
PipelineUtils.blitTexture(
engine,
srcTexture,
this._mipDownRT[0],
undefined,
undefined,
this._material,
0,
RenderBufferStoreAction.BlitMSAA
);
}
private _downsample(engine: Engine, material: Material, mipCount: number): void {
var lastDown = this._mipDownRT[0];
for (let i = 1; i < mipCount; i++) {
PipelineUtils.blitTexture(
engine,
<Texture2D>lastDown.getColorTexture(0),
this._mipUpRT[i],
undefined,
undefined,
material,
1,
RenderBufferStoreAction.BlitMSAA
);
PipelineUtils.blitTexture(
engine,
<Texture2D>this._mipUpRT[i].getColorTexture(0),
this._mipDownRT[i],
undefined,
undefined,
material,
2,
RenderBufferStoreAction.BlitMSAA
);
lastDown = this._mipDownRT[i];
}
}
private _upsample(engine: Engine, camera: Camera, material: Material, mipCount: number): void {
for (let i = mipCount - 2; i >= 0; i--) {
const lowMip = i == mipCount - 2 ? this._mipDownRT[i + 1] : this._mipUpRT[i + 1];
const highMip = this._mipDownRT[i];
const dst = this._mipUpRT[i];
material.shaderData.setTexture(BloomEffect._lowMipTextureProp, lowMip.getColorTexture(0));
if (this.highQualityFiltering) {
const texelSizeLow = material.shaderData.getVector4(BloomEffect._lowMipTexelSizeProp);
texelSizeLow.set(1 / lowMip.width, 1 / lowMip.height, lowMip.width, lowMip.height);
}
PipelineUtils.blitTexture(
engine,
<Texture2D>highMip.getColorTexture(0),
dst,
undefined,
undefined,
material,
3,
RenderBufferStoreAction.BlitMSAA
);
}
}
private _setupBloom(shaderData: ShaderData, camera: Camera): void {
const dirtTexture = this.dirtTexture;
if (dirtTexture) {
const dirtTilingOffset = shaderData.getVector4(BloomEffect._dirtTilingOffsetProp);
const dirtRatio = dirtTexture.width / dirtTexture.height;
const screenRatio = camera.aspectRatio;
if (dirtRatio > screenRatio) {
dirtTilingOffset.set(screenRatio / dirtRatio, 1, (1 - dirtTilingOffset.x) * 0.5, 0);
} else if (dirtRatio < screenRatio) {
dirtTilingOffset.set(1, dirtRatio / screenRatio, 0, (1 - dirtTilingOffset.y) * 0.5);
} else {
dirtTilingOffset.set(1, 1, 0, 0);
}
}
shaderData.setTexture(BloomEffect._bloomTextureProp, this._mipUpRT[0].getColorTexture(0));
}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.
| override onRender(context: RenderContext, srcTexture: Texture2D, destRenderTarget: RenderTarget): void { | |
| const engine = this.engine; | |
| const camera = context.camera; | |
| const material = this._material; | |
| const downRes = this.downScale === BloomDownScaleMode.Half ? 1 : 2; | |
| const pixelViewport = camera.pixelViewport; | |
| const tw = pixelViewport.width >> downRes; | |
| const th = pixelViewport.height >> downRes; | |
| // Determine the iteration count | |
| const mipCount = this._calculateMipCount(tw, th); | |
| // Prefilter | |
| this._prefilter(camera, srcTexture, tw, th, mipCount); | |
| // Down sample - gaussian pyramid | |
| this._downsample(mipCount); | |
| // Up sample (bilinear by default, HQ filtering does bicubic instead | |
| this._upsample(mipCount); | |
| // Setup bloom on uber | |
| this._setupBloom(camera); | |
| PipelineUtils.blitTexture(engine, srcTexture, destRenderTarget, undefined, undefined, material, 4); | |
| } | |
| override onRender(context: RenderContext, srcTexture: Texture2D, destRenderTarget: RenderTarget): void { | |
| const engine = this.engine; | |
| const camera = context.camera; | |
| const material = this._material; | |
| const downRes = this.downScale === BloomDownScaleMode.Half ? 1 : 2; | |
| const pixelViewport = camera.pixelViewport; | |
| const tw = pixelViewport.width >> downRes; | |
| const th = pixelViewport.height >> downRes; | |
| // Determine the iteration count | |
| const mipCount = this._calculateMipCount(tw, th); | |
| // Prefilter | |
| this._prefilter(camera, srcTexture, tw, th, mipCount); | |
| // Down sample - gaussian pyramid | |
| this._downsample(mipCount); | |
| // Up sample (bilinear by default, HQ filtering does bicubic instead | |
| this._upsample(mipCount); | |
| // Setup bloom on uber | |
| this._setupBloom(camera); | |
| PipelineUtils.blitTexture(engine, srcTexture, destRenderTarget, undefined, undefined, material, 4); | |
| } | |
| private _calculateMipCount(tw: number, th: number): number { | |
| const maxSize = Math.max(tw, th); | |
| const iterations = Math.floor(Math.log2(maxSize) - 1); | |
| return Math.min(Math.max(iterations, 1), this._maxIterations); | |
| } | |
| private _prefilter(engine: Engine, camera: Camera, srcTexture: Texture2D, tw: number, th: number, mipCount: number): void { | |
| let mipWidth = tw, | |
| mipHeight = th; | |
| for (let i = 0; i < mipCount; i++) { | |
| this._mipUpRT[i] = PipelineUtils.recreateRenderTargetIfNeeded( | |
| engine, | |
| this._mipUpRT[i], | |
| mipWidth, | |
| mipHeight, | |
| camera._getInternalColorTextureFormat(), | |
| null, | |
| false, | |
| false, | |
| camera.msaaSamples, | |
| TextureWrapMode.Clamp, | |
| TextureFilterMode.Bilinear | |
| ); | |
| this._mipDownRT[i] = PipelineUtils.recreateRenderTargetIfNeeded( | |
| engine, | |
| this._mipDownRT[i], | |
| mipWidth, | |
| mipHeight, | |
| camera._getInternalColorTextureFormat(), | |
| null, | |
| false, | |
| false, | |
| camera.msaaSamples, | |
| TextureWrapMode.Clamp, | |
| TextureFilterMode.Bilinear | |
| ); | |
| mipWidth = Math.max(1, Math.floor(mipWidth / 2)); | |
| mipHeight = Math.max(1, Math.floor(mipHeight / 2)); | |
| } | |
| PipelineUtils.blitTexture( | |
| engine, | |
| srcTexture, | |
| this._mipDownRT[0], | |
| undefined, | |
| undefined, | |
| this._material, | |
| 0, | |
| RenderBufferStoreAction.BlitMSAA | |
| ); | |
| } | |
| private _downsample(engine: Engine, material: Material, mipCount: number): void { | |
| var lastDown = this._mipDownRT[0]; | |
| for (let i = 1; i < mipCount; i++) { | |
| PipelineUtils.blitTexture( | |
| engine, | |
| <Texture2D>lastDown.getColorTexture(0), | |
| this._mipUpRT[i], | |
| undefined, | |
| undefined, | |
| material, | |
| 1, | |
| RenderBufferStoreAction.BlitMSAA | |
| ); | |
| PipelineUtils.blitTexture( | |
| engine, | |
| <Texture2D>this._mipUpRT[i].getColorTexture(0), | |
| this._mipDownRT[i], | |
| undefined, | |
| undefined, | |
| material, | |
| 2, | |
| RenderBufferStoreAction.BlitMSAA | |
| ); | |
| lastDown = this._mipDownRT[i]; | |
| } | |
| } | |
| private _upsample(engine: Engine, camera: Camera, material: Material, mipCount: number): void { | |
| for (let i = mipCount - 2; i >= 0; i--) { | |
| const lowMip = i == mipCount - 2 ? this._mipDownRT[i + 1] : this._mipUpRT[i + 1]; | |
| const highMip = this._mipDownRT[i]; | |
| const dst = this._mipUpRT[i]; | |
| material.shaderData.setTexture(BloomEffect._lowMipTextureProp, lowMip.getColorTexture(0)); | |
| if (this.highQualityFiltering) { | |
| const texelSizeLow = material.shaderData.getVector4(BloomEffect._lowMipTexelSizeProp); | |
| texelSizeLow.set(1 / lowMip.width, 1 / lowMip.height, lowMip.width, lowMip.height); | |
| } | |
| PipelineUtils.blitTexture( | |
| engine, | |
| <Texture2D>highMip.getColorTexture(0), | |
| dst, | |
| undefined, | |
| undefined, | |
| material, | |
| 3, | |
| RenderBufferStoreAction.BlitMSAA | |
| ); | |
| } | |
| } | |
| private _setupBloom(shaderData: ShaderData, camera: Camera): void { | |
| const dirtTexture = this.dirtTexture; | |
| if (dirtTexture) { | |
| const dirtTilingOffset = shaderData.getVector4(BloomEffect._dirtTilingOffsetProp); | |
| const dirtRatio = dirtTexture.width / dirtTexture.height; | |
| const screenRatio = camera.aspectRatio; | |
| if (dirtRatio > screenRatio) { | |
| dirtTilingOffset.set(screenRatio / dirtRatio, 1, (1 - dirtTilingOffset.x) * 0.5, 0); | |
| } else if (dirtRatio < screenRatio) { | |
| dirtTilingOffset.set(1, dirtRatio / screenRatio, 0, (1 - dirtTilingOffset.y) * 0.5); | |
| } else { | |
| dirtTilingOffset.set(1, 1, 0, 0); | |
| } | |
| } | |
| shaderData.setTexture(BloomEffect._bloomTextureProp, this._mipUpRT[0].getColorTexture(0)); | |
| } |
|
待讨论问题：
|
RFC: https://yuque.antfin-inc.com/shensi.zxd/ftoum6/gheugo4e7e1c1oil
Please check if the PR fulfills these requirements
Summary by CodeRabbit
New Features
Improvements