Open post process pipeline and support local mode #2401
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2401 +/- ##
===========================================
+ Coverage 68.43% 68.87% +0.43%
===========================================
Files 913 920 +7
Lines 94780 95421 +641
Branches 7975 8111 +136
===========================================
+ Hits 64864 65721 +857
+ Misses 29666 29447 -219
- Partials 250 253 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes to the post-processing setup in a graphics engine, focusing on restructuring how bloom and tonemapping effects are applied. New classes for Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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 (24)
e2e/case/postProcess-HDR-bloom-ACES.ts (2)
19-19: Redundant Assignment oftonemappingEffect.modeThe
tonemappingEffect.modeis assignedTonemappingMode.ACEStwice in the code—once at line 19 and again later. This redundant assignment can be removed to improve code clarity.Apply this diff to remove the redundant assignment:
tonemappingEffect.mode = TonemappingMode.ACES; bloomEffect.threshold = 0.5; bloomEffect.intensity = 1; bloomEffect.dirtTexture = dirtTexture; -tonemappingEffect.mode = TonemappingMode.ACES;🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 15-19: e2e/case/postProcess-HDR-bloom-ACES.ts#L15-L19
Added lines #L15 - L19 were not covered by tests
15-19: Add Unit Tests for New Post-Processing EffectsThe introduction of the global post-process entity and the addition of
BloomEffectandTonemappingEffectbring significant new functionality. Consider adding unit tests to ensure these effects are correctly applied and functioning as intended.Would you like assistance in creating unit tests for these new effects?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 15-19: e2e/case/postProcess-HDR-bloom-ACES.ts#L15-L19
Added lines #L15 - L19 were not covered by testse2e/case/postProcess-HDR-bloom-neutral.ts (1)
15-19: Consider refactoring the post-processing setup for reusabilityIf similar post-processing setup code (lines 15-19) is used across multiple test cases, consider extracting this logic into a shared utility function or component. This will enhance code reuse and maintainability while reducing duplication.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 15-19: e2e/case/postProcess-HDR-bloom-neutral.ts#L15-L19
Added lines #L15 - L19 were not covered by testse2e/case/postProcess-LDR-bloom-neutral.ts (1)
22-22: Add tests to cover the new configurations.Static analysis indicates that the added lines are not covered by tests. Adding tests will ensure the new post-processing effects work as intended and help maintain code quality.
Would you like assistance in creating unit tests for these post-processing configurations? I can help draft some test cases to improve coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-22: e2e/case/postProcess-LDR-bloom-neutral.ts#L22
Added line #L22 was not covered by testse2e/case/postProcess-customPass.ts (2)
80-80: Duplicate assignment oftonemappingEffect.modeThe
tonemappingEffect.modeis assigned twice: first toTonemappingMode.ACESon line 80 and then toTonemappingMode.Neutralon line 85. Please verify if both assignments are necessary or remove the redundant one to avoid confusion.Also applies to: 85-85
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-80: e2e/case/postProcess-customPass.ts#L76-L80
Added lines #L76 - L80 were not covered by tests
68-89: Add tests to cover the new custom post-processing passThe new code introduced between lines 68-89 is not covered by tests, as indicated by static analysis tools. Adding tests would ensure the custom post-processing pass works correctly and helps prevent future regressions.
Would you like assistance in generating test cases for the new custom post-processing pass?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 68-71: e2e/case/postProcess-customPass.ts#L68-L71
Added lines #L68 - L71 were not covered by tests
[warning] 73-74: e2e/case/postProcess-customPass.ts#L73-L74
Added lines #L73 - L74 were not covered by tests
[warning] 76-80: e2e/case/postProcess-customPass.ts#L76-L80
Added lines #L76 - L80 were not covered by tests
[warning] 82-85: e2e/case/postProcess-customPass.ts#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 87-89: e2e/case/postProcess-customPass.ts#L87-L89
Added lines #L87 - L89 were not covered by testspackages/core/src/postProcess/PostProcessEffect.ts (1)
72-72: Provide implementation or clear guidance for thelerpmethodThe
lerpmethod is currently empty without implementation details. If this method is intended to be used by subclasses, consider providing a default implementation or documenting the expected behavior to guide developers.Add implementation or documentation:
/** * Interpolates this effect's properties with another effect's properties based on the interpolation factor. * @param fromEffect - The effect to interpolate from * @param interpFactor - The interpolation factor in range [0,1] */ lerp(fromEffect: PostProcessEffect, interpFactor: number): void { // Implementation of property interpolation goes here }packages/core/src/postProcess/PostProcess.ts (1)
54-64: Specify the return type to includeundefinedfor clarity.The method
getEffectcan returnundefinedif no effect is found. Explicitly specifying the return type asInstanceType<T> | undefinedenhances code readability.Apply this diff to update the return type:
- getEffect<T extends typeof PostProcessEffect>(type: T): InstanceType<T> { + getEffect<T extends typeof PostProcessEffect>(type: T): InstanceType<T> | undefined {packages/core/src/postProcess/effects/BloomEffect.ts (2)
67-77: Explicitly Declare Access Modifiers for Class MembersFor clarity and consistency, it's recommended to explicitly declare access modifiers (
public,private,protected) for class members. Currently, the propertieshighQualityFiltering,downScale, anddirtTextureare declared without access modifiers, making them public by default. Explicitly declaring them aspublicimproves code readability and maintainability.Apply this diff to specify access modifiers:
- highQualityFiltering = false; + public highQualityFiltering = false; - downScale = BloomDownScaleMode.Half; + public downScale = BloomDownScaleMode.Half; - dirtTexture: Texture2D; + public dirtTexture: Texture2D;
122-123: Simplify Assignment indirtIntensitySetterYou can simplify the assignment by directly assigning the clamped value to
this._dirtIntensity, removing the redundant variable reassignment.Apply this diff to streamline the setter:
set dirtIntensity(value: number) { - value = Math.max(0, value); - this._dirtIntensity = value; + this._dirtIntensity = Math.max(0, value); }packages/core/src/postProcess/PostProcessManager.ts (2)
98-117: Consolidate sorting methods to eliminate code duplicationThe
_sortPostProcess()and_sortPostProcessPass()methods share similar logic for checking the need for sorting and performing the sort operation on their respective arrays. To improve maintainability and reduce code duplication, consider creating a generic sorting utility method.Here's how you might refactor the sorting logic:
private _sortEntities<T>(needSorting: boolean, entities: T[], compareFn: (a: T, b: T) => number): boolean { if (needSorting) { if (entities.length) { entities.sort(compareFn); } return false; } return needSorting; } // Then update the sorting methods: private _sortPostProcess(): void { this._postProcessNeedSorting = this._sortEntities( this._postProcessNeedSorting, this._activePostProcesses, (a, b) => a.priority - b.priority ); } private _sortPostProcessPass(): void { this._postProcessPassNeedSorting = this._sortEntities( this._postProcessPassNeedSorting, this._activePostProcessPasses, (a, b) => a.event - b.event ); }This refactoring enhances code readability and makes future modifications easier.
21-33: Ensure proper initialization of private propertiesThe private properties are initialized appropriately to prevent undefined behaviors. However, consider adding type annotations to clarify the expected types of properties like
_swapRenderTarget,_srcRenderTarget, and_destRenderTarget, especially if they can benullorundefined.Example:
private _swapRenderTarget: RenderTarget | null = null;This makes it clear that the render targets may not be immediately available and helps with strict type checking.
packages/core/src/postProcess/PostProcessUberPass.ts (1)
241-252: Null-check render target textures before destroyingIn the
_releaseBloomRenderTargetsmethod, after checking thatdownRTandupRTare not null, you access their color textures:downRT.getColorTexture(0).destroy(true); upRT.getColorTexture(0).destroy(true);Ensure that
getColorTexture(0)does not returnnullorundefinedto prevent potential null reference errors.packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
203-215: Increase test coverage for new rendering pathsThe static analysis indicates that lines 210-211 and 214-215 are not covered by tests. Adding unit tests for these new code paths will ensure they function correctly and help maintain high code quality.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 210-211: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L210-L211
Added lines #L210 - L211 were not covered by tests
[warning] 214-215: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L214-L215
Added lines #L214 - L215 were not covered by testspackages/core/src/postProcess/index.ts (1)
1-1: Consider documenting the available effects.Since effects are a crucial part of the post-processing system as mentioned in the PR objectives, it would be helpful to have documentation about the available effects (like Bloom and Tonemapping) and their usage.
Would you like me to help create documentation for the effects module?
packages/core/src/postProcess/effects/TonemappingEffect.ts (1)
23-37: Add documentation for PostProcessEffect integrationWhile the enum and properties are well-documented, consider adding class-level documentation explaining:
- The purpose of extending PostProcessEffect
- How this fits into the new post-processing workflow
- Migration guide for users of the previous implementation
packages/core/src/postProcess/PostProcessPass.ts (2)
7-10: Add documentation for the PostProcessPassEvent enumThe enum values suggest room for intermediate events between 0 and 100. Consider adding JSDoc comments to explain:
- The purpose and timing of BeforeUber and AfterUber events
- Why these specific numeric values were chosen
- Whether intermediate values are intended for future use
export enum PostProcessPassEvent { + /** + * Executed before the uber post-processing pass + */ BeforeUber = 0, + /** + * Executed after the uber post-processing pass + */ AfterUber = 100 }
12-40: Consider caching the engine referenceThe class structure is well-designed with clear separation of concerns and proper documentation. However, the
enginegetter could be optimized by caching the reference since it's unlikely to change during the lifetime of the pass.export abstract class PostProcessPass { + private _engine: Engine; + event = PostProcessPassEvent.AfterUber; isActive = true; get scene(): Scene { return this.postProcessManager.scene; } get engine(): Engine { - return this.postProcessManager.scene.engine; + return this._engine ||= this.postProcessManager.scene.engine; } - constructor(public postProcessManager: PostProcessManager) {} + constructor(public postProcessManager: PostProcessManager) { + this._engine = this.postProcessManager.scene.engine; + }packages/core/src/RenderPipeline/OpaqueTexturePass.ts (1)
7-7: LGTM! Good architectural improvementThe introduction of a dedicated
Blitterclass and its import aligns well with separation of concerns, moving the blitting functionality to a specialized utility class.packages/core/src/Component.ts (1)
22-27: Visibility change appears necessary but warrants documentation.The change from private to public (with @internal) for
_phasedActiveis reasonable given the post-processing workflow requirements. However, this internal state exposure should be well documented.Consider adding the following to the JSDoc:
- Why this property needs to be public
- How it's used in the post-processing pipeline
- Warning about internal API usage
/** * @internal + * This property is exposed for use in the post-processing pipeline to manage effect states. + * It tracks whether the component is currently active in the scene hierarchy. + * @warning This is an internal API and should not be used by external code. */ @ignoreClone _phasedActive: boolean = false;packages/core/src/Scene.ts (2)
84-90: Enhance getter documentation with usage examplesWhile the getter is properly implemented, consider enhancing the documentation with usage examples for common post-processing scenarios (e.g., adding bloom or tonemapping effects).
297-299: Consider lazy initialization of PostProcessUberPassThe PostProcessUberPass is currently initialized for every scene, which might impact scene creation performance. Consider lazy initialization if the post-processing features aren't always needed.
- const uberPass = new PostProcessUberPass(this._postProcessManager); - this._postProcessManager.addPostProcessPass(uberPass); + // Initialize post-processing only when needed + if (this._engine.options.enablePostProcess) { + const uberPass = new PostProcessUberPass(this._postProcessManager); + this._postProcessManager.addPostProcessPass(uberPass); + }packages/core/src/Camera.ts (2)
68-72: Enhance documentation for thepostProcessMaskproperty.The current documentation lacks essential details. Consider adding:
- Purpose and use cases
- Relationship with post-processing effects
- Example usage
- Impact on performance
/** - * Determines which PostProcess to use. + * Determines which post-processing effects to apply using a layer-based mask. + * + * @remarks + * Similar to cullingMask, this property uses bit flags to control which post-processing + * effects are applied. Set specific bits to enable/disable effects for different layers. + * + * @example + * ```typescript + * // Apply post-processing only to layer 1 + * camera.postProcessMask = Layer.Layer1; + * + * // Apply post-processing to everything except layer 2 + * camera.postProcessMask = ~Layer.Layer2; + * ``` */
68-72: Add validation for postProcessMask values.Consider adding runtime validation to ensure proper layer values and maintain consistency with the post-processing system.
+ /** + * Determines which PostProcess to use. + */ + get postProcessMask(): Layer { + return this._postProcessMask; + } + + set postProcessMask(value: Layer) { + if (value < 0) { + Logger.warn("PostProcess mask cannot be negative."); + return; + } + this._postProcessMask = value; + } + - postProcessMask: Layer = Layer.Everything; + private _postProcessMask: Layer = Layer.Everything;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/PostProcess_postProcess-customPass.jpgis excluded by!**/*.jpg
📒 Files selected for processing (21)
e2e/case/postProcess-HDR-bloom-ACES.ts(2 hunks)e2e/case/postProcess-HDR-bloom-neutral.ts(2 hunks)e2e/case/postProcess-LDR-bloom-neutral.ts(2 hunks)e2e/case/postProcess-customPass.ts(1 hunks)e2e/config.ts(1 hunks)packages/core/src/Camera.ts(1 hunks)packages/core/src/Component.ts(1 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(2 hunks)packages/core/src/RenderPipeline/Blitter.ts(1 hunks)packages/core/src/RenderPipeline/OpaqueTexturePass.ts(2 hunks)packages/core/src/RenderPipeline/PipelineUtils.ts(0 hunks)packages/core/src/RenderPipeline/index.ts(1 hunks)packages/core/src/Scene.ts(5 hunks)packages/core/src/postProcess/PostProcess.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/PostProcessUberPass.ts(1 hunks)packages/core/src/postProcess/effects/BloomEffect.ts(4 hunks)packages/core/src/postProcess/effects/TonemappingEffect.ts(2 hunks)packages/core/src/postProcess/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/RenderPipeline/PipelineUtils.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/RenderPipeline/index.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
e2e/case/postProcess-HDR-bloom-ACES.ts
[warning] 5-5: e2e/case/postProcess-HDR-bloom-ACES.ts#L5
Added line #L5 was not covered by tests
[warning] 15-19: e2e/case/postProcess-HDR-bloom-ACES.ts#L15-L19
Added lines #L15 - L19 were not covered by tests
[warning] 22-22: e2e/case/postProcess-HDR-bloom-ACES.ts#L22
Added line #L22 was not covered by tests
e2e/case/postProcess-HDR-bloom-neutral.ts
[warning] 5-5: e2e/case/postProcess-HDR-bloom-neutral.ts#L5
Added line #L5 was not covered by tests
[warning] 15-19: e2e/case/postProcess-HDR-bloom-neutral.ts#L15-L19
Added lines #L15 - L19 were not covered by tests
[warning] 22-22: e2e/case/postProcess-HDR-bloom-neutral.ts#L22
Added line #L22 was not covered by tests
e2e/case/postProcess-LDR-bloom-neutral.ts
[warning] 5-5: e2e/case/postProcess-LDR-bloom-neutral.ts#L5
Added line #L5 was not covered by tests
[warning] 15-19: e2e/case/postProcess-LDR-bloom-neutral.ts#L15-L19
Added lines #L15 - L19 were not covered by tests
[warning] 22-22: e2e/case/postProcess-LDR-bloom-neutral.ts#L22
Added line #L22 was not covered by tests
e2e/case/postProcess-customPass.ts
[warning] 19-20: e2e/case/postProcess-customPass.ts#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 22-24: e2e/case/postProcess-customPass.ts#L22-L24
Added lines #L22 - L24 were not covered by tests
[warning] 33-33: e2e/case/postProcess-customPass.ts#L33
Added line #L33 was not covered by tests
[warning] 41-41: e2e/case/postProcess-customPass.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: e2e/case/postProcess-customPass.ts#L43
Added line #L43 was not covered by tests
[warning] 46-46: e2e/case/postProcess-customPass.ts#L46
Added line #L46 was not covered by tests
[warning] 48-50: e2e/case/postProcess-customPass.ts#L48-L50
Added lines #L48 - L50 were not covered by tests
[warning] 52-54: e2e/case/postProcess-customPass.ts#L52-L54
Added lines #L52 - L54 were not covered by tests
[warning] 56-56: e2e/case/postProcess-customPass.ts#L56
Added line #L56 was not covered by tests
[warning] 58-60: e2e/case/postProcess-customPass.ts#L58-L60
Added lines #L58 - L60 were not covered by tests
[warning] 62-66: e2e/case/postProcess-customPass.ts#L62-L66
Added lines #L62 - L66 were not covered by tests
[warning] 68-71: e2e/case/postProcess-customPass.ts#L68-L71
Added lines #L68 - L71 were not covered by tests
[warning] 73-74: e2e/case/postProcess-customPass.ts#L73-L74
Added lines #L73 - L74 were not covered by tests
[warning] 76-80: e2e/case/postProcess-customPass.ts#L76-L80
Added lines #L76 - L80 were not covered by tests
[warning] 82-85: e2e/case/postProcess-customPass.ts#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 87-89: e2e/case/postProcess-customPass.ts#L87-L89
Added lines #L87 - L89 were not covered by tests
e2e/config.ts
[warning] 214-218: e2e/config.ts#L214-L218
Added lines #L214 - L218 were not covered by tests
packages/core/src/RenderPipeline/BasicRenderPipeline.ts
[warning] 210-211: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L210-L211
Added lines #L210 - L211 were not covered by tests
[warning] 214-215: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L214-L215
Added lines #L214 - L215 were not covered by tests
packages/core/src/RenderPipeline/Blitter.ts
[warning] 30-80: packages/core/src/RenderPipeline/Blitter.ts#L30-L80
Added lines #L30 - L80 were not covered by tests
packages/core/src/RenderPipeline/OpaqueTexturePass.ts
[warning] 50-50: packages/core/src/RenderPipeline/OpaqueTexturePass.ts#L50
Added line #L50 was not covered by tests
🪛 Biome
packages/core/src/RenderPipeline/Blitter.ts
[error] 12-81: 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/PostProcess.ts
[error] 45-47: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (18)
e2e/case/postProcess-LDR-bloom-neutral.ts (1)
5-5: Necessary imports are correctly added.
The imported modules BloomEffect, PostProcess, and TonemappingEffect are appropriately used in the code.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 5-5: e2e/case/postProcess-LDR-bloom-neutral.ts#L5
Added line #L5 was not covered by tests
packages/core/src/postProcess/PostProcessEffect.ts (1)
28-28: Ensure safety when accessing nested properties
In the set enabled method, you access this.postProcess.scene._postProcessManager without null checks. If postProcess or scene can be null or undefined, it may lead to runtime errors.
Consider adding null checks to ensure these properties are valid:
const postProcessManager = this.postProcess?.scene?._postProcessManager;
if (!postProcessManager) {
// Handle the error or provide a default behavior
return;
}Alternatively, ensure that postProcess and scene are always initialized before this method is called.
packages/core/src/RenderPipeline/Blitter.ts (1)
30-80: Add unit tests for blitTexture method
The blitTexture method is a crucial part of the rendering pipeline. Currently, the added lines are not covered by tests, as indicated by the code coverage report. To ensure reliability and prevent regressions, please add unit tests to cover this method.
Would you like assistance in generating unit tests for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-80: packages/core/src/RenderPipeline/Blitter.ts#L30-L80
Added lines #L30 - L80 were not covered by tests
packages/core/src/postProcess/PostProcessManager.ts (3)
65-65: Constructor initialization is appropriate
The constructor correctly assigns the scene parameter to a public read-only property, ensuring that the PostProcessManager is associated with the correct scene context.
175-193: Ensure consistent use of _remainPassCount
The _remainPassCount is decremented inside the loop over _activePostProcessPasses. Verify that this variable accurately reflects the number of remaining passes, especially when passes might be skipped or conditional rendering is involved.
Run the following script to check for all modifications of _remainPassCount:
✅ Verification successful
Let me analyze the usage of _remainPassCount further by checking how it's used in the swap render target logic.
Let me try a different pattern to find these methods since the previous one didn't match.
_remainPassCount is used correctly and serves its intended purpose
The variable is used consistently throughout the post-processing pipeline:
- It's initialized with the total number of passes (
this._activePostProcessPasses.length) - Decremented after each pass is rendered
- Used in
_swapRT()and_initSwapRenderTarget()to determine if we're on the last pass, which affects render target swapping behavior:- When
_remainPassCount > 1: Uses swap render targets for intermediate passes - When
_remainPassCount <= 1: Uses the final destination render target
- When
The implementation correctly tracks remaining passes and influences render target management appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure _remainPassCount is used consistently.
rg '_remainPassCount' -A 2
Length of output: 2188
Script:
#!/bin/bash
# Check the implementation of _swapRT and _initSwapRenderTarget to understand the render target swapping logic
ast-grep --pattern 'private _swapRT() {
$$$
}'
ast-grep --pattern 'private _initSwapRenderTarget($_) {
$$$
}'
Length of output: 117
Script:
#!/bin/bash
# Search for _swapRT and _initSwapRenderTarget implementations with more context
rg "_swapRT\(" -A 10
rg "_initSwapRenderTarget\(" -A 10
Length of output: 3649
175-193: Add null checks for render targets before usage
In the render method, when assigning _srcRenderTarget and _destRenderTarget, ensure that these render targets are not null or undefined before proceeding. This prevents runtime errors if the render targets are not properly initialized.
Consider adding checks like:
if (!this._srcRenderTarget || !this._destRenderTarget) {
console.error("Render targets are not initialized.");
return;
}Additionally, you can automate the verification process:
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
24-24: Approved: Importing the Blitter module
The addition of the Blitter import is appropriate for the new texture blitting operations.
packages/core/src/postProcess/index.ts (1)
3-7: LGTM! Well-structured exports for the post-processing system.
The exports are logically organized, providing a clean public API for the post-processing system. The structure follows a clear hierarchy from base classes to specific implementations.
Let's verify the impact of these public API changes:
✅ Verification successful
Exports are properly structured and all referenced classes exist
The verification shows that:
- All exported classes have their corresponding implementation files in the
postProcessdirectory - The exports are being used consistently in the codebase, particularly in
Scene.ts - The post-processing system is well-integrated with test cases (e2e tests) and the loader system
- No references to legacy/old implementations were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usage of the old PostProcessManager
# and verify the new exports are properly referenced
# Check for any remaining references to _PostProcessManager
echo "Checking for old PostProcessManager references..."
rg -l "_PostProcessManager"
# Look for existing imports of the new exports to ensure they're used consistently
echo "Verifying usage of new exports..."
rg -A 2 "import.*from.*postProcess"
Length of output: 498
Script:
#!/bin/bash
# Let's verify the completeness of the exports and their implementations
# Check if all exported classes exist in their respective files
echo "Verifying PostProcess class..."
ast-grep --pattern 'class PostProcess'
echo "Verifying PostProcessEffect class..."
ast-grep --pattern 'class PostProcessEffect'
echo "Verifying PostProcessManager class..."
ast-grep --pattern 'class PostProcessManager'
echo "Verifying PostProcessPass class..."
ast-grep --pattern 'class PostProcessPass'
echo "Verifying PostProcessUberPass class..."
ast-grep --pattern 'class PostProcessUberPass'
# Check for any potential breaking changes by looking for references
echo "Checking all postProcess imports and usages..."
rg -l "postProcess" --type ts
Length of output: 1435
packages/core/src/postProcess/effects/TonemappingEffect.ts (2)
23-25: Verify the usage of _enableMacro after refactoring
The static macro seems to be a remnant from the previous implementation. Let's verify if it's still being used after the refactoring.
✅ Verification successful
The _enableMacro is actively used and correctly implemented
The macro is properly used in the codebase:
- It's enabled/disabled in PostProcessUberPass.ts based on the tonemapping instance
- The shader (UberPost.glsl) uses this macro to conditionally apply tonemapping with the correct mode
- The implementation follows the same pattern as other post-process effects like bloom
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of ENABLE_EFFECT_TONEMAPPING macro
rg -l "ENABLE_EFFECT_TONEMAPPING" --type ts
Length of output: 76
Script:
#!/bin/bash
# Search for usage of ENABLE_EFFECT_TONEMAPPING macro in all files
rg "ENABLE_EFFECT_TONEMAPPING"
# Search for files containing TonemappingEffect to understand the context
rg -l "TonemappingEffect"
# Look for shader files that might use this macro
fd -e glsl
Length of output: 6494
Script:
#!/bin/bash
# Check the content of UberPost.glsl to understand how the macro is used
cat packages/core/src/postProcess/shaders/UberPost.glsl
# Check PostProcessUberPass.ts to see how the macro is enabled
rg -A 5 "TonemappingEffect" packages/core/src/postProcess/PostProcessUberPass.ts
Length of output: 2713
28-30:
Consider shader macro implications of direct mode property
The previous implementation enabled a shader macro when the mode was set. With the property now being public and direct, we've lost this behavior. This could affect shader compilation if the macro was essential.
packages/core/src/postProcess/PostProcessPass.ts (2)
1-5: LGTM! Clean and well-organized imports
The imports are properly organized and include all necessary dependencies for the post-processing implementation.
12-40: Verify post-processing pass integration
Let's verify how this base class is utilized across the codebase to ensure consistent event handling and proper inheritance.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Post-processing pass integration verified successfully
The base class PostProcessPass is properly integrated and utilized across the codebase:
- Event handling is consistent with
PostProcessPassEvent.AfterUberbeing the default event in the base class - Two concrete implementations found:
PostProcessUberPass: Core implementation handling uber effects (bloom, tonemapping) with correct event ordering (AfterUber - 1)CustomPass: Example implementation in e2e tests demonstrating proper inheritance and usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all classes that extend PostProcessPass
ast-grep --pattern 'class $_ extends PostProcessPass {
$$$
}'
# Check for event usage consistency
rg -A 2 'PostProcessPassEvent\.' packages/core/src/
Length of output: 24000
packages/core/src/RenderPipeline/OpaqueTexturePass.ts (2)
50-50:
Add test coverage and validate type assertion
The modified blitting operation lacks test coverage. Additionally, the type assertion to Texture2D should be validated to ensure type safety.
Would you like assistance in generating test cases for this operation? The tests should cover:
- Successful texture blitting
- Error cases when invalid texture types are provided
- Validation of the output render target
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 50-50: packages/core/src/RenderPipeline/OpaqueTexturePass.ts#L50
Added line #L50 was not covered by tests
50-50: Verify consistent migration of blitting operations
Let's ensure all PipelineUtils.blitTexture calls have been migrated to use the new Blitter class.
✅ Verification successful
Let me check the actual implementation context of these Blitter.blitTexture calls to ensure they are being used consistently.
Let me check if there are any remaining PipelineUtils.blitTexture calls to ensure complete migration.
Migration to Blitter.blitTexture is complete and consistent
Based on the verification results, all texture blitting operations are now using the new Blitter.blitTexture API consistently across the codebase. No instances of the old PipelineUtils.blitTexture were found, and the new API is being used correctly in:
- RenderPipeline components (OpaqueTexturePass, BasicRenderPipeline)
- Post-processing (PostProcessUberPass)
- End-to-end tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of PipelineUtils.blitTexture
# and verify consistent migration to Blitter.blitTexture
echo "Checking for remaining PipelineUtils.blitTexture usage..."
rg "PipelineUtils\.blitTexture" -l
echo "\nVerifying Blitter.blitTexture adoption..."
rg "Blitter\.blitTexture" -l
Length of output: 487
Script:
#!/bin/bash
# Get implementation context for all Blitter.blitTexture usages
echo "=== Blitter.blitTexture usage in OpaqueTexturePass.ts ==="
rg "Blitter\.blitTexture" packages/core/src/RenderPipeline/OpaqueTexturePass.ts -B 2 -A 2
echo -e "\n=== Blitter.blitTexture usage in PostProcessUberPass.ts ==="
rg "Blitter\.blitTexture" packages/core/src/postProcess/PostProcessUberPass.ts -B 2 -A 2
echo -e "\n=== Blitter.blitTexture usage in BasicRenderPipeline.ts ==="
rg "Blitter\.blitTexture" packages/core/src/RenderPipeline/BasicRenderPipeline.ts -B 2 -A 2
echo -e "\n=== Blitter.blitTexture usage in e2e test ==="
rg "Blitter\.blitTexture" e2e/case/postProcess-customPass.ts -B 2 -A 2
Length of output: 2431
Script:
#!/bin/bash
# Search for any remaining PipelineUtils.blitTexture with context
rg "PipelineUtils\.blitTexture" -B 3 -A 3
Length of output: 43
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 50-50: packages/core/src/RenderPipeline/OpaqueTexturePass.ts#L50
Added line #L50 was not covered by tests
packages/core/src/Component.ts (1)
22-27: Verify usage in post-processing classes.
Let's verify that this property is actually needed by the post-processing classes.
✅ Verification successful
Property _phasedActive is actively used in post-processing effects
The property is indeed used in PostProcessEffect class for managing phased activation states. Found multiple references:
- Used in conditional checks to control effect activation/deactivation
- Synchronized with parent PostProcess's
_phasedActivestate - Critical for the phased activation logic in post-processing pipeline
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of _phasedActive in post-processing related files
# Expected: References in PostProcess/PostProcessEffect classes
# Search for PostProcess class definition and its usage of _phasedActive
ast-grep --pattern 'class PostProcess extends Component {
$$$
}'
# Search for direct references to _phasedActive in post-processing files
rg "_phasedActive" --type ts packages/core/src/postProcess/
Length of output: 10177
e2e/config.ts (1)
214-218: Configuration looks good, but needs documentation and tests.
The new customPass configuration entry follows the established pattern and correctly aligns with other PostProcess configurations. However, there are a few items to address:
- The PR objectives mention that documentation updates are missing.
- The static analysis indicates that these lines aren't covered by tests.
Let's verify the test file existence and documentation:
Would you like help with:
- Creating documentation for the new custom post-process feature?
- Setting up test coverage for these configuration lines?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 214-218: e2e/config.ts#L214-L218
Added lines #L214 - L218 were not covered by tests
packages/core/src/Scene.ts (1)
15-15: LGTM! Import changes align with post-processing feature
The import changes reflect the transition to a public PostProcessManager API and addition of PostProcessUberPass for the new post-processing workflow.
packages/core/src/Camera.ts (1)
68-72: LGTM: Type safety and consistency maintained.
The implementation maintains type safety through proper TypeScript types and follows established patterns in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (12)
e2e/case/postProcess-customPass.ts (1)
61-64: Consider optimizing the onRender methodThe current implementation creates engine variable in each render call. Consider moving it to a class member for better performance.
class CustomPass extends PostProcessPass { private _blitMaterial: Material; + private readonly _engine = this.engine; onRender(_, srcTexture: Texture2D, dst: RenderTarget): void { - const engine = this.engine; - Blitter.blitTexture(engine, srcTexture, dst, undefined, undefined, this._blitMaterial, 0); + Blitter.blitTexture(this._engine, srcTexture, dst, undefined, undefined, this._blitMaterial, 0); } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 61-65: e2e/case/postProcess-customPass.ts#L61-L65
Added lines #L61 - L65 were not covered by testspackages/core/src/postProcess/PostProcessManager.ts (4)
14-33: Enhance class documentation with property descriptionsWhile the class has good structure, consider adding JSDoc descriptions for the important properties, especially:
_postProcessEffectInstanceMap_defaultPostProcessEffectMap_activePostProcesses
71-74: Add input validation for pass parameterConsider adding null/undefined checks for the
passparameter to prevent potential runtime issues.addPostProcessPass(pass: PostProcessPass) { + if (!pass) { + throw new Error("PostProcessPass cannot be null or undefined"); + } this._activePostProcessPasses.push(pass); this._postProcessPassNeedSorting = true; }
133-169: Consider breaking down the update method for better maintainabilityThe update method has high cyclomatic complexity with multiple nested loops and conditions. Consider extracting the effect processing logic into a separate method.
update(postProcessMask: Layer) { this._resetDefaultValue(); this._sortPostProcess(); this._sortPostProcessPass(); + this._processEffects(postProcessMask); } +private _processEffects(postProcessMask: Layer): void { for (let i = 0; i < this._activePostProcesses.length; i++) { // ... existing effect processing logic ... } }
214-229: Consider making texture parameters configurableThe texture format, wrap mode, and filter mode are currently hardcoded in
_initSwapRenderTarget. Consider making these configurable to support different post-processing requirements.+interface SwapRenderTargetOptions { + wrapMode?: TextureWrapMode; + filterMode?: TextureFilterMode; +} -private _initSwapRenderTarget(camera: Camera) { +private _initSwapRenderTarget(camera: Camera, options?: SwapRenderTargetOptions) { // ... const swapRenderTarget = PipelineUtils.recreateRenderTargetIfNeeded( // ... - TextureWrapMode.Clamp, - TextureFilterMode.Bilinear + options?.wrapMode ?? TextureWrapMode.Clamp, + options?.filterMode ?? TextureFilterMode.Bilinear ); // ... }packages/core/src/Scene.ts (2)
83-88: Enhance getter documentationThe documentation for
postProcessManagergetter could be more descriptive. Consider adding details about its purpose, usage, and any important considerations./** * Get the post process manager. + * @remarks Manages post-processing effects like Bloom and Tonemapping for the scene. + * @returns The post process manager instance that handles all post-processing effects. */
81-88: Address codebase-wide PostProcessManager access patternsWhile this change properly encapsulates the PostProcessManager through a getter, there are inconsistent access patterns across the codebase that need to be addressed:
- External files directly accessing internal properties
- Mixed usage of public/private access
Consider:
- Auditing all PostProcessManager access points
- Creating a migration plan for external files
- Establishing consistent access patterns through public API
packages/core/src/postProcess/PostProcess.ts (2)
37-44: Add unit tests for the 'priority' propertyThe
prioritygetter and setter (lines 37-44) are not covered by tests, as indicated by the static analysis tool. Please consider adding unit tests to ensure this property functions as expected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-39: packages/core/src/postProcess/PostProcess.ts#L38-L39
Added lines #L38 - L39 were not covered by tests
51-61: Consider optimizing effect retrieval for better performanceThe
getEffectmethod iterates over the_effectsarray to find the desired effect. If the number of effects grows large, this linear search could impact performance.Consider maintaining a map or using a more efficient data structure to store effects by their type for faster retrieval:
// Example using a Map private _effectsMap = new Map<typeof PostProcessEffect, PostProcessEffect>(); addEffect<T extends typeof PostProcessEffect>(type: T): InstanceType<T> | undefined { if (this._effectsMap.has(type)) { Logger.error(`effect "${type.name}" already exists in the PostProcess.`); return; } const effect = new type(this) as InstanceType<T>; this._effects.push(effect); this._effectsMap.set(type, effect); effect._setActive(true); return effect; } getEffect<T extends typeof PostProcessEffect>(type: T): InstanceType<T> | undefined { return this._effectsMap.get(type) as InstanceType<T> | undefined; } removeEffect<T extends typeof PostProcessEffect>(type: T): InstanceType<T> | undefined { const effect = this._effectsMap.get(type) as InstanceType<T> | undefined; if (effect) { this._effects.splice(this._effects.indexOf(effect), 1); this._effectsMap.delete(type); effect._setActive(false); return effect; } }packages/core/src/postProcess/PostProcessUberPass.ts (3)
98-233: Refactor '_setupBloom' method for better maintainabilityThe
_setupBloommethod is lengthy and handles multiple responsibilities, including shader data updates, render target management, and rendering operations. Consider splitting it into smaller, focused methods to enhance readability and maintainability.
109-109: Parameterize the 'thresholdKnee' valueThe
thresholdKneeis currently hardcoded as half ofthresholdLinear. MakingthresholdKneea configurable parameter would allow for greater flexibility in fine-tuning the bloom effect.
121-126: Reduce code duplication when setting the high-quality filtering macroThe macro
BloomEffect._hqMacrois enabled or disabled for bothbloomShaderDataanduberShaderDatawith identical code blocks. Consider creating a helper function to set this macro on both shader data instances simultaneously, reducing code duplication.Apply this diff to introduce a helper method:
+ private _setHighQualityFiltering(shaderDatas: ShaderData[], enabled: boolean): void { + for (const shaderData of shaderDatas) { + if (enabled) { + shaderData.enableMacro(BloomEffect._hqMacro); + } else { + shaderData.disableMacro(BloomEffect._hqMacro); + } + } + } // Update shaderData if (highQualityFiltering) { - bloomShaderData.enableMacro(BloomEffect._hqMacro); - uberShaderData.enableMacro(BloomEffect._hqMacro); + this._setHighQualityFiltering([bloomShaderData, uberShaderData], true); } else { - bloomShaderData.disableMacro(BloomEffect._hqMacro); - uberShaderData.disableMacro(BloomEffect._hqMacro); + this._setHighQualityFiltering([bloomShaderData, uberShaderData], false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
e2e/case/postProcess-customPass.ts(1 hunks)packages/core/src/Camera.ts(2 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(2 hunks)packages/core/src/Scene.ts(4 hunks)packages/core/src/postProcess/PostProcess.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/PostProcessUberPass.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/Camera.ts
- packages/core/src/postProcess/PostProcessEffect.ts
- packages/core/src/postProcess/PostProcessPass.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
e2e/case/postProcess-customPass.ts
[warning] 19-20: e2e/case/postProcess-customPass.ts#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 22-24: e2e/case/postProcess-customPass.ts#L22-L24
Added lines #L22 - L24 were not covered by tests
[warning] 33-33: e2e/case/postProcess-customPass.ts#L33
Added line #L33 was not covered by tests
[warning] 41-41: e2e/case/postProcess-customPass.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: e2e/case/postProcess-customPass.ts#L43
Added line #L43 was not covered by tests
[warning] 46-48: e2e/case/postProcess-customPass.ts#L46-L48
Added lines #L46 - L48 were not covered by tests
[warning] 50-53: e2e/case/postProcess-customPass.ts#L50-L53
Added lines #L50 - L53 were not covered by tests
[warning] 55-55: e2e/case/postProcess-customPass.ts#L55
Added line #L55 was not covered by tests
[warning] 57-59: e2e/case/postProcess-customPass.ts#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-65: e2e/case/postProcess-customPass.ts#L61-L65
Added lines #L61 - L65 were not covered by tests
[warning] 67-70: e2e/case/postProcess-customPass.ts#L67-L70
Added lines #L67 - L70 were not covered by tests
[warning] 72-73: e2e/case/postProcess-customPass.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 75-79: e2e/case/postProcess-customPass.ts#L75-L79
Added lines #L75 - L79 were not covered by tests
[warning] 81-84: e2e/case/postProcess-customPass.ts#L81-L84
Added lines #L81 - L84 were not covered by tests
[warning] 86-88: e2e/case/postProcess-customPass.ts#L86-L88
Added lines #L86 - L88 were not covered by tests
packages/core/src/RenderPipeline/BasicRenderPipeline.ts
[warning] 210-211: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L210-L211
Added lines #L210 - L211 were not covered by tests
[warning] 214-215: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L214-L215
Added lines #L214 - L215 were not covered by tests
packages/core/src/postProcess/PostProcess.ts
[warning] 38-39: packages/core/src/postProcess/PostProcess.ts#L38-L39
Added lines #L38 - L39 were not covered by tests
🔇 Additional comments (9)
e2e/case/postProcess-customPass.ts (2)
22-41: Review shader implementation
The fragment shader's use of .rrra swizzling (copying red channel to RGB) creates a grayscale effect. Consider documenting this intentional behavior or adjusting if this wasn't the intended effect.
Additionally, the intensity uniform issue from the previous review remains valid - the uniform is set in the class but not declared or used in the shader.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-24: e2e/case/postProcess-customPass.ts#L22-L24
Added lines #L22 - L24 were not covered by tests
[warning] 33-33: e2e/case/postProcess-customPass.ts#L33
Added line #L33 was not covered by tests
[warning] 41-41: e2e/case/postProcess-customPass.ts#L41
Added line #L41 was not covered by tests
67-88: Add error handling and tests
The initialization lacks error handling for resource array access and effect creation. Additionally, static analysis indicates this code isn't covered by tests.
Consider:
- Adding null checks for resources
- Wrapping effect creation in try-catch blocks
- Adding comprehensive tests for the post-process setup
Would you like me to help create test cases for this implementation?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-70: e2e/case/postProcess-customPass.ts#L67-L70
Added lines #L67 - L70 were not covered by tests
[warning] 72-73: e2e/case/postProcess-customPass.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 75-79: e2e/case/postProcess-customPass.ts#L75-L79
Added lines #L75 - L79 were not covered by tests
[warning] 81-84: e2e/case/postProcess-customPass.ts#L81-L84
Added lines #L81 - L84 were not covered by tests
[warning] 86-88: e2e/case/postProcess-customPass.ts#L86-L88
Added lines #L86 - L88 were not covered by tests
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (3)
24-24: LGTM!
The Blitter import is appropriately added to support the new texture blitting functionality.
200-204: LGTM! Post-process rendering logic is well-structured.
The code properly checks for both camera post-process enablement and active effects before proceeding with post-process rendering, ensuring efficient resource usage.
205-215: 🛠️ Refactor suggestion
Add error handling and improve type safety in texture blitting.
The texture blitting implementation needs some improvements:
- Add error handling for potential null/undefined values
- Add type safety checks before type assertion
- Add test coverage for the texture blitting code path
Consider applying this diff:
} else {
postProcessManager._releaseSwapRenderTarget();
if (internalColorTarget) {
internalColorTarget._blitRenderTarget();
+ const colorTexture = internalColorTarget.getColorTexture(0);
+ if (!colorTexture) {
+ throw new Error("Failed to get color texture from internal color target");
+ }
+ if (!(colorTexture instanceof Texture2D)) {
+ throw new Error("Color texture is not an instance of Texture2D");
+ }
Blitter.blitTexture(
engine,
- <Texture2D>internalColorTarget.getColorTexture(0),
+ colorTexture,
cameraRenderTarget,
0,
camera.viewport
);
}
}Please ensure to add test coverage for this code path.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 210-211: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L210-L211
Added lines #L210 - L211 were not covered by tests
[warning] 214-215: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L214-L215
Added lines #L214 - L215 were not covered by tests
packages/core/src/Scene.ts (1)
297-298: Verify PostProcessUberPass initialization impact
This is a breaking change that adds default post-processing initialization. Ensure all existing scenes and tests handle this new default behavior correctly.
Consider adding error handling:
- const uberPass = new PostProcessUberPass(this._postProcessManager);
- this._postProcessManager.addPostProcessPass(uberPass);
+ try {
+ const uberPass = new PostProcessUberPass(this._postProcessManager);
+ this._postProcessManager.addPostProcessPass(uberPass);
+ } catch (error) {
+ Logger.warn("Failed to initialize PostProcessUberPass:", error);
+ // Consider if fallback behavior is needed
+ }packages/core/src/postProcess/PostProcessUberPass.ts (3)
135-141: Ensure 'iterations' calculation handles small viewport sizes properly
222-231: Prevent potential division by zero when computing 'dirtRatio'
92-96: Proper resource cleanup in 'destroy' method
The destroy method correctly releases allocated resources by disposing of render targets and materials, preventing memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (14)
e2e/case/postProcess-customPass.ts (3)
1-4: Enhance documentation with usage examplesWhile the basic documentation is present, consider adding:
- Example usage of the CustomPass
- Description of the post-processing effect
- Parameters and their effects
22-41: Document shader effect and improve implementationThe shader appears to implement a grayscale effect using only the red channel, but this isn't clearly documented. Consider:
- Adding comments explaining the intended visual effect
- Using proper grayscale conversion weights if a grayscale effect is intended
` varying vec2 v_uv; uniform sampler2D renderer_BlitTexture; + // Convert to grayscale using standard luminance weights void main(){ - gl_FragColor = texture2D(renderer_BlitTexture, v_uv).rrra; + vec4 color = texture2D(renderer_BlitTexture, v_uv); + float gray = dot(color.rgb, vec3(0.299, 0.587, 0.114)); + gl_FragColor = vec4(gray, gray, gray, color.a); } `🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-24: e2e/case/postProcess-customPass.ts#L22-L24
Added lines #L22 - L24 were not covered by tests
[warning] 33-33: e2e/case/postProcess-customPass.ts#L33
Added line #L33 was not covered by tests
[warning] 41-41: e2e/case/postProcess-customPass.ts#L41
Added line #L41 was not covered by tests
67-89: Add cleanup handling for resourcesThe initialization code should handle cleanup to prevent memory leaks. Consider:
- Disposing of the custom pass when no longer needed
- Cleaning up the post-process chain on scene cleanup
Example cleanup handler:
const cleanup = () => { customPass.destroy(); postProcessManager.removePostProcessPass(customPass); globalPostProcessEntity.destroy(); };🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-71: e2e/case/postProcess-customPass.ts#L67-L71
Added lines #L67 - L71 were not covered by tests
[warning] 73-74: e2e/case/postProcess-customPass.ts#L73-L74
Added lines #L73 - L74 were not covered by tests
[warning] 76-80: e2e/case/postProcess-customPass.ts#L76-L80
Added lines #L76 - L80 were not covered by tests
[warning] 82-85: e2e/case/postProcess-customPass.ts#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 87-89: e2e/case/postProcess-customPass.ts#L87-L89
Added lines #L87 - L89 were not covered by testspackages/core/src/postProcess/PostProcessManager.ts (3)
39-61: Consider refactoring for better maintainability and performanceThe current implementation has nested loops and multiple conditions that make it harder to maintain. Consider extracting the effect checking logic into a separate method and using array methods for better readability.
get hasActiveEffect(): boolean { if (!this._activeStateChangeFlag) { return this._hasActiveEffect; } this._activeStateChangeFlag = false; - if (this._activePostProcessPasses.length) { - for (let i = 0; i < this._activePostProcesses.length; i++) { - const postProcess = this._activePostProcesses[i]; - if (postProcess.enabled) { - const effects = postProcess._effects; - for (let j = 0; j < effects.length; j++) { - if (effects[j].enabled) { - this._hasActiveEffect = true; - return true; - } - } - } - } - } + this._hasActiveEffect = this._activePostProcessPasses.length > 0 && + this._activePostProcesses.some(process => + process.enabled && process._effects.some(effect => effect.enabled) + ); return this._hasActiveEffect; }
247-271: Consider making texture parameters configurableThe texture format, wrap mode, and filter mode are currently hardcoded. Consider making these configurable through constructor options or a configuration object for better flexibility.
interface PostProcessManagerConfig { textureWrapMode?: TextureWrapMode; textureFilterMode?: TextureFilterMode; depthFormat?: TextureFormat; }
70-73: Improve method documentationConsider adding more detailed JSDoc comments for public and internal methods, including:
- Parameter descriptions
- Return type descriptions
- Usage examples where appropriate
Also applies to: 90-92, 104-106, 113-115, 132-134, 155-157, 162-164
packages/core/src/postProcess/PostProcessUberPass.ts (3)
23-59: Add JSDoc documentation for the class and its propertiesThe class lacks comprehensive documentation. Consider adding:
- Class-level documentation explaining the purpose and usage
- Property documentation for
_uberMaterialand_bloomMaterial- Documentation for shader parameters and their effects
146-175: Add bounds checking for mipmap chain creationThe mipmap chain creation could be more robust:
- Validate that
mipCountdoesn't exceed the maximum supported by the hardware- Add checks for minimum texture dimensions
- Consider using
Math.max(1, mipWidth >> 1)instead ofMath.floor(mipWidth / 2)for consistency
234-250: Consider clearing texture references before destroyingTo prevent potential memory leaks, consider clearing texture references from shader data before destroying the render targets:
uberShaderData.setTexture(BloomEffect._bloomTextureProp, null); bloomShaderData.setTexture(BloomEffect._lowMipTextureProp, null);packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2)
200-204: Document post-processing flow changesThe post-processing implementation has been significantly modified. Consider adding inline documentation to explain:
- The relationship between
internalColorTargetandcameraRenderTarget- The conditions that trigger post-processing
- The expected visual outcomes
205-215: Consider optimizing texture operationsThe current implementation performs multiple texture operations that could impact performance:
- Release of swap render target
- Blit render target
- Texture blitting with viewport
Consider:
- Combining operations where possible
- Adding early-exit conditions
- Caching frequently accessed properties
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 210-211: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L210-L211
Added lines #L210 - L211 were not covered by tests
[warning] 214-215: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L214-L215
Added lines #L214 - L215 were not covered by testspackages/core/src/Scene.ts (2)
296-298: Consider lazy initialization of PostProcessUberPassThe PostProcessUberPass is always created in the constructor, which could impact performance for scenes that don't require post-processing.
Consider:
- Lazy initialization of the UberPass when first needed
- Adding documentation for this new feature
- Adding configuration options to control when the pass is created
- const uberPass = new PostProcessUberPass(engine); - this._postProcessManager.addPostProcessPass(uberPass); + // Initialize post-process pass only when needed + if (this._engine.options.enablePostProcess) { + const uberPass = new PostProcessUberPass(engine); + this._postProcessManager.addPostProcessPass(uberPass); + }
83-85: Documentation needed for post-process workflowThe PR objectives indicate that documentation updates are missing. Please add JSDoc comments to explain:
- The purpose and functionality of the post-process manager
- Available post-processing effects
- Usage examples and best practices
/** * Get the post process manager. + * @remarks + * The post process manager handles the rendering pipeline's post-processing effects. + * It supports effects such as Bloom and Tonemapping through the PostProcessUberPass. + * @example + * ```typescript + * // Add a bloom effect to the scene + * const bloomEffect = new BloomEffect(); + * scene.postProcessManager.addEffect(bloomEffect); + * ``` */packages/core/src/postProcess/PostProcess.ts (1)
38-39: Increase test coverage for theprioritygetterStatic analysis indicates that the
get priority()method is not covered by tests. To maintain robust code quality and ensure future changes do not introduce regressions, consider adding unit tests for this getter method.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-39: packages/core/src/postProcess/PostProcess.ts#L38-L39
Added lines #L38 - L39 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
e2e/case/postProcess-customPass.ts(1 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(2 hunks)packages/core/src/Scene.ts(4 hunks)packages/core/src/postProcess/PostProcess.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/PostProcessUberPass.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/postProcess/PostProcessEffect.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
e2e/case/postProcess-customPass.ts
[warning] 19-20: e2e/case/postProcess-customPass.ts#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 22-24: e2e/case/postProcess-customPass.ts#L22-L24
Added lines #L22 - L24 were not covered by tests
[warning] 33-33: e2e/case/postProcess-customPass.ts#L33
Added line #L33 was not covered by tests
[warning] 41-41: e2e/case/postProcess-customPass.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: e2e/case/postProcess-customPass.ts#L43
Added line #L43 was not covered by tests
[warning] 46-48: e2e/case/postProcess-customPass.ts#L46-L48
Added lines #L46 - L48 were not covered by tests
[warning] 50-53: e2e/case/postProcess-customPass.ts#L50-L53
Added lines #L50 - L53 were not covered by tests
[warning] 55-55: e2e/case/postProcess-customPass.ts#L55
Added line #L55 was not covered by tests
[warning] 57-59: e2e/case/postProcess-customPass.ts#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-65: e2e/case/postProcess-customPass.ts#L61-L65
Added lines #L61 - L65 were not covered by tests
[warning] 67-71: e2e/case/postProcess-customPass.ts#L67-L71
Added lines #L67 - L71 were not covered by tests
[warning] 73-74: e2e/case/postProcess-customPass.ts#L73-L74
Added lines #L73 - L74 were not covered by tests
[warning] 76-80: e2e/case/postProcess-customPass.ts#L76-L80
Added lines #L76 - L80 were not covered by tests
[warning] 82-85: e2e/case/postProcess-customPass.ts#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 87-89: e2e/case/postProcess-customPass.ts#L87-L89
Added lines #L87 - L89 were not covered by tests
packages/core/src/RenderPipeline/BasicRenderPipeline.ts
[warning] 210-211: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L210-L211
Added lines #L210 - L211 were not covered by tests
[warning] 214-215: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L214-L215
Added lines #L214 - L215 were not covered by tests
packages/core/src/postProcess/PostProcess.ts
[warning] 38-39: packages/core/src/postProcess/PostProcess.ts#L38-L39
Added lines #L38 - L39 were not covered by tests
🪛 Biome
packages/core/src/postProcess/PostProcessPass.ts
[error] 54-54: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 56-56: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 80-80: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (12)
e2e/case/postProcess-customPass.ts (4)
46-48: Remove unused intensity setter
The intensity setter is present but the uniform isn't declared or used in the shader. Either remove the setter or implement the intensity functionality in the shader.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-48: e2e/case/postProcess-customPass.ts#L46-L48
Added lines #L46 - L48 were not covered by tests
61-64: Add error handling for texture operations
The onRender method should validate input parameters and handle potential errors during texture operations.
Consider adding checks:
onRender(_, srcTexture: Texture2D, dst: RenderTarget): void {
if (!srcTexture || !dst) {
console.warn('Invalid source texture or destination render target');
return;
}
const engine = this.engine;
Blitter.blitTexture(engine, srcTexture, dst, undefined, undefined, this._blitMaterial, 0);
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 61-65: e2e/case/postProcess-customPass.ts#L61-L65
Added lines #L61 - L65 were not covered by tests
80-80: Remove redundant tonemapping mode assignment
The tonemapping mode is set twice with different values (ACES and Neutral). Remove one of the assignments to avoid confusion.
Also applies to: 85-85
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-80: e2e/case/postProcess-customPass.ts#L76-L80
Added lines #L76 - L80 were not covered by tests
1-89: Add test coverage for post-processing functionality
Static analysis indicates lack of test coverage. Consider adding tests for:
- CustomPass initialization and destruction
- Shader effect verification
- Post-process chain setup and cleanup
- Error cases and edge conditions
Would you like assistance in generating test cases?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-20: e2e/case/postProcess-customPass.ts#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 22-24: e2e/case/postProcess-customPass.ts#L22-L24
Added lines #L22 - L24 were not covered by tests
[warning] 33-33: e2e/case/postProcess-customPass.ts#L33
Added line #L33 was not covered by tests
[warning] 41-41: e2e/case/postProcess-customPass.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: e2e/case/postProcess-customPass.ts#L43
Added line #L43 was not covered by tests
[warning] 46-48: e2e/case/postProcess-customPass.ts#L46-L48
Added lines #L46 - L48 were not covered by tests
[warning] 50-53: e2e/case/postProcess-customPass.ts#L50-L53
Added lines #L50 - L53 were not covered by tests
[warning] 55-55: e2e/case/postProcess-customPass.ts#L55
Added line #L55 was not covered by tests
[warning] 57-59: e2e/case/postProcess-customPass.ts#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-65: e2e/case/postProcess-customPass.ts#L61-L65
Added lines #L61 - L65 were not covered by tests
[warning] 67-71: e2e/case/postProcess-customPass.ts#L67-L71
Added lines #L67 - L71 were not covered by tests
[warning] 73-74: e2e/case/postProcess-customPass.ts#L73-L74
Added lines #L73 - L74 were not covered by tests
[warning] 76-80: e2e/case/postProcess-customPass.ts#L76-L80
Added lines #L76 - L80 were not covered by tests
[warning] 82-85: e2e/case/postProcess-customPass.ts#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 87-89: e2e/case/postProcess-customPass.ts#L87-L89
Added lines #L87 - L89 were not covered by tests
packages/core/src/postProcess/PostProcessManager.ts (5)
15-34: LGTM! Well-structured class definition with clear member organization.
The class structure follows good practices with:
- Clear separation between internal and private members
- Well-defined state management properties
- Appropriate use of TypeScript visibility modifiers
194-207: Fix parameter order in lerp method call
The previous review comment about incorrect parameter order in the lerp method call is still valid. The current implementation may lead to incorrect reset behavior.
241-241: The previous review comment about the misplaced TODO comment is still valid. This comment appears unrelated to post-processing functionality.
286-288: The previous review comment about adding null checks when accessing color texture is still valid. The current implementation could lead to runtime errors.
135-153: The previous review comment about adding error handling for render target operations is still valid. Consider implementing proper error handling and cleanup logic.
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2)
24-24: LGTM: Import addition is appropriate
The Blitter import aligns with its usage for texture operations later in the code.
208-214: Verify viewport transformation in Blitter.blitTexture
Ensure that the viewport transformation in the Blitter operation correctly handles:
- Different coordinate systems
- Resolution changes
- MSAA configurations
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 210-211: packages/core/src/RenderPipeline/BasicRenderPipeline.ts#L210-L211
Added lines #L210 - L211 were not covered by tests
packages/core/src/Scene.ts (1)
15-15:
Breaking change detected in PostProcessManager import
The change from _PostProcessManager to PostProcessManager represents a breaking change that affects external modules accessing the post-process functionality.
Please ensure all dependent modules are updated to use the new PostProcessManager class name.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
packages/core/src/RenderPipeline/Blitter.ts (3)
12-14: Enhance class description and usage details.Currently, the class JSDoc is brief. Consider adding usage examples, parameter details in the method doc comment, and potential edge cases in the class-level comment to help developers understand how to leverage Blitter for texture operations.
15-90: Consider avoiding a static-only class.Static analysis tools warn about classes with only static members, as they can be replaced by standalone utility functions in many cases. While this class is fine to ship if needed, consider refactoring the code to a simple module of functions if future expansions are not anticipated.
🧰 Tools
🪛 Biome (1.9.4)
[error] 15-90: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
34-34: Unused parameter noted.The JSDoc references the “flipYOfSource” parameter, but it’s not referenced in the function body. If this parameter is deprecated or replaced by the “sourceScaleOffset” logic, remove it from the JSDoc to avoid confusion.
/** * @param flipYOfSource - Whether flip Y axis of source texture */ -static blitTexture( +/** + * @deprecated flipYOfSource is no longer used. Remove if unneeded. + */ static blitTexture(packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2)
51-51: Document the purpose of _sourceScaleOffset.While the variable name is self-descriptive, adding a short explanation or doc comment for _sourceScaleOffset would help clarify how this offset influences the final texture coordinates.
289-304: Add integration tests for post-processing flows.The newly introduced (or modified) post-processing logic at lines 289-304 is lacking test coverage. Testing is crucial to ensure that:
• Pass-through rendering works when post-processing is disabled.
• The postProcessManager._render(...) path is exercised under various scenarios.
• Correct results appear when internalColorTarget is used.Would you like help creating test cases for these workflows?
packages/core/src/postProcess/PostProcess.ts (4)
14-14: Add validation for Layer propertyConsider adding validation to ensure the layer value is within valid bounds when it's modified.
- layer = Layer.Layer0; + private _layer = Layer.Layer0; + + get layer(): number { + return this._layer; + } + + set layer(value: number) { + if (value < Layer.Layer0 || value > Layer.Layer31) { + Logger.warn("Layer value must be between Layer0 and Layer31"); + return; + } + this._layer = value; + }
19-19: Add validation for blendDistance propertyThe blendDistance property should validate against negative values as they wouldn't make sense in a blending context.
- blendDistance = 0; + private _blendDistance = 0; + + get blendDistance(): number { + return this._blendDistance; + } + + set blendDistance(value: number) { + if (value < 0) { + Logger.warn("blendDistance cannot be negative"); + return; + } + this._blendDistance = value; + }
41-41: Enhance warning message for better clarityThe current warning message could be more descriptive to help developers understand why physics is required for local post-processing.
- Logger.warn("Only support local PostProcess in physics enabled Scenes."); + Logger.warn("Local PostProcess requires physics for spatial calculations. Please enable physics in the Scene before setting isGlobal to false.");
7-10: Enhance class documentation with performance considerationsThe class documentation should include information about performance implications and cleanup patterns.
/** * Post Process component can be used for global or local post-processing. + * @remarks + * Performance considerations: + * - Adding multiple effects can impact rendering performance + * - Local post-processing has additional overhead due to physics calculations + * - Effects are processed in priority order + * + * Cleanup: + * - Effects are automatically cleaned up when the component is destroyed + * - Manual cleanup can be performed by removing effects individually */packages/core/src/postProcess/PostProcessManager.ts (3)
50-50: Remove or clarify the TODO commentThe TODO comment about pass and effect waste is unclear and seems incomplete. Consider either removing it or providing a clearer explanation of the potential optimization.
49-68: Simplify active state checking logicThe active state checking logic could be simplified by combining the conditions and removing the intermediate flag.
Consider this refactoring:
_isValid(): boolean { - if (this.scene.engine._activePostProcessPasses.length === 0) { - return false; - } - - if (!this._isValidChangeFlag) { - return this._isActive; - } - - this._isActive = this._activePostProcesses.some( - (postProcess) => postProcess.enabled && postProcess._effects.some((effect) => effect.enabled) - ); - this._isValidChangeFlag = false; - - return this._isActive; + if (!this._isValidChangeFlag) { + return this._isActive; + } + + this._isValidChangeFlag = false; + return (this._isActive = + this.scene.engine._activePostProcessPasses.length > 0 && + this._activePostProcesses.some( + (postProcess) => postProcess.enabled && postProcess._effects.some((effect) => effect.enabled) + ) + ); }
129-136: Improve null checks in resource cleanupThe resource cleanup could be more robust by adding explicit null checks for the color texture.
Apply this diff:
_releaseSwapRenderTarget(): void { const swapRenderTarget = this._swapRenderTarget; if (swapRenderTarget) { - swapRenderTarget.getColorTexture(0)?.destroy(true); + const colorTexture = swapRenderTarget.getColorTexture(0); + if (colorTexture) { + colorTexture.destroy(true); + } swapRenderTarget.destroy(true); this._swapRenderTarget = null; } }packages/core/src/Engine.ts (2)
280-282: Consider enhancing UberPass initialization.While the initialization is correctly placed in the constructor, consider:
- Adding error handling for the addPostProcessPass call
- Storing the UberPass instance in a class property for later reference
private _basicResources: BasicResources; + /** @internal */ + private _uberPass: PostProcessUberPass; constructor(canvas: Canvas, hardwareRenderer: IHardwareRenderer, configuration: EngineConfiguration) { // ... existing code ... const uberPass = new PostProcessUberPass(this); + this._uberPass = uberPass; try { this.addPostProcessPass(uberPass); } catch (error) { Logger.error("Failed to initialize UberPass:", error); throw error; } }
434-444: Enhance error message in addPostProcessPass.The error message could be more descriptive to help developers understand the issue better.
if (pass.engine !== this) { - throw "The pass is not belong to this engine."; + throw new Error(`PostProcessPass belongs to a different engine instance. Expected: ${this}, Found: ${pass.engine}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/core/src/Camera.ts(2 hunks)packages/core/src/Engine.ts(7 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(5 hunks)packages/core/src/RenderPipeline/Blitter.ts(1 hunks)packages/core/src/postProcess/PostProcess.ts(1 hunks)packages/core/src/postProcess/PostProcessEffect.ts(1 hunks)packages/core/src/postProcess/PostProcessManager.ts(1 hunks)tests/src/core/postProcess/PostProcess.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/Camera.ts
- packages/core/src/postProcess/PostProcessEffect.ts
- tests/src/core/postProcess/PostProcess.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/RenderPipeline/Blitter.ts
[error] 15-90: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (11)
packages/core/src/RenderPipeline/Blitter.ts (1)
61-61: Check for zero dimension to avoid division by zero.
As previously mentioned, calling “1 / source.width” and “1 / source.height” will fail for zero-dimension textures. Please add dimension checks or validations to avoid potential division by zero errors.
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
231-235: Validate flipping logic and clarify usage.
The code sets sourceScaleOffset.y and .w for Y-axis flipping. Ensure these assignments match all possible use cases, especially if more flips or transformations are needed in future. A short note explaining the sign logic would be helpful.
packages/core/src/postProcess/PostProcess.ts (2)
70-80: Adjust return type to handle potential 'undefined'
The method can return undefined when no effect is found, but the return type doesn't reflect this possibility.
127-148: Optimize scene checks in lifecycle methods
The methods have redundant scene checks that can be simplified.
packages/core/src/postProcess/PostProcessManager.ts (3)
287-289:
Add null check for color texture
The _getCurrentSourceTexture method assumes the color texture exists. Add a null check to prevent potential runtime errors.
Apply this diff:
private _getCurrentSourceTexture(): Texture2D {
- return this._currentSourceRenderTarget.getColorTexture(0) as Texture2D;
+ const texture = this._currentSourceRenderTarget.getColorTexture(0);
+ if (!texture) {
+ throw new Error("Current source render target does not have a color texture");
+ }
+ return texture as Texture2D;
}Likely invalid or redundant comment.
105-124: 🛠️ Refactor suggestion
Add error handling to render method
The render method performs critical operations without proper error handling. Consider implementing a try-catch block to ensure proper cleanup of resources in case of errors.
Apply this diff:
_render(camera: Camera, srcRenderTarget: RenderTarget, destRenderTarget: RenderTarget): void {
+ try {
const engine = this.scene.engine;
this._srcRenderTarget = srcRenderTarget;
this._destRenderTarget = destRenderTarget;
srcRenderTarget._blitRenderTarget();
this._update(camera);
this._remainActivePassCount = engine._activePostProcessPasses.length;
this._initSwapRenderTarget(camera);
for (let i = 0; i < engine._activePostProcessPasses.length; i++) {
const pass = engine._activePostProcessPasses[i];
pass.onRender(camera, this._getCurrentSourceTexture(), this._currentDestRenderTarget);
this._remainActivePassCount--;
this._swapRT();
}
+ } catch (error) {
+ this._releaseSwapRenderTarget();
+ throw error;
+ }
}Likely invalid or redundant comment.
163-246: 🛠️ Refactor suggestion
Refactor complex update logic into smaller methods
The _update method is too complex with multiple responsibilities. Consider breaking it down into smaller, focused methods for better maintainability and testing.
Consider extracting these methods:
_updateLocalPostProcessfor handling non-global post-process_updateEffectsfor handling effect updates_calculateInterpFactorfor distance calculations
Example refactoring:
private _update(camera: Camera): void {
this._resetDefaultValue();
this._sortActivePostProcess();
+ this._updatePostProcesses(camera);
}
+private _updatePostProcesses(camera: Camera): void {
for (let i = 0; i < this._activePostProcesses.length; i++) {
const postProcess = this._activePostProcesses[i];
if (!postProcess.enabled || !(camera.postProcessMask & postProcess.layer)) {
continue;
}
const interpFactor = postProcess.isGlobal ? 1 : this._calculateInterpFactor(postProcess, camera);
if (interpFactor === null) continue;
this._updateEffects(postProcess._effects, interpFactor);
}
}
+private _calculateInterpFactor(postProcess: PostProcess, camera: Camera): number | null {
const shapes = this._getColliderShapes(postProcess);
if (!shapes.length) {
Logger.warn(`No collider shape found in the entity:"${postProcess.entity.name}"...`);
return null;
}
const closestDistance = this._findClosestDistance(shapes, camera);
return closestDistance > postProcess.blendDistance ? null :
postProcess.blendDistance > 0 ? 1 - closestDistance / postProcess.blendDistance : 1;
}Likely invalid or redundant comment.
packages/core/src/Engine.ts (4)
32-33: LGTM! Well-structured post-process management properties.
The new imports and internal properties provide a clean foundation for managing post-process passes. The use of specific array types and a boolean flag for optimization shows good attention to performance considerations.
Also applies to: 111-116
222-228: LGTM! Good use of ReadonlyArray for encapsulation.
The getter properly encapsulates the internal post-process passes array by returning it as a ReadonlyArray, preventing external modifications.
446-473: LGTM! Well-implemented post-process management methods.
The internal methods for removing and refreshing post-process passes are well-implemented with proper state management and array handling.
661-669: Verify the necessity of post-process pass sorting.
While the sorting implementation is technically correct, a previous review comment suggests it might not be needed. Let's verify if this sorting is essential for the post-processing workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/core/src/postProcess/effects/BloomEffect.ts (2)
78-78: Consider lowering the default threshold valueThe default threshold value of 0.9 might be too high for typical bloom effects, potentially making the effect too subtle. Most engines use values between 0.5-0.7 as defaults.
- threshold = new PostProcessEffectParameter(0.9, 0, Number.POSITIVE_INFINITY, true); + threshold = new PostProcessEffectParameter(0.6, 0, Number.POSITIVE_INFINITY, true);
101-102: Optimize validity check orderConsider checking the intensity first since it's likely to be the more frequent exclusion condition.
- return this.enabled && this.intensity.value > 0; + return this.intensity.value > 0 && this.enabled;packages/core/src/Engine.ts (3)
222-228: Consider enhancing the documentation.While the getter implementation is correct, the documentation could be more descriptive. Consider adding:
- @remarks explaining the purpose and usage of post-process passes
- @returns description clarifying that the array is read-only
430-473: Consider enhancing error handling in addPostProcessPass.The implementation is solid, but the error message could be more descriptive.
Consider this improvement:
- throw "The pass is not belong to this engine."; + throw new Error(`Post-process pass belongs to a different engine instance. Expected: ${this}, Found: ${pass.engine}`);
665-673: Consider optimizing sort for larger arrays.While the current implementation is correct, for better performance with larger arrays, consider:
- Using a stable sort algorithm
- Caching the comparison results
- passes.sort((a, b) => a.event - b.event); + passes.sort((a, b) => { + // Stable sort for consistent ordering of equal priority passes + const diff = a.event - b.event; + return diff === 0 ? passes.indexOf(a) - passes.indexOf(b) : diff; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/core/src/Camera.ts(2 hunks)packages/core/src/Engine.ts(8 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(5 hunks)packages/core/src/postProcess/PostProcess.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/PostProcessUberPass.ts(1 hunks)packages/core/src/postProcess/effects/BloomEffect.ts(2 hunks)tests/src/core/postProcess/PostProcess.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/Camera.ts
- tests/src/core/postProcess/PostProcess.test.ts
👮 Files not reviewed due to content moderation or server errors (3)
- packages/core/src/postProcess/PostProcessPass.ts
- packages/core/src/postProcess/PostProcessEffect.ts
- packages/core/src/postProcess/PostProcess.ts
🔇 Additional comments (12)
packages/core/src/postProcess/PostProcessManager.ts (2)
194-201:
Add null checks in resource cleanup
The resource cleanup should verify texture existence before destruction to prevent potential null reference exceptions.
private _releaseSwapRenderTarget(): void {
const swapRenderTarget = this._swapRenderTarget;
if (swapRenderTarget) {
- swapRenderTarget.getColorTexture(0)?.destroy(true);
+ const colorTexture = swapRenderTarget.getColorTexture(0);
+ if (colorTexture) {
+ colorTexture.destroy(true);
+ }
swapRenderTarget.destroy(true);
this._swapRenderTarget = null;
}
}Likely invalid or redundant comment.
267-268:
Add error handling for texture retrieval
The _getCurrentSourceTexture method should handle potential null textures.
private _getCurrentSourceTexture(): Texture2D {
- return this._currentSourceRenderTarget.getColorTexture(0) as Texture2D;
+ const texture = this._currentSourceRenderTarget.getColorTexture(0);
+ if (!texture) {
+ throw new Error("Failed to get color texture from current source render target");
+ }
+ return texture as Texture2D;
}Likely invalid or redundant comment.
packages/core/src/postProcess/PostProcessUberPass.ts (4)
277-277:
Add error handling for shader creation
The shader creation should be wrapped in a try-catch block to handle potential compilation errors.
-Shader.create(PostProcessUberPass.UBER_SHADER_NAME, blitVs, UberPost);
+try {
+ Shader.create(PostProcessUberPass.UBER_SHADER_NAME, blitVs, UberPost);
+} catch (error) {
+ console.error(`Failed to create ${PostProcessUberPass.UBER_SHADER_NAME} shader:`, error);
+ throw error;
+}Likely invalid or redundant comment.
149-151:
Validate viewport dimensions
Add validation for viewport dimensions to prevent issues with very small viewports.
const tw = pixelViewport.width >> downRes;
const th = pixelViewport.height >> downRes;
+ if (tw < 1 || th < 1) {
+ throw new Error("Viewport dimensions too small for bloom effect");
+ }
const maxSize = Math.max(tw, th);Likely invalid or redundant comment.
234-234:
Add safety check for dirt texture dimensions
The dirt texture ratio calculation needs validation to prevent division by zero.
- const dirtRatio = dirtTexture.value.width / dirtTexture.value.height;
+ const height = dirtTexture.value.height;
+ if (height === 0) {
+ throw new Error("Invalid dirt texture dimensions: height cannot be zero");
+ }
+ const dirtRatio = dirtTexture.value.width / height;Likely invalid or redundant comment.
39-43:
Add shader existence validation
The material initialization should verify shader existence to prevent runtime errors.
- const uberMaterial = new Material(engine, Shader.find(PostProcessUberPass.UBER_SHADER_NAME));
+ const uberShader = Shader.find(PostProcessUberPass.UBER_SHADER_NAME);
+ if (!uberShader) {
+ throw new Error(`Shader ${PostProcessUberPass.UBER_SHADER_NAME} not found`);
+ }
+ const uberMaterial = new Material(engine, uberShader);Likely invalid or redundant comment.
packages/core/src/Engine.ts (3)
32-33: LGTM! Well-structured post-process management properties.
The new imports and internal properties are well-organized and properly encapsulated for managing post-process passes.
Also applies to: 111-116
280-282: LGTM! Proper initialization of post-processing pipeline.
The PostProcessUberPass is correctly initialized after all required resources are set up.
566-569: LGTM! Proper integration with render pipeline.
The post-process manager update is correctly placed in the render pipeline, ensuring proper synchronization with camera updates.
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (3)
1-1: LGTM! Clean architectural improvements
The addition of Vector4 and Blitter imports, along with the _sourceScaleOffset member variable, shows a good architectural shift towards more maintainable texture coordinate handling.
Also applies to: 24-24, 51-51
231-243: Add test coverage for UV transformation logic
While the UV transformation logic using sourceScaleOffset is well-implemented, it would benefit from dedicated test coverage to ensure correct behavior across different render target configurations.
Let's verify the test coverage:
#!/bin/bash
# Description: Check test coverage for UV transformation logic
# Look for test files that cover UV transformations in texture blitting
# Search for relevant test files
echo "Searching for relevant test files..."
rg -l "sourceScaleOffset|blitTexture" "tests/"
# Search for test assertions related to UV transformations
echo "Searching for UV transformation test assertions..."
rg "expect.*sourceScaleOffset|assert.*UV" "tests/"289-304: Verify post-process manager state management
The post-processing integration looks good, but we should ensure proper cleanup of resources in all code paths. The _releaseSwapRenderTarget call is correctly placed for the non-post-processing path, but verify that resources are also properly released in the post-processing path.
Let's verify the resource management:
✅ Verification successful
Let me gather more specific information about the post-processing cleanup by checking the implementation of _render method.
Let me try a different approach to find the PostProcessManager implementation.
Let me check the specific implementation of _render and resource cleanup methods.
Based on the implementation details found, I can now provide the final response:
Post-process resource management is properly handled
The resource cleanup is correctly implemented in both paths:
- In the non-post-processing path,
_releaseSwapRenderTargetis explicitly called as shown in the original code - In the post-processing path, resources are properly managed through
_rendermethod which:- Initializes swap render target only when needed
- Properly handles MSAA resolution via
_blitRenderTarget - Manages render target swapping during processing
- Cleanup is handled through the
_releaseSwapRenderTargetwhich properly destroys both the render target and its associated color texture
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper resource cleanup in post-processing paths
# Search for resource cleanup patterns in post-processing code
echo "Searching for resource cleanup in post-processing..."
ast-grep --pattern 'postProcessManager._render($_) {
$$$
$_._releaseSwapRenderTarget();
$$$
}'
# Search for potential resource leaks
echo "Searching for potential resource leak patterns..."
rg "new (RenderTarget|Texture)" --type ts
Length of output: 14334
Script:
#!/bin/bash
# Search for PostProcessManager's _render implementation
echo "Searching for PostProcessManager._render implementation..."
ast-grep --pattern 'class PostProcessManager {
$$$
_render($_) {
$$$
}
$$$
}'
# Also check for cleanup in _isValidInCamera method
echo "Searching for _isValidInCamera implementation..."
ast-grep --pattern 'class PostProcessManager {
$$$
_isValidInCamera($_) {
$$$
}
$$$
}'
Length of output: 426
Script:
#!/bin/bash
# Search for PostProcessManager file and implementation
echo "Searching for PostProcessManager file..."
fd "PostProcessManager" --type f
echo "Searching for PostProcessManager implementation with context..."
rg "class PostProcessManager" -A 20 --type ts
Length of output: 2272
Script:
#!/bin/bash
# Search for _render and cleanup related methods in PostProcessManager
echo "Searching for _render and cleanup methods..."
rg "_render|_releaseSwapRenderTarget|_isValidInCamera" -B 2 -A 15 packages/core/src/postProcess/PostProcessManager.ts
Length of output: 2108
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/core/src/postProcess/PostProcessEffectParameter.ts (2)
27-33: 🛠️ Refactor suggestionImprove type checking and value validation
The current implementation has two issues:
- Using
.constructorfor type checking is not type-safe- The clamp operation should handle undefined min/max values
set value(value: T) { - if (this.value?.constructor === Number) { + if (typeof value === 'number') { - this._value = <T>(<unknown>MathUtil.clamp(<number>value, this._min, this._max)); + const min = this._min ?? Number.NEGATIVE_INFINITY; + const max = this._max ?? Number.POSITIVE_INFINITY; + this._value = <T>(<unknown>MathUtil.clamp(<number>value, min, max)); } else { this._value = value; } }
67-93:⚠️ Potential issueImprove lerp method implementation
The current implementation has several issues:
- No validation for interpolation factor
- Uses constructor for type checking
- No color value clamping after interpolation
lerp(to: T, factor: number) { + const clampedFactor = MathUtil.clamp(factor, 0, 1); if (this._needLerp) { - switch (this.value?.constructor) { + switch (true) { + case typeof this.value === 'number': - this.value = <T>(<unknown>MathUtil.lerp(<number>this.value, <number>to, factor)); + this.value = <T>(<unknown>MathUtil.lerp(<number>this.value, <number>to, clampedFactor)); break; - case Color: + case this.value instanceof Color: Color.lerp(<Color>this.value, <Color>to, factor, <Color>this.value); + (<Color>this.value).clamp(0, 1); break; - case Vector2: + case this.value instanceof Vector2: - Vector2.lerp(<Vector2>this.value, <Vector2>to, factor, <Vector2>this.value); + Vector2.lerp(<Vector2>this.value, <Vector2>to, clampedFactor, <Vector2>this.value); break; - case Vector3: + case this.value instanceof Vector3: - Vector3.lerp(<Vector3>this.value, <Vector3>to, factor, <Vector3>this.value); + Vector3.lerp(<Vector3>this.value, <Vector3>to, clampedFactor, <Vector3>this.value); break; - case Vector4: + case this.value instanceof Vector4: - Vector4.lerp(<Vector4>this.value, <Vector4>to, factor, <Vector4>this.value); + Vector4.lerp(<Vector4>this.value, <Vector4>to, clampedFactor, <Vector4>this.value); break; default: - if (factor > 0) { + if (clampedFactor > 0) { this.value = to; } } - } else if (factor > 0) { + } else if (clampedFactor > 0) { this.value = to; } }
🧹 Nitpick comments (2)
packages/core/src/postProcess/PostProcessEffectParameter.ts (2)
4-8: Enhance class documentationThe documentation could be more detailed about:
- The generic type parameter T and its constraints
- Examples of typical usage scenarios
- The relationship with post process manager
44-60: Simplify constructor logicThe current implementation has redundant type checking and complex conditional logic.
constructor(value: T, needLerpOrMin?: boolean | number, max?: number, needLerp?: boolean) { - if (typeof needLerpOrMin === "boolean") { - this._needLerp = needLerpOrMin; - } else if (typeof value === "number") { - if (typeof needLerpOrMin === "boolean") { - this._needLerp = needLerpOrMin; - this._min = Number.NEGATIVE_INFINITY; - this._max = Number.POSITIVE_INFINITY; - } else if (typeof needLerpOrMin === "number") { - this._min = needLerpOrMin; - this._max = max ?? Number.POSITIVE_INFINITY; - this._needLerp = needLerp ?? false; - } + if (typeof value === "number") { + if (typeof needLerpOrMin === "number") { + this._min = needLerpOrMin; + this._max = max ?? Number.POSITIVE_INFINITY; + this._needLerp = needLerp ?? false; + } else { + this._needLerp = needLerpOrMin ?? false; + } + } else { + this._needLerp = needLerpOrMin as boolean ?? false; } this.value = value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/src/Camera.ts(2 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(5 hunks)packages/core/src/postProcess/PostProcess.ts(1 hunks)packages/core/src/postProcess/PostProcessEffect.ts(1 hunks)packages/core/src/postProcess/PostProcessEffectParameter.ts(1 hunks)packages/core/src/postProcess/PostProcessManager.ts(1 hunks)tests/src/core/postProcess/PostProcess.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/Camera.ts
- tests/src/core/postProcess/PostProcess.test.ts
- packages/core/src/postProcess/PostProcessEffect.ts
👮 Files not reviewed due to content moderation or server errors (3)
- packages/core/src/postProcess/PostProcess.ts
- packages/core/src/postProcess/PostProcessManager.ts
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/postProcess/PostProcessEffectParameter.ts
[error] 9-9: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 9-9: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
[error] 36-36: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
[error] 38-38: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
| * @remarks | ||
| * The parameter will be mixed to a final value and be used in post process manager. | ||
| */ | ||
| export class PostProcessEffectParameter<T extends Number | Boolean | Color | Vector2 | Vector3 | Vector4 | Texture> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use primitive types instead of object types
Replace Number and Boolean with their primitive counterparts number and boolean for better type safety and consistency with TypeScript best practices.
-export class PostProcessEffectParameter<T extends Number | Boolean | Color | Vector2 | Vector3 | Vector4 | Texture> {
+export class PostProcessEffectParameter<T extends number | boolean | Color | Vector2 | Vector3 | Vector4 | Texture> {📝 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.
| export class PostProcessEffectParameter<T extends Number | Boolean | Color | Vector2 | Vector3 | Vector4 | Texture> { | |
| export class PostProcessEffectParameter<T extends number | boolean | Color | Vector2 | Vector3 | Vector4 | Texture> { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 9-9: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
| constructor(value: Exclude<T, number>, needLerp?: boolean); | ||
| constructor(value: Exclude<T, Boolean | Color | Vector2 | Vector3 | Vector4 | Texture>, needLerp?: boolean); | ||
| constructor( | ||
| value: Exclude<T, Boolean | Color | Vector2 | Vector3 | Vector4 | Texture>, | ||
| min?: number, | ||
| max?: number, | ||
| needLerp?: boolean | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use primitive types in constructor overloads
Replace object types with primitives in constructor overloads for consistency.
- constructor(value: Exclude<T, number>, needLerp?: boolean);
- constructor(value: Exclude<T, Boolean | Color | Vector2 | Vector3 | Vector4 | Texture>, needLerp?: boolean);
- constructor(
- value: Exclude<T, Boolean | Color | Vector2 | Vector3 | Vector4 | Texture>,
+ constructor(value: Exclude<T, number>, needLerp?: boolean);
+ constructor(value: Exclude<T, boolean | Color | Vector2 | Vector3 | Vector4 | Texture>, needLerp?: boolean);
+ constructor(
+ value: Exclude<T, boolean | Color | Vector2 | Vector3 | Vector4 | Texture>,
min?: number,
max?: number,
needLerp?: boolean
);📝 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(value: Exclude<T, number>, needLerp?: boolean); | |
| constructor(value: Exclude<T, Boolean | Color | Vector2 | Vector3 | Vector4 | Texture>, needLerp?: boolean); | |
| constructor( | |
| value: Exclude<T, Boolean | Color | Vector2 | Vector3 | Vector4 | Texture>, | |
| min?: number, | |
| max?: number, | |
| needLerp?: boolean | |
| ); | |
| constructor(value: Exclude<T, number>, needLerp?: boolean); | |
| constructor(value: Exclude<T, boolean | Color | Vector2 | Vector3 | Vector4 | Texture>, needLerp?: boolean); | |
| constructor( | |
| value: Exclude<T, boolean | Color | Vector2 | Vector3 | Vector4 | Texture>, | |
| min?: number, | |
| max?: number, | |
| needLerp?: boolean | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
[error] 38-38: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/core/src/postProcess/effects/TonemappingEffect.ts (2)
8-22: Consider adding more common tonemapping operators.The current implementation with Neutral and ACES modes is well-documented and covers basic needs. However, consider adding other common tonemapping operators like Reinhard, Uncharted 2, or Filmic for more flexibility.
24-31: Document the boolean parameter in PostProcessEffectParameter constructor.The
modeproperty initialization includes a boolean parameter (false), but its purpose isn't immediately clear. Consider adding a comment or using a named constant to clarify its meaning.- mode = new PostProcessEffectParameter(TonemappingMode.Neutral, false); + /** Controls the tonemapping algorithm. The boolean parameter indicates if the effect requires real-time updates. */ + mode = new PostProcessEffectParameter(TonemappingMode.Neutral, /* requiresUpdate */ false);packages/core/src/postProcess/effects/BloomEffect.ts (2)
74-74: Clarify gamma-space threshold documentation.The threshold parameter's documentation mentions gamma-space, but the value range starts from 0 without clear indication of how gamma correction is applied. Consider adding more details about the gamma-space conversion process and expected input values.
79-94: Consider enhancing parameter documentation.The scatter, intensity, and tint parameters would benefit from more detailed documentation explaining:
- For scatter: How different values affect the bloom radius visually
- For intensity: Guidelines for typical value ranges
- For tint: How the color channels affect the final bloom appearance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/Scene.ts(2 hunks)packages/core/src/postProcess/PostProcessPass.ts(1 hunks)packages/core/src/postProcess/effects/BloomEffect.ts(2 hunks)packages/core/src/postProcess/effects/TonemappingEffect.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/postProcess/PostProcessPass.ts
🔇 Additional comments (8)
packages/core/src/postProcess/effects/TonemappingEffect.ts (1)
1-3: LGTM! Clean and well-organized imports.
The imports are appropriate and follow a clear organization pattern.
packages/core/src/postProcess/effects/BloomEffect.ts (5)
17-21: LGTM! Well-documented enum with clear options.
The BloomDownScaleMode enum provides clear options for controlling bloom texture size with appropriate documentation.
27-52: Well-organized static properties with clear internal/external boundaries.
The static properties are logically grouped into bloom and uber shader properties, with clear @internal annotations and descriptive comments. The naming convention is consistent and follows best practices.
96-99: LGTM! Clear and concise validation logic.
The isValid override provides appropriate validation by checking both the enabled state and intensity value.
Line range hint 101-106: LGTM! Well-structured shader pipeline setup.
The shader creation follows a logical sequence of passes (prefilter -> blur -> upsample) with clear naming conventions.
Line range hint 8-11: Verify shader file dependencies.
Let's ensure all referenced shader files exist and are properly imported.
✅ Verification successful
Let me try a different search strategy to locate these shader files, as the initial path assumption might be incorrect.
All shader dependencies are present and correctly located
The shader files are present at the correct relative path from the importing file:
- packages/core/src/postProcess/shaders/Bloom/BloomBlurH.glsl
- packages/core/src/postProcess/shaders/Bloom/BloomBlurV.glsl
- packages/core/src/postProcess/shaders/Bloom/BloomPrefilter.glsl
- packages/core/src/postProcess/shaders/Bloom/BloomUpsample.glsl
The import paths in the code match the actual file structure, so all dependencies are properly resolved.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify existence of required shader files
echo "Checking shader file dependencies..."
for shader in "Bloom/BloomBlurH.glsl" "Bloom/BloomBlurV.glsl" "Bloom/BloomPrefilter.glsl" "Bloom/BloomUpsample.glsl"; do
if fd -t f "$shader" src/postProcess/shaders/; then
echo "✓ Found: $shader"
else
echo "✗ Missing: $shader"
fi
done
Length of output: 1555
Script:
#!/bin/bash
# Search for the shader files anywhere in the repository
echo "Searching for shader files..."
fd -t f -e glsl . | grep -i "bloom"
# Also check the imports directory structure
echo -e "\nChecking directory structure..."
fd -t d "shaders"
Length of output: 556
packages/core/src/Scene.ts (2)
15-15:
Breaking change detected in PostProcessManager import
The change from _PostProcessManager to PostProcessManager represents a breaking change that could affect existing code using the old import.
Let's verify the impact:
53-55: Consider moving postProcessManager to Engine class
As suggested in previous discussions, if postProcessManager is used to configure the post-processing pipeline, it might be more appropriate to move it to the Engine class for better architectural organization.
Additionally, verify that the initialization timing in the constructor properly handles the setup of post-processing effects.
Let's analyze the usage patterns:
| private _defaultEffectMap = new Map<typeof PostProcessEffect, PostProcessEffect>(); | ||
| private _remainActivePassCount = 0; | ||
| private _tempColliders: Collider[] = []; | ||
| private _tempColliderShapes: ColliderShape[] = []; |
|
|
||
| for (let i = 0, n = parameters.length; i < n; i++) { | ||
| const targetParameter = toParameters[i]; | ||
| if (targetParameter.enabled) { |
There was a problem hiding this comment.
targetParameter->toParameter
Not consider from enable?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/core/src/postProcess/PostProcessManager.ts (2)
23-33: Ensure proper cleanup of render targets.The class maintains several render target references but only
_swapRenderTargetis properly cleaned up. Consider adding cleanup for other render targets in the destructor to prevent memory leaks.
216-229: Optimize effect instance resetting.The
_resetDefaultValuemethod creates new default effects on demand. Consider pre-creating these instances during initialization for better performance.packages/core/src/Engine.ts (1)
582-585: Consider caching post-process update results.The post-process manager update is called for each camera. Consider caching the results if multiple cameras share the same post-process configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/Engine.ts(8 hunks)packages/core/src/postProcess/PostProcessEffectParameter.ts(1 hunks)packages/core/src/postProcess/PostProcessManager.ts(1 hunks)packages/core/src/postProcess/PostProcessPass.ts(1 hunks)tests/src/core/postProcess/PostProcessPass.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/src/core/postProcess/PostProcessPass.test.ts
- packages/core/src/postProcess/PostProcessPass.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/Engine.ts
[error] 440-440: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 453-453: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/core/src/postProcess/PostProcessEffectParameter.ts
[error] 9-9: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 9-9: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
[error] 36-36: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
[error] 38-38: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (2)
packages/core/src/postProcess/PostProcessManager.ts (1)
270-272:
Add null check for color texture.
The _getCurrentSourceTexture method assumes the color texture is always available. Add a null check to prevent potential runtime errors.
Apply this diff to add proper error handling:
private _getCurrentSourceTexture(): Texture2D {
- return this._currentSourceRenderTarget.getColorTexture(0) as Texture2D;
+ const texture = this._currentSourceRenderTarget?.getColorTexture(0) as Texture2D;
+ if (!texture) {
+ throw new Error("Current source render target does not have a color texture");
+ }
+ return texture;
}Likely invalid or redundant comment.
packages/core/src/postProcess/PostProcessEffectParameter.ts (1)
9-9: Use primitive types instead of object types
Replace Number and Boolean with their primitive counterparts number and boolean for better type safety and consistency with TypeScript best practices.
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 9-9: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
| _update(camera: Camera): void { | ||
| // Start by resetting post process effect instance to default values | ||
| this._resetDefaultValue(); | ||
|
|
||
| // Sort post process | ||
| this._sortActivePostProcess(); | ||
| const activePostProcesses = this._activePostProcesses; | ||
|
|
||
| for (let i = 0, n = activePostProcesses.length; i < n; i++) { | ||
| const postProcess = activePostProcesses[i]; | ||
|
|
||
| depthState.enabled = false; | ||
| depthState.writeEnabled = false; | ||
| if (!postProcess.enabled) { | ||
| continue; | ||
| } | ||
|
|
||
| const bloomEffect = new BloomEffect(uberMaterial); | ||
| const tonemappingEffect = new TonemappingEffect(uberMaterial); | ||
| if (!(camera.postProcessMask & postProcess.layer)) { | ||
| continue; | ||
| } | ||
|
|
||
| this._uberMaterial = uberMaterial; | ||
| this._bloomEffect = bloomEffect; | ||
| this._tonemappingEffect = tonemappingEffect; | ||
| const isGlobal = postProcess.isGlobal; | ||
| let interpFactor = 1; // Global default value | ||
| if (!isGlobal) { | ||
| const currentColliders = this._tempColliders; | ||
| const cameraPosition = camera.entity.transform.worldPosition; | ||
| const blendDistance = postProcess.blendDistance; | ||
|
|
||
| let hasColliderShape = false; | ||
| // Find closest distance to current postProcess, 0 means it's inside it | ||
| let closestDistance = Number.POSITIVE_INFINITY; | ||
|
|
||
| postProcess.entity.getComponents(Collider, currentColliders); | ||
| for (let i = 0; i < currentColliders.length; i++) { | ||
| const collider = currentColliders[i]; | ||
| if (!collider.enabled) { | ||
| continue; | ||
| } | ||
| const shapes = collider.shapes; | ||
| for (let j = 0; j < shapes.length; j++) { | ||
| const currentShape = shapes[j]; | ||
| hasColliderShape = true; | ||
|
|
||
| const distance = currentShape.getClosestPoint(cameraPosition, this._tempVector3); | ||
| if (distance < closestDistance) { | ||
| closestDistance = distance; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!hasColliderShape) { | ||
| Logger.warn( | ||
| `No collider shape found in the entity:"${postProcess.entity.name}", the local mode of post process will not take effect.` | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| // Post process has no influence, ignore it | ||
| if (closestDistance > blendDistance) { | ||
| continue; | ||
| } | ||
|
|
||
| if (blendDistance > 0) { | ||
| interpFactor = 1 - closestDistance / blendDistance; | ||
| } | ||
| } | ||
|
|
||
| const effects = postProcess._effects; | ||
| for (let j = 0; j < effects.length; j++) { | ||
| const effect = effects[j]; | ||
| if (!effect.enabled) { | ||
| continue; | ||
| } | ||
| const PostConstructor = effect.constructor as typeof PostProcessEffect; | ||
|
|
||
| let blendEffect = this._blendEffectMap.get(PostConstructor); | ||
| if (!blendEffect) { | ||
| blendEffect = new PostConstructor(null); | ||
| this._blendEffectMap.set(PostConstructor, blendEffect); | ||
| } | ||
|
|
||
| blendEffect.lerp(effect, interpFactor); | ||
| blendEffect.enabled = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the update method for better maintainability.
The _update method is complex with multiple responsibilities:
- Collider-based post-processing
- Effect blending
- Priority-based sorting
Consider breaking it down into smaller, focused methods for better maintainability and testability.
Apply this diff to improve the structure:
_update(camera: Camera): void {
+ this._resetDefaultValue();
+ this._sortActivePostProcess();
+ this._updatePostProcessEffects(camera);
+}
+
+private _updatePostProcessEffects(camera: Camera): void {
const activePostProcesses = this._activePostProcesses;
for (let i = 0, n = activePostProcesses.length; i < n; i++) {
const postProcess = activePostProcesses[i];
if (!postProcess.enabled || !(camera.postProcessMask & postProcess.layer)) {
continue;
}
- // Rest of the method...
+ this._updateSinglePostProcess(postProcess, camera);
}
}
+
+private _updateSinglePostProcess(postProcess: PostProcess, camera: Camera): void {
+ const interpFactor = this._calculateBlendFactor(postProcess, camera);
+ if (interpFactor === null) return;
+
+ this._blendEffects(postProcess._effects, interpFactor);
+}Committable suggestion skipped: line range outside the PR's diff.
| addPostProcessPass(pass: PostProcessPass): void { | ||
| if (pass.engine !== this) { | ||
| throw "The pass is not belong to this engine."; | ||
| } | ||
|
|
||
| const passes = this._postProcessPasses; | ||
| if (passes.indexOf(pass) === -1) { | ||
| passes.push(pass); | ||
| pass.isActive && (this._postProcessPassNeedRefresh = true); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and fix assignment in expression.
The addPostProcessPass method could use better error handling and should address the static analysis warning about assignments in expressions.
Apply this diff to improve the code:
addPostProcessPass(pass: PostProcessPass): void {
if (pass.engine !== this) {
- throw "The pass is not belong to this engine.";
+ throw new Error("The pass does not belong to this engine");
}
const passes = this._postProcessPasses;
if (passes.indexOf(pass) === -1) {
passes.push(pass);
- pass.isActive && (this._postProcessPassNeedRefresh = true);
+ if (pass.isActive) {
+ this._postProcessPassNeedRefresh = true;
+ }
}
}📝 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.
| addPostProcessPass(pass: PostProcessPass): void { | |
| if (pass.engine !== this) { | |
| throw "The pass is not belong to this engine."; | |
| } | |
| const passes = this._postProcessPasses; | |
| if (passes.indexOf(pass) === -1) { | |
| passes.push(pass); | |
| pass.isActive && (this._postProcessPassNeedRefresh = true); | |
| } | |
| } | |
| addPostProcessPass(pass: PostProcessPass): void { | |
| if (pass.engine !== this) { | |
| throw new Error("The pass does not belong to this engine"); | |
| } | |
| const passes = this._postProcessPasses; | |
| if (passes.indexOf(pass) === -1) { | |
| passes.push(pass); | |
| if (pass.isActive) { | |
| this._postProcessPassNeedRefresh = true; | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 440-440: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| constructor(value: T, needLerpOrMin?: boolean | number, max?: number, needLerp?: boolean) { | ||
| if (typeof value === "number") { | ||
| if (typeof needLerpOrMin === "boolean") { | ||
| this._needLerp = needLerpOrMin; | ||
| this._min = Number.NEGATIVE_INFINITY; | ||
| this._max = Number.POSITIVE_INFINITY; | ||
| } else if (typeof needLerpOrMin === "number") { | ||
| this._min = needLerpOrMin; | ||
| this._max = max ?? Number.POSITIVE_INFINITY; | ||
| this._needLerp = needLerp ?? false; | ||
| } else if (needLerpOrMin == undefined) { | ||
| this._min = Number.NEGATIVE_INFINITY; | ||
| this._max = Number.POSITIVE_INFINITY; | ||
| } | ||
| } else { | ||
| this._needLerp = <boolean>needLerpOrMin ?? false; | ||
| } | ||
|
|
||
| this.value = value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify constructor implementation and add parameter validation
The constructor implementation is complex and could benefit from:
- Clearer parameter validation
- Simplified type checking
- Default value handling
Consider this cleaner implementation:
constructor(value: T, needLerpOrMin?: boolean | number, max?: number, needLerp?: boolean) {
+ // Initialize defaults
+ this._min = Number.NEGATIVE_INFINITY;
+ this._max = Number.POSITIVE_INFINITY;
+ this._needLerp = false;
+
if (typeof value === "number") {
- if (typeof needLerpOrMin === "boolean") {
- this._needLerp = needLerpOrMin;
- this._min = Number.NEGATIVE_INFINITY;
- this._max = Number.POSITIVE_INFINITY;
- } else if (typeof needLerpOrMin === "number") {
+ if (typeof needLerpOrMin === "number") {
this._min = needLerpOrMin;
this._max = max ?? Number.POSITIVE_INFINITY;
this._needLerp = needLerp ?? false;
- } else if (needLerpOrMin == undefined) {
- this._min = Number.NEGATIVE_INFINITY;
- this._max = Number.POSITIVE_INFINITY;
+ } else if (typeof needLerpOrMin === "boolean") {
+ this._needLerp = needLerpOrMin;
}
} else {
this._needLerp = <boolean>needLerpOrMin ?? false;
}
this.value = value;
}📝 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(value: T, needLerpOrMin?: boolean | number, max?: number, needLerp?: boolean) { | |
| if (typeof value === "number") { | |
| if (typeof needLerpOrMin === "boolean") { | |
| this._needLerp = needLerpOrMin; | |
| this._min = Number.NEGATIVE_INFINITY; | |
| this._max = Number.POSITIVE_INFINITY; | |
| } else if (typeof needLerpOrMin === "number") { | |
| this._min = needLerpOrMin; | |
| this._max = max ?? Number.POSITIVE_INFINITY; | |
| this._needLerp = needLerp ?? false; | |
| } else if (needLerpOrMin == undefined) { | |
| this._min = Number.NEGATIVE_INFINITY; | |
| this._max = Number.POSITIVE_INFINITY; | |
| } | |
| } else { | |
| this._needLerp = <boolean>needLerpOrMin ?? false; | |
| } | |
| this.value = value; | |
| } | |
| constructor(value: T, needLerpOrMin?: boolean | number, max?: number, needLerp?: boolean) { | |
| // Initialize defaults | |
| this._min = Number.NEGATIVE_INFINITY; | |
| this._max = Number.POSITIVE_INFINITY; | |
| this._needLerp = false; | |
| if (typeof value === "number") { | |
| if (typeof needLerpOrMin === "number") { | |
| this._min = needLerpOrMin; | |
| this._max = max ?? Number.POSITIVE_INFINITY; | |
| this._needLerp = needLerp ?? false; | |
| } else if (typeof needLerpOrMin === "boolean") { | |
| this._needLerp = needLerpOrMin; | |
| } | |
| } else { | |
| this._needLerp = <boolean>needLerpOrMin ?? false; | |
| } | |
| this.value = value; | |
| } |
| set value(value: T) { | ||
| if (this.value?.constructor === Number) { | ||
| this._value = <T>(<unknown>MathUtil.clamp(<number>value, this._min, this._max)); | ||
| } else { | ||
| this._value = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve type checking and value handling
The current implementation has several issues that need to be addressed:
- Using
.constructorfor type checking is not type-safe - Multiple type assertions make the code harder to maintain
- Undefined
_minand_maxvalues could lead toNaNresults
Consider this safer implementation:
set value(value: T) {
- if (this.value?.constructor === Number) {
+ if (typeof value === "number") {
+ const min = this._min ?? Number.NEGATIVE_INFINITY;
+ const max = this._max ?? Number.POSITIVE_INFINITY;
- this._value = <T>(<unknown>MathUtil.clamp(<number>value, this._min, this._max));
+ this._value = MathUtil.clamp(value, min, max) as T;
} else {
this._value = value;
}
}📝 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.
| set value(value: T) { | |
| if (this.value?.constructor === Number) { | |
| this._value = <T>(<unknown>MathUtil.clamp(<number>value, this._min, this._max)); | |
| } else { | |
| this._value = value; | |
| } | |
| } | |
| set value(value: T) { | |
| if (typeof value === "number") { | |
| const min = this._min ?? Number.NEGATIVE_INFINITY; | |
| const max = this._max ?? Number.POSITIVE_INFINITY; | |
| this._value = MathUtil.clamp(value, min, max) as T; | |
| } else { | |
| this._value = value; | |
| } | |
| } |
| lerp(to: T, factor: number) { | ||
| if (this._needLerp) { | ||
| switch (this.value?.constructor) { | ||
| case Number: | ||
| this.value = <T>(<unknown>MathUtil.lerp(<number>this.value, <number>to, factor)); | ||
| break; | ||
| case Color: | ||
| Color.lerp(<Color>this.value, <Color>to, factor, <Color>this.value); | ||
| break; | ||
| case Vector2: | ||
| Vector2.lerp(<Vector2>this.value, <Vector2>to, factor, <Vector2>this.value); | ||
| break; | ||
| case Vector3: | ||
| Vector3.lerp(<Vector3>this.value, <Vector3>to, factor, <Vector3>this.value); | ||
| break; | ||
| case Vector4: | ||
| Vector4.lerp(<Vector4>this.value, <Vector4>to, factor, <Vector4>this.value); | ||
| break; | ||
| default: | ||
| if (factor > 0) { | ||
| this.value = to; | ||
| } | ||
| } | ||
| } else if (factor > 0) { | ||
| this.value = to; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Improve type safety and value validation in lerp method
The review comment's suggestions are valid and should be implemented:
- The type checking using constructor is unsafe and should be replaced with instanceof checks
- The interpolation factor should be clamped using MathUtil.clamp(factor, 0, 1)
- The Color class implementation shows no value clamping after interpolation, which could lead to out-of-range values
The suggested implementation in the review comment correctly addresses these issues by:
- Using safer type checking with instanceof
- Using if-else chains instead of switch-case
- Adding factor clamping
Additionally, the Color class should be enhanced to include value clamping in its setters to maintain color component values within the valid 0-1 range.
packages/core/src/postProcess/PostProcessEffectParameter.ts: Implement the suggested changespackages/math/src/Color.ts: Add value clamping in setters using MathUtil.clamp(value, 0, 1)
🔗 Analysis chain
Improve type safety and value validation in lerp method
The current implementation has several issues:
- Unsafe type checking using constructor
- Missing interpolation factor validation
- Potential out-of-range values after interpolation
Consider this safer implementation:
lerp(to: T, factor: number) {
+ const clampedFactor = MathUtil.clamp(factor, 0, 1);
if (this._needLerp) {
- switch (this.value?.constructor) {
- case Number:
+ if (typeof this.value === "number" && typeof to === "number") {
this.value = <T>(<unknown>MathUtil.lerp(<number>this.value, <number>to, factor));
- break;
+ } else if (this.value instanceof Color && to instanceof Color) {
- case Color:
Color.lerp(<Color>this.value, <Color>to, factor, <Color>this.value);
- break;
+ } else if (this.value instanceof Vector2 && to instanceof Vector2) {
- case Vector2:
Vector2.lerp(<Vector2>this.value, <Vector2>to, factor, <Vector2>this.value);
- break;
+ } else if (this.value instanceof Vector3 && to instanceof Vector3) {
- case Vector3:
Vector3.lerp(<Vector3>this.value, <Vector3>to, factor, <Vector3>this.value);
- break;
+ } else if (this.value instanceof Vector4 && to instanceof Vector4) {
- case Vector4:
Vector4.lerp(<Vector4>this.value, <Vector4>to, factor, <Vector4>this.value);
- break;
- default:
+ } else {
if (factor > 0) {
this.value = to;
}
- }
} else if (factor > 0) {
this.value = to;
}
}Additionally, consider adding value clamping after interpolation for color values to ensure they remain in the valid range.
Let's verify the Color class implementation:
Let me check if there's any value clamping in the Color class setters or other relevant methods.
Let me try a different approach to check the Color class implementation.
Let me check the MathUtil implementation specifically for the clamp function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Color class implementation for any value clamping
rg -A 5 "class Color|lerp.*Color" packages/math/src/Color.ts
Length of output: 553
Script:
#!/bin/bash
# Check Color class implementation for value clamping in setters and other methods
ast-grep --pattern 'class Color {
$$$
set r($_) {
$$$
}
$$$
}'
# Also check for any clamp utility usage
rg -B 2 -A 2 "clamp.*Color|Color.*clamp" packages/math/src/Color.ts
Length of output: 146
Script:
#!/bin/bash
# Get the full Color class implementation to check for value clamping
cat packages/math/src/Color.ts
# Also check if MathUtil has a clamp function that might be used
rg -A 2 "clamp.*=|function clamp" packages/math/src/MathUtil.ts
Length of output: 9809
Script:
#!/bin/bash
# Get the MathUtil class implementation to check clamp function
cat packages/math/src/MathUtil.ts
Length of output: 2303
GuoLei1990
changed the title
GuoLei1990
changed the title
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
Release Notes
New Features
PostProcessclass to manage visual effects in scenes.CustomPassfor custom shader effects.postProcessMaskin the Camera class to control applied effects.Blitterclass for enhanced texture blitting functionality.PostProcessManagerfor improved management of post-process passes and effects.PostProcessEffectParameterfor better management of effect parameters.Improvements
BloomEffectandTonemappingEffectclasses for easier property management.PostProcessUberPass.SceneLoaderto simplify scene configuration regarding post-processing.Bug Fixes
Documentation