Skip to content

fix(camera): mark replacement shader fields as assignment clone #2988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cptbtptpbcptdtptp wants to merge 2 commits into
base: dev/2.0
Choose a base branch
from
Open

fix(camera): mark replacement shader fields as assignment clone #2988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f90e66d
fix(camera): mark replacement shader fields as assignment clone
cptbtptpbcptdtptp May 11, 2026
9845234
test(camera): verify replacement shader fields use assignment clone
cptbtptpbcptdtptp May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/core/src/Camera.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { Transform } from "./Transform";
import { UpdateFlagManager } from "./UpdateFlagManager";
import { VirtualCamera } from "./VirtualCamera";
import { GLCapabilityType, Logger } from "./base";
import { deepClone, ignoreClone } from "./clone/CloneManager";
import { assignmentClone, deepClone, ignoreClone } from "./clone/CloneManager";
import { AntiAliasing } from "./enums/AntiAliasing";
import { CameraClearFlags } from "./enums/CameraClearFlags";
import { CameraModifyFlags } from "./enums/CameraModifyFlags";
Expand Down Expand Up @@ -125,8 +125,10 @@ export class Camera extends Component {
@deepClone
_virtualCamera: VirtualCamera = new VirtualCamera();
/** @internal */
@assignmentClone
_replacementShader: Shader = null;
/** @internal */
@assignmentClone
_replacementSubShaderTag: ShaderTagKey = null;
/** @internal */
_replacementFailureStrategy: ReplacementFailureStrategy = null;
Expand Down
29 changes: 26 additions & 3 deletions tests/src/core/Camera.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ describe("camera test", function () {
// get enableHDR
expect(camera.enableHDR).to.eq(false);
// @ts-ignore
expect(camera._isIndependentCanvasEnabled()).to.eq(true);// Because sRGB pass
expect(camera._isIndependentCanvasEnabled()).to.eq(true); // Because sRGB pass
// set enableHDR
camera.enableHDR = true;
expect(camera.enableHDR).to.eq(true);
Expand Down Expand Up @@ -420,14 +420,37 @@ describe("camera test", function () {
camera.nearClipPlane = 1;
camera.farClipPlane = 255;
const cloneCamera = camera.entity.clone().getComponent(Camera);
expect(cloneCamera.isOrthographic).to.eq(camera.isOrthographic)
expect(cloneCamera.isOrthographic).to.eq(camera.isOrthographic);
expect(cloneCamera.nearClipPlane).to.eq(camera.nearClipPlane);
expect(cloneCamera.farClipPlane).to.eq(camera.farClipPlane);
expect(cloneCamera.renderTarget).to.eq(camera.renderTarget);
expect(cloneCamera.shaderData).to.not.eq(camera.shaderData);
// @ts-ignore
expect(cloneCamera._globalShaderMacro).to.not.eq(camera._globalShaderMacro);
})
});

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;
});
Comment on lines +432 to +453
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix type violations in replacement shader tag assignment and assertion.

The test has two critical correctness issues:

  1. Line 440: Assigns a string "TestTag" directly to _replacementSubShaderTag, but this field is typed as ShaderTagKey, not string. While @ts-ignore suppresses the type error, this violates the actual type contract.

  2. Line 447: Compares _replacementSubShaderTag to the string "TestTag". If line 440 is corrected to use a proper ShaderTagKey object, 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.

Suggested change
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.


it("destroy test", () => {
camera.destroy();
Expand Down
Loading