fix(camera): mark replacement shader fields as assignment clone#2988
fix(camera): mark replacement shader fields as assignment clone#2988cptbtptpbcptdtptp wants to merge 2 commits into
Conversation
Add @assignmentClone to _replacementShader and _replacementSubShaderTag so cloned cameras share the same shader/tag reference instead of deep cloning them (which would create unnecessary duplicates). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughCamera's internal replacement-shader state ( ChangesReplacement Shader Clone Preservation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/src/core/Camera.test.ts`:
- Around line 432-453: Replace the string assignment and string comparison with
a proper ShaderTagKey: create a ShaderTagKey via
ShaderTagKey.getByName("TestTag"), assign that to
camera._replacementSubShaderTag (instead of the raw string), and update the
assertion to compare the cloned camera's _replacementSubShaderTag to the same
ShaderTagKey (or compare their names via .getName()) so types and semantics
match; references: camera._replacementSubShaderTag, ShaderTagKey.getByName,
cloneCamera.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1688ce5f-7744-4d70-9cf9-41db4bc2fbf8
📒 Files selected for processing (2)
packages/core/src/Camera.tstests/src/core/Camera.test.ts
| it("clone preserves replacement shader by reference (assignmentClone)", () => { | ||
| const shader = Shader.create("TestCloneReplaceShader", [ | ||
| new SubShader("Default", [new ShaderPass("Default", [], [], ShaderLanguage.GLSLES100)]) | ||
| ]); | ||
|
|
||
| // @ts-ignore | ||
| camera._replacementShader = shader; | ||
| // @ts-ignore | ||
| camera._replacementSubShaderTag = "TestTag"; | ||
|
|
||
| const cloneCamera = camera.entity.clone().getComponent(Camera); | ||
|
|
||
| // @ts-ignore - assignmentClone copies reference, not deep clone | ||
| expect(cloneCamera._replacementShader).to.eq(shader); | ||
| // @ts-ignore | ||
| expect(cloneCamera._replacementSubShaderTag).to.eq("TestTag"); | ||
|
|
||
| // @ts-ignore - cleanup | ||
| camera._replacementShader = null; | ||
| // @ts-ignore | ||
| camera._replacementSubShaderTag = null; | ||
| }); |
There was a problem hiding this comment.
Fix type violations in replacement shader tag assignment and assertion.
The test has two critical correctness issues:
-
Line 440: Assigns a string
"TestTag"directly to_replacementSubShaderTag, but this field is typed asShaderTagKey, notstring. While@ts-ignoresuppresses the type error, this violates the actual type contract. -
Line 447: Compares
_replacementSubShaderTagto the string"TestTag". If line 440 is corrected to use a properShaderTagKeyobject, this assertion will fail since you'd be comparing an object to a string.
The proper usage pattern is shown in Camera.ts line 719: ShaderTagKey.getByName(replacementTag).
🔧 Proposed fix using ShaderTagKey.getByName()
+ const testTag = ShaderTagKey.getByName("TestTag");
+
// `@ts-ignore`
camera._replacementShader = shader;
// `@ts-ignore`
- camera._replacementSubShaderTag = "TestTag";
+ camera._replacementSubShaderTag = testTag;
const cloneCamera = camera.entity.clone().getComponent(Camera);
// `@ts-ignore` - assignmentClone copies reference, not deep clone
expect(cloneCamera._replacementShader).to.eq(shader);
// `@ts-ignore`
- expect(cloneCamera._replacementSubShaderTag).to.eq("TestTag");
+ expect(cloneCamera._replacementSubShaderTag).to.eq(testTag);📝 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.
| it("clone preserves replacement shader by reference (assignmentClone)", () => { | |
| const shader = Shader.create("TestCloneReplaceShader", [ | |
| new SubShader("Default", [new ShaderPass("Default", [], [], ShaderLanguage.GLSLES100)]) | |
| ]); | |
| // @ts-ignore | |
| camera._replacementShader = shader; | |
| // @ts-ignore | |
| camera._replacementSubShaderTag = "TestTag"; | |
| const cloneCamera = camera.entity.clone().getComponent(Camera); | |
| // @ts-ignore - assignmentClone copies reference, not deep clone | |
| expect(cloneCamera._replacementShader).to.eq(shader); | |
| // @ts-ignore | |
| expect(cloneCamera._replacementSubShaderTag).to.eq("TestTag"); | |
| // @ts-ignore - cleanup | |
| camera._replacementShader = null; | |
| // @ts-ignore | |
| camera._replacementSubShaderTag = null; | |
| }); | |
| it("clone preserves replacement shader by reference (assignmentClone)", () => { | |
| const shader = Shader.create("TestCloneReplaceShader", [ | |
| new SubShader("Default", [new ShaderPass("Default", [], [], ShaderLanguage.GLSLES100)]) | |
| ]); | |
| const testTag = ShaderTagKey.getByName("TestTag"); | |
| // `@ts-ignore` | |
| camera._replacementShader = shader; | |
| // `@ts-ignore` | |
| camera._replacementSubShaderTag = testTag; | |
| const cloneCamera = camera.entity.clone().getComponent(Camera); | |
| // `@ts-ignore` - assignmentClone copies reference, not deep clone | |
| expect(cloneCamera._replacementShader).to.eq(shader); | |
| // `@ts-ignore` | |
| expect(cloneCamera._replacementSubShaderTag).to.eq(testTag); | |
| // `@ts-ignore` - cleanup | |
| camera._replacementShader = null; | |
| // `@ts-ignore` | |
| camera._replacementSubShaderTag = null; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/core/Camera.test.ts` around lines 432 - 453, Replace the string
assignment and string comparison with a proper ShaderTagKey: create a
ShaderTagKey via ShaderTagKey.getByName("TestTag"), assign that to
camera._replacementSubShaderTag (instead of the raw string), and update the
assertion to compare the cloned camera's _replacementSubShaderTag to the same
ShaderTagKey (or compare their names via .getName()) so types and semantics
match; references: camera._replacementSubShaderTag, ShaderTagKey.getByName,
cloneCamera.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2988 +/- ##
===========================================
+ Coverage 78.25% 78.30% +0.04%
===========================================
Files 900 900
Lines 99234 99236 +2
Branches 10172 10192 +20
===========================================
+ Hits 77657 77705 +48
+ Misses 21406 21360 -46
Partials 171 171
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
所有历史问题均已关闭。
总结
为 Camera._replacementShader 和 _replacementSubShaderTag 添加 @assignmentClone，修复 clone camera 时对 Shader 全局资源做不必要深拷贝的问题。改动精准，方向正确，测试验证了 clone 后引用相等性。
[P2] Camera.test.ts — 测试用裸字符串 "TestTag" 赋值给 _replacementSubShaderTag（类型应为 ShaderTagKey）
当前测试通过 @ts-ignore 压制类型错误，验证的是字符串的 reference 语义，无法反映真实使用中 ShaderTagKey 对象的 clone 行为。建议改为：
import { ShaderTagKey } from "@galacean/engine-core";
const tagKey = ShaderTagKey.getByName("TestTag");
// @ts-ignore
camera._replacementSubShaderTag = tagKey;
// ...
// @ts-ignore
expect(cloneCamera._replacementSubShaderTag).to.eq(tagKey);不阻塞合并，LGTM。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
所有历史问题均已关闭。
总结
为 Camera._replacementShader 和 _replacementSubShaderTag 两个字段加 @assignmentClone，修复克隆时对 Shader 对象的无意义深克隆。测试验证了 clone 后引用保持一致。修复点精准，代码改动最小。
无新问题。可以合并。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
单行修复：给 Camera 的 _replacementShader 和 _replacementSubShaderTag 加 @assignmentClone，避免 clone 时深拷贝 Shader 对象。修复正确，测试覆盖了 reference equality。无问题，LGTM。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
修复方向正确。Shader 是引擎共享资产，克隆时应保持引用相同而非创建空壳。加 @assignmentClone 语义明确，测试覆盖完整。
问题
[P2] _replacementFailureStrategy 同样无装饰器，需确认类型
_replacementFailureStrategy: ReplacementFailureStrategy = null 没有装饰器。若 ReplacementFailureStrategy 是 TypeScript enum（数字），sourceProperty instanceof Object 判断为 false，走直接赋值，行为正确；若是 class/object，则在 PR #2992 的 _inferCloneMode 引入之前需要补 @assignmentClone。请确认类型或补加装饰器。
[P2] 需对齐与 PR #2992 的合并顺序
PR #2992 引入 _inferCloneMode，其 fallback 已能正确处理 class 实例的 assignment 语义（Shader 会走 Assignment 路径）。若 #2992 先于本 PR 合入，本 PR 的修复成为"防御性标注"（非必需但有益于可读性）；若本 PR 先合入，#2992 合入后需回归验证不冲突（理论上显式装饰器优先于推断，不存在冲突）。两个 PR 需对齐合并顺序。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
为 Camera._replacementShader 和 _replacementSubShaderTag 两个字段加 @assignmentClone，修复克隆时对 Shader 全局资源做不必要深克隆的问题。Shader 是引擎全局单例（通过 Shader.find(name) 统一管理），clone 时应共享引用而非创建空壳。测试验证了 clone 后引用相等性。
问题
[P2] Camera.test.ts 中 _replacementSubShaderTag 赋字符串而非 ShaderTagKey
测试直接将字符串 "TestTag" 赋给 _replacementSubShaderTag，该字段类型为 ShaderTagKey，通过 @ts-ignore 压制类型错误。这能验证字符串的引用保持，但无法反映真实使用中 ShaderTagKey 对象的 clone 行为。建议改为：
const tagKey = ShaderTagKey.getByName("TestTag");
// @ts-ignore
camera._replacementSubShaderTag = tagKey;
// @ts-ignore
expect(cloneCamera._replacementSubShaderTag).to.eq(tagKey);P2，不阻塞合并。整体修复精准干净。LGTM。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
所有历史问题均已关闭。
总结
为 Camera._replacementShader 和 _replacementSubShaderTag 添加 @assignmentClone，修复 clone 时对 Shader 全局资产做不必要深克隆的问题。Shader 通过 Shader.find(name) 统一管理，clone 时应共享引用。测试验证了 clone 后引用相等性。
[P2] Camera.ts:134 — _replacementFailureStrategy 无装饰器，确认正确
_replacementFailureStrategy: ReplacementFailureStrategy = null，ReplacementFailureStrategy 是 enum（本质为 number），原始类型会被 cloneProperty 直接 assign，无需装饰器。已确认，行为正确。
[P2] Camera.test.ts:442 — _replacementSubShaderTag 赋字符串而非 ShaderTagKey
建议改为 ShaderTagKey.getByName("TestTag") 以测试真实对象类型的 reference equality（而非字符串，通过 === 比较时字符串字面量可能因 interning 而意外通过）。不阻塞合并。
无新 P0/P1，LGTM，可合入。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
为 Camera._replacementShader 和 _replacementSubShaderTag 添加 @assignmentClone 装饰器，避免 clone 时深拷贝 Shader 对象。修复精准，改动最小（2 行 + 1 个测试），语义正确：Shader 是引擎全局注册的共享资产，clone 时保持同一引用是正确行为。
问题
无 P0/P1，LGTM，可合入。
[P3] 测试用 new SubShader(...) / new ShaderPass(...) 直接构造 Shader 对象验证 assignment，逻辑没有问题，但若后续 ShaderPass 构造签名变化测试会随之失效，如果有 Shader.find(name) 可用的简单 fixture 会更稳定（非阻塞）。
2 participants
Summary
@assignmentCloneto_replacementShaderand_replacementSubShaderTagso cloned cameras share the same shader/tag reference instead of deep cloning them.Background
Extracted from
ca5252a9aon fix/shaderlab. Without this, cloning a camera that has a replacement shader set would deep-clone the Shader object, creating an unnecessary duplicate.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit