From 599752f34fe1743cec10316eeb4f48d8679ebe9a Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Sun, 13 Sep 2026 18:30:41 +0800 Subject: [PATCH] Light3d: take a Vector3d for direction and position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were `[number, number, number]` read by index, so a `Vector3d` — the obvious thing to reach for when an option is named `direction` in a 3D engine — set all three components to `undefined`, `normalize()` kept them `NaN`, and the light contributed nothing. Silently: no warning, and the scene still renders with whatever else is lit. `color` on the same options object already accepts a `Color`, a CSS string or an array, so this makes the object consistent with itself rather than adding a new idea. The value is READ, not retained — the light keeps its own normalized vector, and moving yours afterwards does not steer it. Additive: widening the union leaves every array call compiling and on the identical path. The only behaviour that changes is passing a `Vector3d`, which produced no light at all. Three shipped examples got this wrong, which is why it is worth the branch. Closes #1661 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NGvtaUNATVCVxD2qcbiY4t --- packages/melonjs/CHANGELOG.md | 2 + packages/melonjs/skills/melonjs-3d/SKILL.md | 32 +++++------ packages/melonjs/src/lighting/light3d.ts | 62 +++++++++++++++------ packages/melonjs/tests/lighting3d.spec.js | 46 +++++++++++++++ 4 files changed, 109 insertions(+), 33 deletions(-) diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index f4e942c71..9448923f5 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -9,6 +9,8 @@ - `save`: registered keys are reachable from TypeScript without a cast. The namespace was typed `Record`, and that index signature swallowed its own members, so under `strict` `save.add()` was `unknown` and could not be called at all. `add()` now returns the namespace typed with the keys just registered, and chained calls accumulate - `audio.tone()` and `audio.noise()` take a `delay` in seconds, scheduled on the audio clock, so a multi-part sound — a stinger's second note, an explosion's double-tap — sequences without a `setTimeout`. A timer fires on the main thread and a busy frame slips the note; this is scheduled up front and is sample-accurate. Omitting it is unchanged +- `Light3d`: `direction` and `position` accept a `Vector3d` as well as an `[x, y, z]` array, so a value the game already holds goes straight in — `color` already took a `Color`, a string or an array. The vector is read, not retained. Passing one used to be read by index and silently produce a `NaN` direction, so the light contributed nothing ([#1661](https://github.com/melonjs/melonJS/issues/1661)) + ### Fixed - Renderable: opacity now cascades — `alpha` multiplies with the alpha already on the renderer instead of replacing it, so a child at 0.5 inside a parent at 0.5 draws at 0.25. **A nested renderable that was visibly opaque under a faded ancestor will now fade with it** - GLTFModel: a loaded model reported no bounds at all, so a model with a `Body` was filed in the broadphase away from where it stood and collided with nothing. Its extent now comes from the glTF bounding box, measured once at load diff --git a/packages/melonjs/skills/melonjs-3d/SKILL.md b/packages/melonjs/skills/melonjs-3d/SKILL.md index 5e8afc16b..6e4b5cbb7 100644 --- a/packages/melonjs/skills/melonjs-3d/SKILL.md +++ b/packages/melonjs/skills/melonjs-3d/SKILL.md @@ -478,19 +478,24 @@ default), `"ambient"`, `"point"` and `"spot"`. ```js world.addChild(new Light3d({ type: "directional", direction: [0.3, 1, 0.2] })); world.addChild(new Light3d({ type: "ambient", intensity: 0.3 })); +// a Vector3d works too, wherever the game already has one +world.addChild(new Light3d({ type: "spot", position: torch.pos, range: 400 })); ``` -Both of that call's traps fail **silently**, and they compound: - -- `new Light3d(0, 0, {…})` — JavaScript drops the extra arguments, so `options` - becomes the number `0` and every setting in your literal is discarded. The - light still appears, on pure defaults: a `type: "ambient"` written this way is - a second DIRECTIONAL light, and the scene looks plausible enough that nobody - checks. -- `direction: new Vector3d(x, y, z)` — `direction` and `position` are `[x, y, z]` - **arrays**, read by index. A `Vector3d` has no `[0]`, so the light's direction - becomes `NaN` and it contributes nothing. (`color` is the odd one out: it does - take a `Color`, a CSS string or an `[r, g, b]` array.) +`direction` and `position` take an `[x, y, z]` array **or** a `Vector3d`, and +`color` takes a `Color`, a CSS string or an `[r, g, b]` array — so a value the +game already holds can go straight in. Each is read, not retained: move your +vector afterwards and the light stays where it was. + +Two things still bite, both silently: + +- `new Light3d(0, 0, {…})` — the constructor takes **options alone**. JavaScript + drops the extra arguments, so `options` becomes the number `0` and every + setting in your literal is discarded. The light still appears, on pure + defaults: a `type: "ambient"` written this way is a second DIRECTIONAL light, + and the scene looks plausible enough that nobody checks. TypeScript stops + checking a literal once the argument count is wrong, so nothing inside it is + verified either. - **`direction` is where the light GOES, and this is a Y-down space** — so a sun overhead travels *downward* and its Y is **positive**. Get the sign wrong and the scene is lit from underneath: faces that should be in shade are bright, @@ -504,11 +509,6 @@ Both of that call's traps fail **silently**, and they compound: `position` follows the same convention: a lamp above the floor has a **smaller** y than the floor. -They hide behind each other: TypeScript stops checking an object literal once -the argument count is already wrong, so fixing the call reveals the `Vector3d`, -and fixing that finally lets the sign show. A scene can go from "looks fine" to -"entirely black" to "lit from below" across three apparently-correct edits. - Use **both halves**: with a key light but no ambient, the shadow side of a mesh goes black. With no `Light3d` in the world at all, a `lit: true` mesh falls back to a white ambient and renders fullbright — indistinguishable from unlit, which diff --git a/packages/melonjs/src/lighting/light3d.ts b/packages/melonjs/src/lighting/light3d.ts index 96e484bad..ef83e0839 100644 --- a/packages/melonjs/src/lighting/light3d.ts +++ b/packages/melonjs/src/lighting/light3d.ts @@ -24,15 +24,19 @@ export interface Light3dOptions { * late-afternoon sun. A negative Y lights everything from underneath, * which reads instantly as wrong and is the usual mistake here. * - * An `[x, y, z]` array, read by index — a `Vector3d` has no `[0]`, so - * passing one yields `NaN` and the light contributes nothing at all. + * Either an `[x, y, z]` array or a {@link Vector3d} — as + * {@link Light3dOptions.color} already takes several forms. The vector is + * READ, not retained: {@link Light3d#direction} remains the engine's own, + * and normalized, so mutating what you passed in afterwards changes + * nothing. */ - direction?: [number, number, number]; + direction?: [number, number, number] | Vector3d; /** - * World-space position (point and spot lights), as an `[x, y, z]` array. - * Y-down again: a lamp above the floor has a **smaller** y than the floor. + * World-space position (point and spot lights), as an `[x, y, z]` array or + * a {@link Vector3d}. Y-down again: a lamp above the floor has a + * **smaller** y than the floor. */ - position?: [number, number, number]; + position?: [number, number, number] | Vector3d; /** * light color — a {@link Color}, a CSS color string, or an `[r, g, b]` * array with components in `0..1` (the glTF convention). Defaults to white. @@ -98,9 +102,41 @@ export interface Light3dOptions { * range: 800, innerConeAngle: 0.2, outerConeAngle: 0.45, * })); * - * // animate the sun in-game (direction is the way light travels) + * // `direction` and `position` also take a Vector3d, so a value the game + * // already keeps can be handed over without unpacking it. It is READ, not + * // retained — move the vector afterwards and the light does not follow. + * app.world.addChild(new Light3d({ + * type: "spot", + * position: torch.pos, + * direction: new Vector3d(0, 1, 0.4), + * })); + * + * // animate the sun in-game (direction is the way light travels). Y-down, so + * // a positive Y is a sun overhead; a negative one lights from underneath. * sun.direction.set(Math.sin(t), 1, Math.cos(t)).normalize(); */ +/** + * Read an `[x, y, z]` array or a {@link Vector3d} into `out`. + * + * Both forms are accepted for the same reason `color` accepts a {@link Color}, + * a CSS string or an array: a `Vector3d` is the obvious thing to reach for when + * an option is named `direction`, and index-reading one silently produces + * `NaN`. The value is copied, never retained. + * @param out - the vector the light owns + * @param value - what the caller passed + * @returns `out` + * @ignore + * @internal + */ +function readVector( + out: Vector3d, + value: [number, number, number] | Vector3d, +): Vector3d { + return Array.isArray(value) + ? out.set(value[0], value[1], value[2]) + : out.set(value.x, value.y, value.z); +} + export class Light3d extends Renderable { /** `"directional"`, `"ambient"`, `"point"` or `"spot"`. */ override type: "directional" | "ambient" | "point" | "spot"; @@ -136,21 +172,13 @@ export class Light3d extends Renderable { this.direction = new Vector3d(0, 1, 0); if (options.direction) { - this.direction.set( - options.direction[0], - options.direction[1], - options.direction[2], - ); + readVector(this.direction, options.direction); } this.direction.normalize(); this.position = new Vector3d(0, 0, 0); if (options.position) { - this.position.set( - options.position[0], - options.position[1], - options.position[2], - ); + readVector(this.position, options.position); } if (options.color instanceof Color) { diff --git a/packages/melonjs/tests/lighting3d.spec.js b/packages/melonjs/tests/lighting3d.spec.js index afb23ff70..58531ac1d 100644 --- a/packages/melonjs/tests/lighting3d.spec.js +++ b/packages/melonjs/tests/lighting3d.spec.js @@ -6,6 +6,7 @@ import { Light3d, Stage, state, + Vector3d, video, } from "../src/index.js"; import Renderable from "../src/renderable/renderable.js"; @@ -32,6 +33,51 @@ describe("Light3d", () => { expect([l.direction.x, l.direction.y, l.direction.z]).toEqual([0, 1, 0]); }); + it("takes a Vector3d for direction, the same as an array", () => { + const fromArray = new Light3d({ direction: [-0.35, 0.8, 0.45] }); + const fromVector = new Light3d({ + direction: new Vector3d(-0.35, 0.8, 0.45), + }); + + // identical, not merely close: both go through the same normalize + expect(fromVector.direction.x).toBe(fromArray.direction.x); + expect(fromVector.direction.y).toBe(fromArray.direction.y); + expect(fromVector.direction.z).toBe(fromArray.direction.z); + // and it is a real direction rather than the NaN an index-read produced + expect(Number.isFinite(fromVector.direction.x)).toBe(true); + expect(fromVector.direction.length()).toBeCloseTo(1, 5); + }); + + it("takes a Vector3d for position, the same as an array", () => { + const fromArray = new Light3d({ type: "point", position: [120, -40, 60] }); + const fromVector = new Light3d({ + type: "point", + position: new Vector3d(120, -40, 60), + }); + expect([ + fromVector.position.x, + fromVector.position.y, + fromVector.position.z, + ]).toEqual([ + fromArray.position.x, + fromArray.position.y, + fromArray.position.z, + ]); + }); + + it("reads the vector rather than retaining it", () => { + // a caller's vector is theirs — moving it afterwards must not steer the + // light, and the light must not be able to write back into it + const mine = new Vector3d(0, 1, 0); + const l = new Light3d({ type: "spot", position: mine, direction: mine }); + mine.set(99, 99, 99); + + expect(l.position.x).toBe(0); + expect(l.direction.y).toBeCloseTo(1, 5); + expect(l.position).not.toBe(mine); + expect(l.direction).not.toBe(mine); + }); + it("normalizes the direction on construction", () => { const l = new Light3d({ direction: [0, 5, 0] }); expect(l.direction.length()).toBeCloseTo(1, 5);