Refactor PostProcessParameter to strong type
#2487
Conversation
WalkthroughThis pull request introduces significant enhancements to the post-processing parameter system in the Galacean engine. The changes focus on refactoring the Changes
Suggested labels
Suggested reviewers
Poem
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: 1
🔭 Outside diff range comments (1)
packages/core/src/postProcess/PostProcessEffectParameter.ts (1)
Line range hint
9-42: Enhance type safety for thetypeconstructor parameter.In the
PostProcessEffectParameter<T>constructor, thetypeparameter is untyped and can accept any value. Consider explicitly defining its type (e.g.,constructor(type: new (...args: any[]) => any, value: T, needLerp = false)) or introducing an interface for reflection to reinforce compile-time checks.
🧹 Nitpick comments (10)
packages/core/src/postProcess/PostProcessEffectParameter.ts (5)
2-2: Consider adding type annotations or docstrings for the importedTexture.Right now, we're importing
Texturefrom"../texture", but there's no inline doc comment for the import itself to clarify how it's used. It might be helpful to add a brief note or ensure theTextureclass is well documented in its own file, especially since it’s a core type used across multiple parameter classes.
44-80: Use DRY patterns for_lerpacross numeric parameter classes.In
PostProcessEffectFloatParameter, we override_lerpto clamp and linearly interpolate. Similar logic recurs in other numeric-based classes. Consider extracting a shared interpolation utility function to ensure consistency and reduce duplication across parameter classes like floats, vectors, etc.
82-93: Document behavior for non-interpolated parameters.
PostProcessEffectBoolParameterdoesn’t interpolate, which is logical. Briefly document in_lerpor in the class doc that booleans are effectively toggles and do not support blending.
130-150: Refactor repeated vector interpolation logic.
PostProcessEffectVector2Parameterduplicates the same_lerpapproach as other vector classes. Similar to the float parameter suggestion, factor out the vector interpolation if possible.
196-208: Cover enum parameter usage with tests.
PostProcessEffectEnumParameteris a specialized class that doesn’t interpolate values. Since enumerations can be set and toggled at runtime, adding test scenarios ensures it handles valid and invalid enum values properly.Would you like me to help create a new test file or open a GitHub issue to track additional unit tests?
e2e/case/postProcess-customPass.ts (1)
12-12: Incorporate test coverage forCustomPassintensity usage.
intensity = new PostProcessEffectFloatParameter(0.7, 0, 1)is a great example of typed parameters. However, e2e tests generally verify the rendering outcome rather than unit-level logic. Consider introducing or updating unit tests to confirm boundaries (0, 1) behave as expected.Also applies to: 47-47
packages/core/src/postProcess/effects/BloomEffect.ts (4)
63-63: Boolean parameter inherits clarity from its name.Naming is consistent with usage (a boolean toggle for high-quality filtering). Consider providing documentation comments about performance implications of switching this on/off.
79-79: Float parameter with minimum set to 0.Ensuring threshold values don’t go below zero is logical. If an upper bound is needed (e.g., typical threshold might not exceed 1.0), consider adding it for stricter validation.
89-89: No upper bound on intensity.Permitting an unbounded upper limit is acceptable if physically-based or high dynamic range effects require it. If you want to prevent extreme or accidental values, consider an upper bound.
94-94: No upper bound on dirtIntensity.Similar to intensity, you might consider limiting excessively large values if it can cause unexpected visual artifacts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/case/postProcess-customPass.ts(2 hunks)packages/core/src/postProcess/PostProcess.ts(1 hunks)packages/core/src/postProcess/PostProcessEffectParameter.ts(2 hunks)packages/core/src/postProcess/effects/BloomEffect.ts(2 hunks)packages/core/src/postProcess/effects/TonemappingEffect.ts(2 hunks)packages/core/src/postProcess/index.ts(1 hunks)tests/src/core/postProcess/PostProcess.test.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (17)
packages/core/src/postProcess/PostProcessEffectParameter.ts (5)
31-42: Review the_lerpfallback logic.When
factor > 0, the base implementation simply setsthis.valuetoto. This may ignore the precise interpolation iffactoris between 0 and 1. Verify this is intended behavior; if not, update it to a proper mixed (i.e.,current*(1-factor) + to*factor) approach for partial interpolation.
95-106: Confirm defaultneedLerpfor texture parameters.Textures typically don’t get blended, so
needLerpis set tofalse. Verify that no other feature in the codebase attempts to animate or crossfade texture parameters. If yes, you may need a custom_lerpor a fallback strategy.
108-125: Ensure color interpolation is consistent with the engine’s color space.
PostProcessEffectColorParameterusesColor.lerp. Confirm that the engine’s default color space (e.g., linear vs. gamma) is appropriate for this operation. If the engine uses a linear space, ensure the color being interpolated is also in the same space when applying partial blends.
152-172: Validate partial interpolation for Vector3.
Vector3.lerp(this.value, to, factor, this.value);modifiesthis.valuein place. Ensure that side effects do not disrupt other references to the sameVector3object.
174-194: Consistent in-place mutation for Vector4.Similar to
Vector3,Vector4.lerp(...)also modifiesthis.valuein place. Make sure that external references to the sameVector4aren’t expecting an immutable operation.packages/core/src/postProcess/index.ts (1)
5-15: Exported bundle clarifies usage of typed parameters.This consolidated export block is an improvement in readability. However, remember to update any existing import statements in the codebase that might directly import from
PostProcessEffectParameterto avoid confusion or redundancy.packages/core/src/postProcess/effects/TonemappingEffect.ts (2)
3-3: Use descriptive type names in the import.The import uses
PostProcessEffectEnumParameterfor an enum-based approach. This is clearer than the generic parameter usage. Confirm all references to the oldPostProcessEffectParameterare removed or updated to avoid duplication.
31-31: Validate fallback for themodeparameter.
TonemappingEffect.modenow defaults toTonemappingMode.Neutral. Ensure there's a fallback or an error-check if the user supplies an invalid enum value. Typically, the engine or the dev environment can detect out-of-range enum usage, but consider additional safety checks if needed.packages/core/src/postProcess/effects/BloomEffect.ts (5)
6-12: Good adoption of the new typed parameter classes.Using specialized parameter classes helps eliminate ambiguity and enhances code readability.
68-68: Enum parameter usage looks correct.This approach ensures only valid downscale modes can be passed, minimizing runtime errors. You might consider providing a default or fallback logic if new enum values are introduced elsewhere.
73-73: Texture parameter addition is well-structured.Allowing a null default is a clean way to optionally disable dirt textures.
84-84: Float parameter with range [0, 1].Clear usage for scatter. Code is concise, and constraints are explicit.
99-99: Explicit color parameter is well-defined.This strengthens clarity for developers, indicating a distinct color input instead of a more generic parameter.
tests/src/core/postProcess/PostProcess.test.ts (4)
10-11: Imports align with newly specialized parameters.The new imports correctly reflect the transition from generic to typed parameters, ensuring the tests remain consistent with production code.
30-30: Float parameter usage is consistent with the custom effect logic.Explicit range [0,1] ensures robust clamping for intensity in the test scenario.
226-231: Comprehensive float parameter tests.This covers default values, clamping logic, and optional autoFit. Such thorough testing supports confident refactoring.
242-244: Boolean parameter tests are straightforward.It’s good to see essential coverage for true/false states.
|
|
||
| /** | ||
| * @inheritdoc | ||
| */ | ||
| override _onDestroy(): void { | ||
| super._onDestroy(); | ||
| this._effects.length = 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure all effect resources are properly released in _onDestroy.
Clearing _effects is a good step. Also consider calling a destroy / cleanup method on each effect if they hold resources (buffers, textures, etc.). That ensures no memory leaks persist within the engine or GPU resources.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/core/src/postProcess/PostProcessEffectParameter.ts (7)
Line range hint
9-42: LGTM! Consider adding type constraints for better type safety.The refactoring to an abstract base class with protected members improves the design. The simplified value setter and default lerp implementation provide a good foundation for derived classes.
Consider adding type constraints to prevent primitive types from being boxed:
- export abstract class PostProcessEffectParameter<T> { + export abstract class PostProcessEffectParameter<T extends object | number | boolean> {
44-80: Fix documentation parameter names.The implementation looks good with proper value clamping and lerp behavior. However, there's a minor documentation issue.
Remove the underscore prefix from parameter names in the constructor documentation to match the actual parameter names:
/** * Create a new float parameter. * @param value - The default value of the parameter - * @param _min - The minimum value of the parameter, default is Number.NEGATIVE_INFINITY - * @param _max - The maximum value of the parameter, default is Number.POSITIVE_INFINITY + * @param min - The minimum value of the parameter, default is Number.NEGATIVE_INFINITY + * @param max - The maximum value of the parameter, default is Number.POSITIVE_INFINITY * @param needLerp - Whether the parameter needs to be lerp, default is true */
95-106: Consider adding null safety for texture parameter.The implementation is good, but textures might be null in some cases.
Consider adding null safety:
- export class PostProcessEffectTextureParameter extends PostProcessEffectParameter<Texture> { + export class PostProcessEffectTextureParameter extends PostProcessEffectParameter<Texture | null> { constructor(value: Texture | null = null) { super(value, false); } }
108-128: Consider caching the temporary color object.The implementation is correct, but creating temporary Color objects during lerp could be optimized.
Consider caching the temporary color object:
export class PostProcessEffectColorParameter extends PostProcessEffectParameter<Color> { + private static readonly _tempColor = new Color(); override _lerp(to: Color, factor: number) { if (this._needLerp) { - Color.lerp(this.value, to, factor, this.value); + Color.lerp(this.value, to, factor, PostProcessEffectColorParameter._tempColor); + this.value.copyFrom(PostProcessEffectColorParameter._tempColor); } else { super._lerp(to, factor); } } }
130-194: Consider reducing code duplication in vector parameter classes.The vector parameter implementations are correct but contain duplicated lerp logic.
Consider creating a base vector parameter class:
abstract class PostProcessEffectVectorParameter<T extends Vector2 | Vector3 | Vector4> extends PostProcessEffectParameter<T> { constructor(value: T, needLerp = true) { super(value, needLerp); } protected abstract lerpVectors(start: T, end: T, factor: number, out: T): void; override _lerp(to: T, factor: number) { if (this._needLerp) { this.lerpVectors(this.value, to, factor, this.value); } else { super._lerp(to, factor); } } }Then extend it in the vector parameter classes:
export class PostProcessEffectVector2Parameter extends PostProcessEffectVectorParameter<Vector2> { protected lerpVectors(start: Vector2, end: Vector2, factor: number, out: Vector2): void { Vector2.lerp(start, end, factor, out); } }
196-211: Improve type safety for enum parameter.The enum parameter implementation could be more strongly typed.
Consider using TypeScript's enum features more effectively:
- export class PostProcessEffectEnumParameter<T> extends PostProcessEffectParameter<T> { - constructor( - readonly enumType: Record<string, number | string>, - value: T - ) { - super(value as T, false); - } - } + export class PostProcessEffectEnumParameter<T extends number | string> extends PostProcessEffectParameter<T> { + constructor( + readonly enumType: { [key: string]: T }, + value: T + ) { + if (!(Object.values(enumType) as T[]).includes(value)) { + throw new Error('Invalid enum value'); + } + super(value, false); + } + }
Line range hint
1-211: Overall excellent implementation with room for minor improvements.The new strongly-typed parameter system is well-designed and maintainable. The separation into specific parameter types improves type safety and provides appropriate behavior for each type. Consider implementing the suggested improvements for:
- Type constraints in the base class
- Null safety for textures
- Vector parameter code reuse
- Enum parameter type safety
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/postProcess/PostProcessEffectParameter.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/core/src/postProcess/PostProcessEffectParameter.ts (1)
82-93: LGTM! Clean and straightforward implementation.The boolean parameter implementation is appropriately simple with needLerp disabled by default.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2487 +/- ##
===========================================
+ Coverage 68.42% 68.45% +0.02%
===========================================
Files 923 923
Lines 95904 96043 +139
Branches 8138 8134 -4
===========================================
+ Hits 65621 65744 +123
- Misses 30029 30046 +17
+ Partials 254 253 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
GuoLei1990
changed the title
GuoLei1990
changed the title
PostProcessParameter to strong type
* refactor: `PostProcessParameter` to strong type
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes