Skip to content

refactor(light): align LightComponent with CameraComponent architecture#8666

Merged
willeastcott merged 11 commits into
mainfrom
refactor/light-component-camera-style
May 3, 2026
Merged

refactor(light): align LightComponent with CameraComponent architecture#8666
willeastcott merged 11 commits into
mainfrom
refactor/light-component-camera-style

Conversation

@willeastcott
Copy link
Copy Markdown
Contributor

@willeastcott willeastcott commented May 2, 2026

Summary

Refactor LightComponent to mirror CameraComponent's architecture, paving the way for eliminating *ComponentData classes engine-wide.

  • LightComponent now owns its Light instance (_light) directly and constructs it in its own constructor, mirroring how CameraComponent owns _camera.
  • Every property is either a direct delegate to this._light or backed by a small private field on the component. Properties no longer round-trip through data[name] and the _setValue helper has been removed.
  • LightComponentData is reduced to a single enabled field. The properties export is gone.
  • LightComponentSystem matches CameraComponentSystem: _schema = ['enabled'], explicit property lists drive initializeComponentData and cloneComponent, and Component._buildAccessors(LightComponent.prototype, _schema) handles the enabled accessor. The previous changeType system method was a one-liner with no system state and is now inlined into the component's type setter, so the system is pure boilerplate.

Public API

No additions, removals, or behaviour changes. The full property surface of LightComponent is preserved (verified against the regenerated .d.ts), including round-tripping 'point' as 'point' (rather than normalising it to 'omni').

Test plan

  • npm run lint passes
  • npm run build:types regenerates .d.ts cleanly; npm run test:types passes
  • npm test — 1653 passing, 0 failing

LightComponent now owns its Light instance directly and delegates almost
every property to it, mirroring the CameraComponent pattern. The bespoke
_setValue / data[name] indirection is removed, LightComponentData is
reduced to `enabled`, and the LightComponentSystem is now pure
boilerplate. This is a step toward eliminating *ComponentData classes
entirely.

Co-authored-by: Cursor <cursoragent@cursor.com>
The reverse map is purely a presentation concern of the component;
Light itself only deals with int types. Move it out of scene/light.js
so we don't expose an extra public symbol that nothing else needs.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors LightComponent to mirror CameraComponent’s architecture by having the component directly own and manage its underlying Light instance, reducing reliance on *ComponentData property bags.

Changes:

  • LightComponent now constructs/owns this._light and delegates most accessors directly to it; _setValue and data[name] round-tripping are removed.
  • LightComponentData is reduced to enabled only; LightComponentSystem uses _schema = ['enabled'] and an explicit property list for init/clone.
  • Adds lightTypeNames reverse lookup (and export) to support normalized LightComponent.type getter behavior; updates entity test expectation to 'omni'.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/framework/entity.test.mjs Updates assertion to match normalized light type read-back ('omni').
src/scene/light.js Adds/export lightTypeNames reverse map for light type string getters.
src/framework/components/light/system.js Aligns system with CameraComponentSystem pattern: schema-only enabled, explicit init/clone via _properties.
src/framework/components/light/data.js Shrinks data class to enabled only, removing legacy property bag.
src/framework/components/light/component.js Moves Light ownership into component and rewires accessors to delegate to this._light / small private fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/framework/components/light/component.js Outdated
Cache the user's input type string in a private `_type` field so that
`entity.light.type` round-trips `'point'` rather than normalising to
`'omni'`. Avoids a small breaking change that the previous commit
introduced.

Co-authored-by: Cursor <cursoragent@cursor.com>
The _properties array drives both initializeComponentData and
cloneComponent, so omitting these accessors meant they could no longer
be set via addComponent('light', { ... }) or carried over by clone.
Restores the missing entries: penumbraSize, penumbraFalloff,
shadowSamples, shadowBlockerSamples.

Co-authored-by: Cursor <cursoragent@cursor.com>
Cover addComponent property round-trip, cloneComponent (including deep-cloning of Color/Vec2 references), the 'point'/'omni' alias preservation, shadowBias scaling, cookieTransform setup/clear, mask flag interactions (affectDynamic, affectLightmapped, bake), layer attachment on enable/disable and the directional-only behaviour of affectSpecularity.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/framework/components/light/system.js Outdated
The legacy enable alias was only forwarded to enabled when truthy, so �ddComponent('light', { enable: false }) silently left the component enabled. Use a hasOwnProperty check so alse is honoured (and the deprecation warning still fires whenever the key is supplied).

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/framework/components/light/component.js Outdated
Comment thread src/framework/components/light/component.js
…color through public Light API

- Type setter: remove the light from its layers BEFORE mutating Light.type. Layer#removeLight gates _clusteredLightsSet.delete on the light's *current* type, so changing type first leaked spot/omni entries when transitioning to directional.

- color getter now goes through Light#getColor() instead of reaching into the private _color field, matching the rest of the component's public-API delegation.

Adds regression tests for both behaviours.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/framework/components/light/component.js
Comment thread src/framework/components/light/component.js Outdated
…e component

- Add component-side _castShadows backing field. Light#castShadows is a mask-aware getter (returns false when _mask === MASK_BAKE or 0), so reaching into _light._castShadows returned the raw user value but coupled the component to a private field. The new field stores the user's intent and the setter forwards to Light#castShadows for the effective behaviour.

- Add component-side _affectSpecularity backing field. Light#affectSpecularity silently ignores writes for non-directional lights, so the previous code lost the user's value when the type was spot/omni and the value never re-applied if the type later became directional. The setter now stores on the component unconditionally and 
efreshProperties() (called from set type) re-applies through the Light setter once the type is directional.

Updates the round-trip and clone tests to cover �ffectSpecularity: false and adds dedicated tests for the new behaviours plus a castShadows/mask preservation test.

Co-authored-by: Cursor <cursoragent@cursor.com>
…tests

Three properties were being asserted at their default values (vsmBlurMode: BLUR_GAUSSIAN, cookieChannel: 'rgb', bake: false), so the round-trip and clone assertions for those keys were trivially passing — the setters early-return when the new value matches the current one, meaning a regression that dropped the property entirely from initializeComponentData / cloneComponent would not have been caught. Switched to BLUR_BOX, 'rrr' and bake: true (with affectLightmapped flipped to false to keep them mask-compatible). Mask is intentionally still omitted because affectDynamic/affectLightmapped/bake setters mutate the mask after a direct write — the dedicated 'mask flags' suite covers that interaction.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/framework/components/light/component.js:1318

  • refreshProperties() reassigns layers (this.layers = this.layers), which with the current layers setter triggers a full remove/re-add across all layers, and then refreshProperties() calls onEnable() which adds the light to layers again. Since refreshProperties() is only used from the type setter, consider skipping layers in the refresh loop and handling layer re-add explicitly in the type setter (or otherwise ensure the type change only does one remove + one add) to avoid redundant layer bookkeeping.
    refreshProperties() {
        for (let i = 0; i < _properties.length; i++) {
            const name = _properties[i];

            /* eslint-disable no-self-assign */
            this[name] = this[name];
            /* eslint-enable no-self-assign */
        }
        if (this.enabled && this.entity.enabled) {
            this.onEnable();
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@willeastcott willeastcott merged commit e2957a3 into main May 3, 2026
12 checks passed
@willeastcott willeastcott deleted the refactor/light-component-camera-style branch May 3, 2026 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: graphics Graphics related issue enhancement Request for a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants