Fix material clone name #2576
Fix material clone name #2576
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2576 +/- ##
==========================================
- Coverage 68.95% 68.95% -0.01%
==========================================
Files 961 961
Lines 100298 100304 +6
Branches 8667 8665 -2
==========================================
+ Hits 69163 69165 +2
- Misses 30875 30879 +4
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
WalkthroughThe pull request updates several core components by modifying naming conventions and cloning logic. In the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Material
Client->>Material: clone()
Material->>Material: _cloneToAndModifyName(target)
Note right of Material: Internally calls cloneTo() and appends "(Clone)" to the original name.
Material-->>Client: Returns cloned Material with modified name
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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/Renderer.ts (1)
487-494:⚠️ Potential issueImportant fix for material instance naming
The change correctly sets the instance material name to be based on the original material's name rather than potentially using undefined or incorrect naming. This ensures consistent naming of instance materials in the engine.
Before this fix, instance materials might have had names like "undefined(Instance)" if the original material's name wasn't properly set, leading to confusing debugging experiences.
🧹 Nitpick comments (2)
tests/src/core/MeshRenderer.test.ts (1)
199-200: Remove debug log statementThe console.log statement should be removed before merging as it's likely just for debugging purposes during development.
-console.log(material.name);packages/core/src/material/Material.ts (1)
116-119: Consider handling multiple clone operations more elegantly.While the current implementation works well for single cloning operations, cloning an already-cloned material will result in names like "Material(Clone)(Clone)". This could become unwieldy with multiple levels of cloning.
protected _cloneToAndModifyName(target: Material): void { this.cloneTo(target); - target.name = this.name + "(Clone)"; + // Avoid appending multiple "(Clone)" suffixes + target.name = this.name.endsWith("(Clone)") + ? this.name + : this.name + "(Clone)"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/core/src/Entity.ts(1 hunks)packages/core/src/Renderer.ts(1 hunks)packages/core/src/material/BaseMaterial.ts(1 hunks)packages/core/src/material/BlinnPhongMaterial.ts(1 hunks)packages/core/src/material/Material.ts(2 hunks)packages/core/src/material/PBRMaterial.ts(1 hunks)packages/core/src/material/PBRSpecularMaterial.ts(1 hunks)packages/core/src/material/UnlitMaterial.ts(1 hunks)packages/core/src/particle/ParticleMaterial.ts(1 hunks)packages/core/src/sky/SkyBoxMaterial.ts(1 hunks)packages/core/src/sky/SkyProceduralMaterial.ts(1 hunks)tests/src/core/MeshRenderer.test.ts(2 hunks)tests/src/core/material/Material.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/sky/SkyBoxMaterial.ts
[warning] 99-99: packages/core/src/sky/SkyBoxMaterial.ts#L99
Added line #L99 was not covered by tests
packages/core/src/particle/ParticleMaterial.ts
[warning] 59-59: packages/core/src/particle/ParticleMaterial.ts#L59
Added line #L59 was not covered by tests
packages/core/src/sky/SkyProceduralMaterial.ts
[warning] 152-152: packages/core/src/sky/SkyProceduralMaterial.ts#L152
Added line #L152 was not covered by tests
packages/core/src/material/BaseMaterial.ts
[warning] 223-223: packages/core/src/material/BaseMaterial.ts#L223
Added line #L223 was not covered by tests
🔇 Additional comments (14)
tests/src/core/material/Material.test.ts (1)
95-101: Test coverage added for material clone naming. Great!This test ensures that material clones follow the expected naming convention, where "(Clone)" is appended to the original material name. This helps validate the changes made to the cloning mechanism across material types.
packages/core/src/material/PBRMaterial.ts (1)
487-487: Consistent cloning mechanism adopted.Changed from using
cloneToto_cloneToAndModifyNamewhich ensures that cloned materials have a standardized naming convention that includes "(Clone)" suffix, making it easier to identify cloned materials.packages/core/src/Entity.ts (1)
254-254: Simplified default entity naming.The default name for entities has been simplified from "New Entity" to just "Entity", which is more concise and aligns better with common naming conventions.
packages/core/src/material/UnlitMaterial.ts (1)
76-76: Consistent cloning mechanism applied.Changed from
cloneToto_cloneToAndModifyNameto maintain consistency with the clone naming convention across all material types. This ensures that cloned UnlitMaterial instances will have the "(Clone)" suffix in their names.packages/core/src/material/PBRSpecularMaterial.ts (1)
76-79: Good improvement on material clone naming!The change from
cloneToto_cloneToAndModifyNameis a positive improvement that ensures cloned materials have a meaningful name based on the original material. This promotes better debug experiences and makes it easier to identify cloned materials in the scene hierarchy.packages/core/src/particle/ParticleMaterial.ts (1)
57-60:❓ Verification inconclusive
Good improvement on material clone naming!
The change from
cloneToto_cloneToAndModifyNameensures cloned particle materials have a meaningful name based on the original material, which is consistent with the naming changes in other material types.However, I notice that this line is flagged as not covered by tests according to the static analysis:
🏁 Script executed:
#!/bin/bash # Verify if there are tests for the ParticleMaterial clone method # Search for any tests targeting ParticleMaterial clone functionality rg -A 5 "ParticleMaterial.*clone" --glob "tests/**/*.test.ts"Length of output: 63
Attention: Verify Test Coverage for ParticleMaterial.clone()
Good improvement on the material clone naming! Changing from
cloneToto_cloneToAndModifyNameensures that cloned particle materials automatically receive a meaningful name based on the original material, keeping the behavior consistent with other material types.However, static analysis flagged that the
clonemethod inpackages/core/src/particle/ParticleMaterial.ts(lines 57–60) is not covered by tests. The recent search did not return any test cases for this functionality. Please manually verify whether tests for theclonefunctionality exist or need to be added. If missing, it would be beneficial to include a test case to safeguard against regressions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 59-59: packages/core/src/particle/ParticleMaterial.ts#L59
Added line #L59 was not covered by teststests/src/core/MeshRenderer.test.ts (2)
197-208: Test update correctly validates material instance namingThe test expectations have been updated to match the new material instance naming convention, which is good. Now the test properly expects "unlit(Instance)" and "pbr(Instance)" instead of potentially incorrect names like "undefined(Instance)".
223-226: Test update correctly validates material instance naming in arraysThe updated expectations for instance material names in arrays correctly validate the new naming convention. This ensures the consistency of naming across all instance materials, whether accessed individually or as part of a collection.
packages/core/src/material/BaseMaterial.ts (1)
222-224:✅ Verification successful
Consistent cloning approach implemented
The switch from
cloneToto_cloneToAndModifyNameensures consistent handling of material names during cloning, which addresses the primary goal of this PR to fix material clone naming.
🏁 Script executed:
#!/bin/bash # Check the implementation of the _cloneToAndModifyName method in the Material class rg -A 6 "_cloneToAndModifyName" --type ts packages/core/src/material/Material.tsLength of output: 374
Consistent Cloning Implementation Verified
The changes inpackages/core/src/material/BaseMaterial.tscorrectly instantiate a newBaseMaterialand use_cloneToAndModifyName, which callscloneToand appends "(Clone)" to the material’s name. This confirms that material clone naming is now handled as intended.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 223-223: packages/core/src/material/BaseMaterial.ts#L223
Added line #L223 was not covered by testspackages/core/src/material/BlinnPhongMaterial.ts (1)
179-181: Consistent naming convention for material clonesThe implementation now uses the centralized
_cloneToAndModifyNamemethod which ensures consistent naming of cloned materials across different material types.packages/core/src/sky/SkyProceduralMaterial.ts (1)
151-153: Cloning consistency improved with shared methodUsing
_cloneToAndModifyNamealigns this material's cloning behavior with the rest of the material system, ensuring proper naming of cloned instances.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 152-152: packages/core/src/sky/SkyProceduralMaterial.ts#L152
Added line #L152 was not covered by testspackages/core/src/material/Material.ts (3)
87-87: Improved material naming with shader reference.Initializing the material name based on the shader name provides better default naming semantics and consistency throughout the application.
93-96: Better naming convention for cloned materials.Using the new method to create a unique name for cloned materials improves clarity when working with cloned instances.
87-87:✅ Verification successful
Test the correct cloning behavior across different material types.
Since this change affects how materials are named during cloning, ensure that this behavior is consistent across all material types, especially if some materials might have custom naming logic.
Also applies to: 116-119
🏁 Script executed:
#!/bin/bash # Check all material classes to verify consistent implementation of the _cloneToAndModifyName method echo "Checking for material classes with clone methods:" grep -r "clone()" --include="*.ts" packages/core/src/material/ echo "\nChecking for _cloneToAndModifyName implementations:" grep -r "_cloneToAndModifyName" --include="*.ts" packages/core/src/material/ echo "\nChecking for material test cases that verify clone naming:" grep -r "clone.*name.*Clone" --include="*.test.ts" tests/Length of output: 1725
Ensure Consistent Clone Naming Across All Material Types
The tests confirm that every material class calls
_cloneToAndModifyNamein theirclone()methods and that the naming convention (e.g., "blinn-phong(Clone)") is enforced through dedicated tests. Please make sure that the changes in Material.ts (lines 87 and 116–119) continue to work seamlessly with custom clone logic in other material types.
| const dest = new SkyBoxMaterial(this._engine); | ||
| this.cloneTo(dest); | ||
| this._cloneToAndModifyName(dest); | ||
| return dest; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for cloning functionality
The new cloning approach ensures consistent naming, but according to static analysis, this change lacks test coverage.
🏁 Script executed:
#!/bin/bash
# Check if there are any material cloning tests
rg -A 3 "clone\(\)" --type ts tests/src/core/materialLength of output: 2728
Action: Add dedicated test for SkyBoxMaterial cloning functionality
The new cloning approach in packages/core/src/sky/SkyBoxMaterial.ts (lines 98–100) appears to be working as intended. However, while tests exist for other material clone methods (e.g., in tests/src/core/material/Material.test.ts and similar files), there isn’t any direct test covering the cloning behavior of SkyBoxMaterial. Please add a test specifically for this class to ensure that the helper _cloneToAndModifyName correctly updates the cloned instance (such as validating its naming convention and any other critical properties).
- Consider placing the new test in a suitable location, for example in a new file under
tests/src/core/skyor in an existing material tests file if appropriate. - Verify that the cloned instance’s properties (especially the name) conform to the consistent naming convention established by the cloning functionality.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 99-99: packages/core/src/sky/SkyBoxMaterial.ts#L99
Added line #L99 was not covered by tests
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
Bug Fixes
Tests