From d371fa01197dcdb2a32e068b757e33d6e2cbd5eb Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Thu, 11 Jun 2026 20:01:13 +0100 Subject: [PATCH 1/2] refactor(camera): share property list between init and clone Hoist the property list from initializeComponentData to a module-level _properties array (matching light/gsplat/render) and drive cloneComponent from the same list, so the two can no longer drift. The lists had already drifted: cloneComponent was missing fog and clearDepth, so cloning an entity silently dropped those camera settings. The new CameraComponent unit test demonstrated the bug before the fix and now locks in clone behavior for all shared properties. Also drops the unused legacy `properties` parameter from the initializeComponentData override. Co-Authored-By: Claude Fable 5 --- src/framework/components/camera/system.js | 117 +++++++--------- .../components/camera/component.test.mjs | 129 ++++++++++++++++++ 2 files changed, 177 insertions(+), 69 deletions(-) create mode 100644 test/framework/components/camera/component.test.mjs diff --git a/src/framework/components/camera/system.js b/src/framework/components/camera/system.js index ea4c03a09c0..074a0be3552 100644 --- a/src/framework/components/camera/system.js +++ b/src/framework/components/camera/system.js @@ -8,6 +8,40 @@ import { CameraComponent } from './component.js'; * @import { AppBase } from '../../app-base.js' */ +const _properties = [ + 'aspectRatio', + 'aspectRatioMode', + 'calculateProjection', + 'calculateTransform', + 'clearColor', + 'clearColorBuffer', + 'clearDepth', + 'clearDepthBuffer', + 'clearStencilBuffer', + 'renderSceneColorMap', + 'renderSceneDepthMap', + 'cullFaces', + 'farClip', + 'flipFaces', + 'fog', + 'fov', + 'frustumCulling', + 'horizontalFov', + 'layers', + 'renderTarget', + 'nearClip', + 'orthoHeight', + 'projection', + 'priority', + 'rect', + 'scissorRect', + 'aperture', + 'shutter', + 'sensitivity', + 'gammaCorrection', + 'toneMapping' +]; + /** * Used to add and remove {@link CameraComponent}s from Entities. It also holds an array of all * active cameras. @@ -39,43 +73,9 @@ class CameraComponentSystem extends ComponentSystem { this.app.on('prerender', this.onAppPrerender, this); } - initializeComponentData(component, data, properties) { - properties = [ - 'aspectRatio', - 'aspectRatioMode', - 'calculateProjection', - 'calculateTransform', - 'clearColor', - 'clearColorBuffer', - 'clearDepth', - 'clearDepthBuffer', - 'clearStencilBuffer', - 'renderSceneColorMap', - 'renderSceneDepthMap', - 'cullFaces', - 'farClip', - 'flipFaces', - 'fog', - 'fov', - 'frustumCulling', - 'horizontalFov', - 'layers', - 'renderTarget', - 'nearClip', - 'orthoHeight', - 'projection', - 'priority', - 'rect', - 'scissorRect', - 'aperture', - 'shutter', - 'sensitivity', - 'gammaCorrection', - 'toneMapping' - ]; - - for (let i = 0; i < properties.length; i++) { - const property = properties[i]; + initializeComponentData(component, data) { + for (let i = 0; i < _properties.length; i++) { + const property = _properties[i]; if (data.hasOwnProperty(property)) { const value = data[property]; switch (property) { @@ -106,38 +106,17 @@ class CameraComponentSystem extends ComponentSystem { cloneComponent(entity, clone) { const c = entity.camera; - return this.addComponent(clone, { - aspectRatio: c.aspectRatio, - aspectRatioMode: c.aspectRatioMode, - calculateProjection: c.calculateProjection, - calculateTransform: c.calculateTransform, - clearColor: c.clearColor, - clearColorBuffer: c.clearColorBuffer, - clearDepthBuffer: c.clearDepthBuffer, - clearStencilBuffer: c.clearStencilBuffer, - renderSceneDepthMap: c.renderSceneDepthMap, - renderSceneColorMap: c.renderSceneColorMap, - cullFaces: c.cullFaces, - enabled: c.enabled, - farClip: c.farClip, - flipFaces: c.flipFaces, - fov: c.fov, - frustumCulling: c.frustumCulling, - horizontalFov: c.horizontalFov, - layers: c.layers, - renderTarget: c.renderTarget, - nearClip: c.nearClip, - orthoHeight: c.orthoHeight, - projection: c.projection, - priority: c.priority, - rect: c.rect, - scissorRect: c.scissorRect, - aperture: c.aperture, - sensitivity: c.sensitivity, - shutter: c.shutter, - gammaCorrection: c.gammaCorrection, - toneMapping: c.toneMapping - }); + + const data = { + enabled: c.enabled + }; + + for (let i = 0; i < _properties.length; i++) { + const property = _properties[i]; + data[property] = c[property]; + } + + return this.addComponent(clone, data); } onBeforeRemove(entity, component) { diff --git a/test/framework/components/camera/component.test.mjs b/test/framework/components/camera/component.test.mjs new file mode 100644 index 00000000000..a2b1e5ad7b8 --- /dev/null +++ b/test/framework/components/camera/component.test.mjs @@ -0,0 +1,129 @@ +import { expect } from 'chai'; + +import { Color } from '../../../../src/core/math/color.js'; +import { Vec4 } from '../../../../src/core/math/vec4.js'; +import { Entity } from '../../../../src/framework/entity.js'; +import { + ASPECT_MANUAL, + FOG_LINEAR, + LAYERID_UI, + PROJECTION_ORTHOGRAPHIC, + PROJECTION_PERSPECTIVE +} from '../../../../src/scene/constants.js'; +import { FogParams } from '../../../../src/scene/fog-params.js'; +import { createApp } from '../../../app.mjs'; +import { jsdomSetup, jsdomTeardown } from '../../../jsdom.mjs'; + +describe('CameraComponent', function () { + let app; + + beforeEach(function () { + jsdomSetup(); + app = createApp(); + }); + + afterEach(function () { + app?.destroy(); + app = null; + jsdomTeardown(); + }); + + describe('#addComponent', function () { + + it('creates a component with sensible defaults', function () { + const e = new Entity(); + e.addComponent('camera'); + + expect(e.camera).to.exist; + expect(e.camera.enabled).to.equal(true); + expect(e.camera.fov).to.equal(45); + expect(e.camera.nearClip).to.equal(0.1); + expect(e.camera.farClip).to.equal(1000); + expect(e.camera.clearColor.equals(new Color(0.75, 0.75, 0.75, 1))).to.equal(true); + expect(e.camera.clearDepth).to.equal(1); + expect(e.camera.fog).to.equal(null); + expect(e.camera.orthoHeight).to.equal(10); + expect(e.camera.priority).to.equal(0); + expect(e.camera.projection).to.equal(PROJECTION_PERSPECTIVE); + }); + + }); + + describe('#cloneComponent', function () { + + it('copies every property to the clone', function () { + const fog = new FogParams(); + fog.type = FOG_LINEAR; + fog.start = 10; + fog.end = 100; + + const e = new Entity(); + e.addComponent('camera', { + aspectRatioMode: ASPECT_MANUAL, + aspectRatio: 2, + clearColor: new Color(0.1, 0.2, 0.3, 0.4), + clearColorBuffer: false, + clearDepth: 0.5, + clearDepthBuffer: false, + clearStencilBuffer: false, + cullFaces: false, + farClip: 500, + flipFaces: true, + fog: fog, + fov: 60, + frustumCulling: false, + horizontalFov: true, + layers: [LAYERID_UI], + nearClip: 2, + orthoHeight: 7, + priority: 3, + projection: PROJECTION_ORTHOGRAPHIC, + rect: new Vec4(0.1, 0.1, 0.5, 0.5), + scissorRect: new Vec4(0.2, 0.2, 0.6, 0.6), + aperture: 8, + shutter: 1 / 500, + sensitivity: 400 + }); + + const clone = e.clone(); + const c = clone.camera; + + expect(c).to.exist; + expect(c.enabled).to.equal(true); + expect(c.aspectRatioMode).to.equal(ASPECT_MANUAL); + expect(c.aspectRatio).to.equal(2); + expect(c.clearColor.equals(new Color(0.1, 0.2, 0.3, 0.4))).to.equal(true); + expect(c.clearColorBuffer).to.equal(false); + expect(c.clearDepth).to.equal(0.5); + expect(c.clearDepthBuffer).to.equal(false); + expect(c.clearStencilBuffer).to.equal(false); + expect(c.cullFaces).to.equal(false); + expect(c.farClip).to.equal(500); + expect(c.flipFaces).to.equal(true); + expect(c.fog).to.equal(fog); + expect(c.fov).to.equal(60); + expect(c.frustumCulling).to.equal(false); + expect(c.horizontalFov).to.equal(true); + expect(c.layers).to.deep.equal([LAYERID_UI]); + expect(c.nearClip).to.equal(2); + expect(c.orthoHeight).to.equal(7); + expect(c.priority).to.equal(3); + expect(c.projection).to.equal(PROJECTION_ORTHOGRAPHIC); + expect(c.rect.equals(new Vec4(0.1, 0.1, 0.5, 0.5))).to.equal(true); + expect(c.scissorRect.equals(new Vec4(0.2, 0.2, 0.6, 0.6))).to.equal(true); + expect(c.aperture).to.equal(8); + expect(c.shutter).to.equal(1 / 500); + expect(c.sensitivity).to.equal(400); + }); + + it('copies the enabled state to the clone', function () { + const e = new Entity(); + e.addComponent('camera', { enabled: false }); + + const clone = e.clone(); + + expect(clone.camera.enabled).to.equal(false); + }); + + }); +}); From 71eebd9fc4b977b0c204491c3d044cd63bcc04dd Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Thu, 11 Jun 2026 20:09:50 +0100 Subject: [PATCH 2/2] test(camera): cover callbacks and gamma/tone properties in clone test Address review feedback: set non-default values for calculateProjection, calculateTransform, gammaCorrection and toneMapping in the clone round-trip test so regressions in cloning those fields are caught. Co-Authored-By: Claude Fable 5 --- .../components/camera/component.test.mjs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/test/framework/components/camera/component.test.mjs b/test/framework/components/camera/component.test.mjs index a2b1e5ad7b8..6823db9f253 100644 --- a/test/framework/components/camera/component.test.mjs +++ b/test/framework/components/camera/component.test.mjs @@ -6,9 +6,11 @@ import { Entity } from '../../../../src/framework/entity.js'; import { ASPECT_MANUAL, FOG_LINEAR, + GAMMA_NONE, LAYERID_UI, PROJECTION_ORTHOGRAPHIC, - PROJECTION_PERSPECTIVE + PROJECTION_PERSPECTIVE, + TONEMAP_ACES } from '../../../../src/scene/constants.js'; import { FogParams } from '../../../../src/scene/fog-params.js'; import { createApp } from '../../../app.mjs'; @@ -57,10 +59,15 @@ describe('CameraComponent', function () { fog.start = 10; fog.end = 100; + const calculateProjection = () => {}; + const calculateTransform = () => {}; + const e = new Entity(); e.addComponent('camera', { aspectRatioMode: ASPECT_MANUAL, aspectRatio: 2, + calculateProjection: calculateProjection, + calculateTransform: calculateTransform, clearColor: new Color(0.1, 0.2, 0.3, 0.4), clearColorBuffer: false, clearDepth: 0.5, @@ -82,7 +89,9 @@ describe('CameraComponent', function () { scissorRect: new Vec4(0.2, 0.2, 0.6, 0.6), aperture: 8, shutter: 1 / 500, - sensitivity: 400 + sensitivity: 400, + gammaCorrection: GAMMA_NONE, + toneMapping: TONEMAP_ACES }); const clone = e.clone(); @@ -92,6 +101,8 @@ describe('CameraComponent', function () { expect(c.enabled).to.equal(true); expect(c.aspectRatioMode).to.equal(ASPECT_MANUAL); expect(c.aspectRatio).to.equal(2); + expect(c.calculateProjection).to.equal(calculateProjection); + expect(c.calculateTransform).to.equal(calculateTransform); expect(c.clearColor.equals(new Color(0.1, 0.2, 0.3, 0.4))).to.equal(true); expect(c.clearColorBuffer).to.equal(false); expect(c.clearDepth).to.equal(0.5); @@ -114,6 +125,8 @@ describe('CameraComponent', function () { expect(c.aperture).to.equal(8); expect(c.shutter).to.equal(1 / 500); expect(c.sensitivity).to.equal(400); + expect(c.gammaCorrection).to.equal(GAMMA_NONE); + expect(c.toneMapping).to.equal(TONEMAP_ACES); }); it('copies the enabled state to the clone', function () {