Clear the ShaderProgramPool cached on the Engine object when destroy shaderPass
#2409
Conversation
Sway007
added
Rendering
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Not sure whether to change the open api destroy(force: boolean = false): boolean in Shader class to destroy(engine: Engine, forece = false): boolean
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2409 +/- ##
========================================
Coverage 66.67% 66.67%
========================================
Files 895 895
Lines 92090 92093 +3
Branches 7274 7276 +2
========================================
+ Hits 61397 61403 +6
+ Misses 30448 30445 -3
Partials 245 245
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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 and nitpick comments (1)
packages/core/src/shader/Shader.ts (1)
Line range hint 337-337: Consider making engine dependency explicit in destroy method.
Regarding the previous discussion about changing the method signature to destroy(engine: Engine, force = false): While making the engine dependency explicit would improve clarity, it would be a breaking change. Consider:
- Adding it in the next major version
- Documenting the implicit engine dependency in the meantime
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 348-348: packages/core/src/shader/Shader.ts#L348
Added line #L348 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/core/src/shader/Shader.ts (1 hunks)
- packages/core/src/shader/ShaderPass.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/shader/Shader.ts
[warning] 348-348: packages/core/src/shader/Shader.ts#L348
Added line #L348 was not covered by tests
packages/core/src/shader/ShaderPass.ts
[warning] 152-154: packages/core/src/shader/ShaderPass.ts#L152-L154
Added lines #L152 - L154 were not covered by tests
🔇 Additional comments (5)
packages/core/src/shader/Shader.ts (3)
Line range hint 391-419: LGTM! Well-documented deprecation.
The deprecation notice for getMacroByName is clear and provides a proper migration path to ShaderMacro.getByName.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 348-348: packages/core/src/shader/Shader.ts#L348
Added line #L348 was not covered by tests
Line range hint 421-427: LGTM! Well-documented deprecation.
The deprecation notice for getPropertyByName is clear and provides a proper migration path to ShaderProperty.getByName.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 348-348: packages/core/src/shader/Shader.ts#L348
Added line #L348 was not covered by tests
348-348: Add test coverage for shader program pool cleanup.
The change from _destroy to _clearShaderProgramPool correctly implements the intended functionality. However, this critical change lacks test coverage.
Let's verify the test coverage:
Would you like me to help create test cases that verify:
- The shader program pool is properly cleared when destroy is called
- No cached shader programs can be retrieved after destruction
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 348-348: packages/core/src/shader/Shader.ts#L348
Added line #L348 was not covered by tests
packages/core/src/shader/ShaderPass.ts (2)
146-146: Enhancement: Added optional 'engine' parameter to '_clearShaderProgramPool'
The inclusion of the optional engine parameter in the _clearShaderProgramPool method allows for explicit control over the engine's shader program pool when clearing cached ShaderPrograms. This change improves resource management by ensuring that the engine's cache can be properly cleared upon destruction of a ShaderPass.
152-154: Verify consistent usage of '_clearShaderProgramPool' with 'engine' parameter
Since _clearShaderProgramPool now accepts an optional engine parameter to clear the engine's shader program pool, it's important to ensure that all calls to this method pass the engine parameter when necessary. This will prevent any residual cached ShaderPrograms from persisting unintentionally.
Run the following script to find all usages of _clearShaderProgramPool and check if the engine parameter is being passed:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 152-154: packages/core/src/shader/ShaderPass.ts#L152-L154
Added lines #L152 - L154 were not covered by tests
| if (engine) { | ||
| delete engine._shaderProgramPools[this._shaderPassId]; | ||
| } |
There was a problem hiding this comment.
Add tests to cover the new logic when 'engine' is provided
The newly added lines that delete the shader program pool from the engine are not currently covered by tests, as indicated by static analysis. To ensure the correctness of this functionality and prevent future regressions, please consider adding unit tests that validate the clearing of the shader program pool from the engine when _clearShaderProgramPool is called with an engine parameter.
Would you like assistance in generating unit tests for this code?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 152-154: packages/core/src/shader/ShaderPass.ts#L152-L154
Added lines #L152 - L154 were not covered by tests
|
fixed by #2410 |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
ShaderPass._destroyis called,ShaderPass._getShaderProgramcan still got the cachedShaderProgramfromEngine. The PR fix it._clearShaderProgramPoolis a better name than_destroyunder current situation.Summary by CodeRabbit
New Features
ShaderPassobjects with a consolidated constructor.Deprecations
getMacroByNameandgetPropertyByNamemethods as deprecated, encouraging users to adopt new access methods.Bug Fixes
ShaderPassclass.