From 427edb5e6f09835bfa507da66c410bf02a294348 Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Sat, 3 Oct 2026 15:41:09 +0800 Subject: [PATCH] Color: setLinear, and the glTF sRGB bridge moves onto it glTF defines baseColorFactor and emissiveFactor as linear (spec 3.9.2), while a melonJS tint is sRGB, so the loader carried a private module to encode between them. That conversion is a Color concern rather than a glTF one: Color#setLinear is the public way in, the sibling of setFloat, which takes the same 0..1 range but treats it as already sRGB. The two are not interchangeable, since a linear 0.42 is sRGB 0.68. src/level/gltf/srgb.js is deleted and both call sites use setLinear. The old helper rounded to an 8-bit integer before Color stored it as a float; setLinear writes straight into normalizedRGBA, so nothing is rounded through 8 bits on the way in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NGvtaUNATVCVxD2qcbiY4t --- packages/melonjs/CHANGELOG.md | 1 + .../melonjs/skills/melonjs-3d-assets/SKILL.md | 49 +++++++++++ packages/melonjs/src/level/gltf/GLTFModel.js | 11 +-- packages/melonjs/src/level/gltf/GLTFScene.js | 14 ++- packages/melonjs/src/level/gltf/srgb.js | 26 ------ packages/melonjs/src/math/color.ts | 55 ++++++++++++ packages/melonjs/tests/color.spec.ts | 87 +++++++++++++++++++ packages/melonjs/tests/gltf-srgb.spec.js | 63 +++----------- 8 files changed, 214 insertions(+), 92 deletions(-) delete mode 100644 packages/melonjs/src/level/gltf/srgb.js diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index 6df6990c4..c0a62fbaa 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -5,6 +5,7 @@ ### Added - UI: `onOver` can return `false` to consume the pointer, the way `onClick` and `onRelease` already could. Returning anything else propagates, so nothing existing moves. Without it a UI element had no way to be opaque to hover: it could swallow a click on whatever it covered but not stop that thing lighting up underneath it. `onOver` only runs on the frame the pointer crosses in, so an element that has to stay opaque for as long as the pointer rests on it wants a `pointermove` callback registered through `input.registerPointerEvent` returning `false` as well. `onOut` stays `void` on purpose, since suppressing a leave would strand the element lit - `ProgressBar` is a renderable: a track, a fill sized by a value, an optional border and an optional label, for a health bar, a shield gauge, a cooldown or a loading bar. Set `value` and it redraws. `min` and `max` default to `0` and `1`, so a fraction works without stating either, and `ratio` reads back the normalized form. Four fill directions, horizontal or vertical, each growing from its own edge. Drawn with primitives and no artwork, so `trackColor`, `fillColor` and `borderColor` each take a colour, a css string or a `Gradient`; a `null` track leaves the bar hollow and a `radius` rounds it. The colour is re-read every frame, which is what makes a value-driven one a matter of mutating the `Color` you passed rather than anything the bar has to know about. `bindEvent` names an event to take the value from, and the subscription then lives exactly as long as the bar does, which a listener held anywhere else does not. The engine's own loading screen is built on it +- `Color#setLinear(r, g, b, alpha)` sets a colour from LINEAR values, encoding them to sRGB. The sibling of `setFloat`, which takes the same `0..1` range and treats it as already sRGB: the two are not interchangeable, since a linear `0.42` is sRGB `0.68`. Reach for it whenever the numbers come from a renderer's own colour space rather than from a css string or an image, glTF's `baseColorFactor` and `emissiveFactor` being the common case. Handing those to `setFloat`, or scaling them by 255 into `setColor`, renders every untextured material markedly too dark, by 60 to 70 counts per channel in the midtones, and it is easy to leave in because the result still looks coherent - `Mesh#depthTest` decides whether geometry in front of a mesh hides it, `true` by default so nothing existing moves. Depth TEST, not depth write: the transparent pass already turns writing off for everything blended, and that is policy, while being occluded at all is an authoring choice. `false` is for a mesh standing in for a screen-space effect, an additive glow carrying a world position only so it can sort and move with what it belongs to. Left depth tested, a flat billboard is sliced along a hard straight line the moment any geometry is nearer at some pixel, worst around something round where the near surface bulges further toward the camera than any offset would clear. Honoured in the transparent pass on both GPU backends; the Canvas renderer has no depth buffer and ignores it - Physics: `Sphere` is a collision shape. A body can carry one, and the builtin 3D narrowphase resolves it against another `Sphere`, a `Box3d`, or any planar shape through its own XY silhouette. It has no orientation to get wrong, which is what a `Box3d` cannot say and what anything tumbling or laid out over a curved surface needs. It joins `Box3d` in the portable `BodyShape` union - Physics: `raycast3d` reports the surface of a sphere body, measured as that sphere rather than as a bounding sphere derived from the renderable's 2D bounds diff --git a/packages/melonjs/skills/melonjs-3d-assets/SKILL.md b/packages/melonjs/skills/melonjs-3d-assets/SKILL.md index 84222ff42..7e6d16707 100644 --- a/packages/melonjs/skills/melonjs-3d-assets/SKILL.md +++ b/packages/melonjs/skills/melonjs-3d-assets/SKILL.md @@ -129,6 +129,54 @@ asset fails to load. Export uncompressed. `"auto"`) drives sampling; glTF materials that declare their own sampler carry it through. Pixel-art models want `"nearest"`. +### `baseColorFactor` is LINEAR; a `tint` is sRGB + +The loader handles this for you. It only bites when you build meshes yourself +from `loader.getGLTF(name).nodes` — which is what you do for per-node control, +since there is no public per-node `Mesh` factory — and reach for the obvious +`* 255`: + +```js +// WRONG — every untextured material renders far too dark +mesh.tint.setColor(f[0] * 255, f[1] * 255, f[2] * 255, 1); +``` + +glTF defines `pbrMetallicRoughness.baseColorFactor` in **linear** space (spec +3.9.2), while a melonJS `tint` is 8-bit **sRGB** — the same space as a CSS +colour or a texel out of a PNG. They are not the same number, and the gap is +widest exactly where art tends to live: + +| linear | `* 255` | correct | +|---|---|---| +| 0.05 | 13 | 63 | +| 0.20 | 51 | 124 | +| 0.42 | 107 | 173 | +| 0.72 | 184 | 221 | +| 1.00 | 255 | 255 | + +`Color.setLinear` does the encoding for you — it is the linear-space sibling of +`setFloat`, which takes the same `0..1` range but treats it as already-sRGB: + +```js +mesh.tint.setLinear(...node.baseColorFactor); // encodes; what you want +mesh.tint.setFloat(...node.baseColorFactor); // does NOT encode; too dark +``` + +It clamps, so the slightly out-of-range factors some exporters emit cannot turn +into `NaN`, and it keeps the encoded value as a float rather than rounding +through 8 bits. + +Measured on a scene whose every material went through `* 255`: mean frame luma +85.5 against 147.8 once encoded, with each channel sitting 60 to 73 counts low. + +**Fix it early.** It is nearly invisible as a bug, because the scene still +looks coherent — just moody — so lighting and bloom thresholds get tuned +against the wrong values. Correcting the encoding afterwards blows the frame +out and forces a lighting retune, which is a much bigger job than the encode. + +This applies only to factors coming out of glTF. Colours **you** author for the +screen, a HUD tint or a glow, are already sRGB: `* 255` is right for those. + ## Lights Authored `KHR_lights_punctual` lights — sun, point, spot — become `Light3d` @@ -423,6 +471,7 @@ need a prefix. | one part of a rig renders rigid while its curves exist | it was re-parented mid-pose, baking the inverse into `matrix_parent_inverse` | | a merged mesh lights wrong along one axis | it inherited the active object's non-uniform scale on join — apply transforms | | an authored palette comes out uniformly dark, but white is correct | linear values written to a strip that is saved verbatim | +| every untextured glTF material renders too dark, white unaffected | `baseColorFactor` is linear and a `tint` is sRGB — `tint.setLinear(...)`, not `* 255` or `setFloat` | | `getGLTF(name).nodes[0]` geometry lands in the wrong place | `nodes` is per primitive, each with its own `world`; merge to one primitive and export at the origin | | a prop casts no visible shadow | wide and flat-bottomed — the blob is under it; `shadowGroundY` haloes it rather than revealing it | | shadows only show on casters near the camera | `shadowGroundY` is on the wrong side — Y-down means the floor is a **greater** y, so a `pos.y - lift` puts the blob inside the caster and the depth test leaves only a hairline ring | diff --git a/packages/melonjs/src/level/gltf/GLTFModel.js b/packages/melonjs/src/level/gltf/GLTFModel.js index 69b1258a5..8391f92c8 100644 --- a/packages/melonjs/src/level/gltf/GLTFModel.js +++ b/packages/melonjs/src/level/gltf/GLTFModel.js @@ -11,7 +11,6 @@ import InstancedMesh from "../../renderable/instanced_mesh.js"; import Mesh from "../../renderable/mesh.js"; import { fillInstances } from "./GLTFScene.js"; import { sampleChannel } from "./gltf_sampler.js"; -import { linearToSrgb8 } from "./srgb.js"; /** * additional import for TypeScript @@ -294,15 +293,11 @@ export default class GLTFModel extends Container { if (prim.instances) { fillInstances(mesh, prim.instances); } - // LINEAR per the glTF spec; a tint is 8-bit sRGB — see - // `linearToSrgb8` + // LINEAR per the glTF spec; a tint is sRGB. See + // {@link Color#setLinear}. const f = prim.baseColorFactor; if (f) { - mesh.tint.setColor( - linearToSrgb8(f[0]), - linearToSrgb8(f[1]), - linearToSrgb8(f[2]), - ); + mesh.tint.setLinear(f[0], f[1], f[2]); } if (prim.colors) { mesh.vertexColors = prim.colors; diff --git a/packages/melonjs/src/level/gltf/GLTFScene.js b/packages/melonjs/src/level/gltf/GLTFScene.js index f2b552adb..1498b0a54 100644 --- a/packages/melonjs/src/level/gltf/GLTFScene.js +++ b/packages/melonjs/src/level/gltf/GLTFScene.js @@ -6,7 +6,6 @@ import InstancedMesh from "../../renderable/instanced_mesh.js"; import Mesh from "../../renderable/mesh.js"; import { writeInstanceTRS } from "../../video/gpu/instancerecord.ts"; import GLTFModel from "./GLTFModel.js"; -import { linearToSrgb8 } from "./srgb.js"; /** * additional import for TypeScript @@ -262,16 +261,13 @@ export default class GLTFScene { // path renders opaque.) Composes with COLOR_0 and the texture: the // batcher does factor × vertexColor × texel, matching glTF. // - // The factor is LINEAR per the glTF spec and a tint is 8-bit sRGB, - // so it has to be encoded rather than scaled by 255 — see - // `linearToSrgb8`. + // The factor is LINEAR per the glTF spec and a tint is sRGB, so it + // has to be encoded rather than scaled by 255: that is exactly + // what `setLinear` is for, and it keeps the encoded value as a + // float instead of rounding it through 8 bits on the way in. const f = node.baseColorFactor; if (f) { - mesh.tint.setColor( - linearToSrgb8(f[0]), - linearToSrgb8(f[1]), - linearToSrgb8(f[2]), - ); + mesh.tint.setLinear(f[0], f[1], f[2]); } // per-vertex colors (COLOR_0) — multiplied by the tint per vertex if (node.colors) { diff --git a/packages/melonjs/src/level/gltf/srgb.js b/packages/melonjs/src/level/gltf/srgb.js deleted file mode 100644 index 31bcf97c8..000000000 --- a/packages/melonjs/src/level/gltf/srgb.js +++ /dev/null @@ -1,26 +0,0 @@ -/** - * Color-space bridge for glTF material factors. - * - * glTF 2.0 defines `pbrMetallicRoughness.baseColorFactor` in **linear** space - * (spec §3.9.2), while a melonJS `tint` is an 8-bit **sRGB** value — the same - * space as a CSS color or a texel out of a PNG. Handing the linear number - * straight to `tint.setColor(f * 255)` therefore displays every untextured - * glTF material far too light and desaturated: a linear `0.29` shows up as - * sRGB `0.58`, so an authored mid-green renders as pale mint. - * - * The encode below is the standard sRGB transfer function. - * @module level/gltf/srgb - */ - -/** - * Encode one linear channel (0..1) to an 8-bit sRGB value (0..255). - * @param {number} c - linear channel value - * @returns {number} the sRGB-encoded channel, rounded to 0..255 - */ -export function linearToSrgb8(c) { - // guard the domain: exporters can emit slightly out-of-range factors, and - // `Math.pow` on a negative base returns NaN, which would poison the tint - const v = c <= 0 ? 0 : c >= 1 ? 1 : c; - const s = v <= 0.0031308 ? v * 12.92 : 1.055 * v ** (1 / 2.4) - 0.055; - return Math.round(s * 255); -} diff --git a/packages/melonjs/src/math/color.ts b/packages/melonjs/src/math/color.ts index c4ea3a936..d1e7ede91 100644 --- a/packages/melonjs/src/math/color.ts +++ b/packages/melonjs/src/math/color.ts @@ -209,6 +209,26 @@ for (const [name, rgb] of CSS_COLORS) { * A color manipulation object. * @category Math */ +/** + * Encode one LINEAR colour channel to sRGB. + * + * The standard sRGB transfer function. Kept at `0..1` rather than `0..255` + * because that is what {@link Color} stores internally, so nothing is rounded + * through 8 bits on the way in. + * `@internal`, so it is stripped from the published declarations; + * {@link Color#setLinear} is the public way in. + * @param c - linear channel value, clamped to `0..1` + * @returns the sRGB-encoded channel, `0..1` + * @internal + */ +export function linearToSrgb(c: number) { + // clamp first: exporters emit slightly out-of-range factors, and a + // negative base under a fractional power is NaN, which would poison the + // whole colour rather than just one channel + const v = c <= 0 ? 0 : c >= 1 ? 1 : c; + return v <= 0.0031308 ? v * 12.92 : 1.055 * v ** (1 / 2.4) - 0.055; +} + export class Color { private normalizedRGBA: Float32Array; @@ -331,6 +351,41 @@ export class Color { return this; } + /** + * Sets the color from LINEAR values, encoding them to sRGB. + * + * The sibling of {@link Color#setFloat}, which takes the same `0..1` range + * but treats it as already-sRGB. Which one you want depends entirely on + * where the numbers came from, and the two are NOT interchangeable: a + * linear `0.42` is sRGB `0.68`, not `0.42`. + * + * Reach for this whenever a value arrives from a renderer's own colour + * space rather than from a CSS string or an image. The common case is + * glTF, which defines `baseColorFactor` and `emissiveFactor` as linear + * (spec 3.9.2) — handing those straight to `setFloat` or scaling them by + * 255 into `setColor` renders every untextured material markedly too + * DARK, by 60 to 70 counts per channel in the midtones. It is an easy + * mistake to leave in, because the result still looks coherent, just + * moody, so lighting gets tuned against the wrong values. + * @param r - The red component [0.0 .. 1.0], linear. + * @param g - The green component [0.0 .. 1.0], linear. + * @param b - The blue component [0.0 .. 1.0], linear. + * @param [alpha=1.0] - The alpha value [0.0 .. 1.0]. Alpha is NOT a colour + * and carries no transfer function, so it is taken as-is. + * @returns Reference to this object for method chaining. + * @example + * // a glTF material factor, which the spec defines as linear + * mesh.tint.setLinear(...node.baseColorFactor); + */ + setLinear(r: number, g: number, b: number, alpha = 1.0) { + const a = this.normalizedRGBA; + a[0] = linearToSrgb(r); + a[1] = linearToSrgb(g); + a[2] = linearToSrgb(b); + a[3] = clamp(alpha, 0, 1.0); + return this; + } + /** * Sets the color to the specified HSV values. * @param h - The hue [0 .. 1]. diff --git a/packages/melonjs/tests/color.spec.ts b/packages/melonjs/tests/color.spec.ts index 475d96875..1a422ef0e 100644 --- a/packages/melonjs/tests/color.spec.ts +++ b/packages/melonjs/tests/color.spec.ts @@ -424,6 +424,93 @@ describe("Color", () => { }); }); + describe("setLinear", () => { + it("encodes linear to sRGB rather than copying the number", () => { + // the whole point: linear 0.42 is sRGB 173, NOT 107 (0.42 * 255) + const c = new Color().setLinear(0.42, 0.42, 0.42); + expect(c.r).toBe(173); + expect(c.g).toBe(173); + expect(c.b).toBe(173); + }); + + it("differs from setFloat, which treats the same range as sRGB", () => { + const linear = new Color().setLinear(0.42, 0.37, 0.33); + const asIs = new Color().setFloat(0.42, 0.37, 0.33); + expect(linear.r).not.toBe(asIs.r); + // the byte getters TRUNCATE (`~~(v * 255)`), so these are the + // encoded values floored, not rounded + expect([linear.r, linear.g, linear.b]).toEqual([173, 163, 155]); + expect([asIs.r, asIs.g, asIs.b]).toEqual([107, 94, 84]); + }); + + it("keeps full float precision rather than going through 8 bits", () => { + // what the renderer actually samples is the normalized array, and + // storing the encoded float there is why this is not just + // an encode that rounds to 8 bits on the way in + const c = new Color().setLinear(0.37, 0.37, 0.37); + const expected = 1.055 * 0.37 ** (1 / 2.4) - 0.055; + expect(c.toArray()[0]).toBeCloseTo(expected, 6); + }); + + it("matches the sRGB transfer function to within the byte truncation", () => { + // the reference encode, rounded to 8 bits. This class keeps the + // float and its byte getters truncate, so the two can sit one + // count apart in that view and no more. + const reference = (c: number) => { + const v = c <= 0 ? 0 : c >= 1 ? 1 : c; + const sr = v <= 0.0031308 ? v * 12.92 : 1.055 * v ** (1 / 2.4) - 0.055; + return Math.round(sr * 255); + }; + for (const v of [0, 0.002, 0.1, 0.29, 0.42, 0.73, 1]) { + expect( + Math.abs(new Color().setLinear(v, v, v).r - reference(v)), + ).toBeLessThanOrEqual(1); + } + }); + + it("keeps the endpoints exact", () => { + const black = new Color().setLinear(0, 0, 0); + expect([black.r, black.g, black.b]).toEqual([0, 0, 0]); + const white = new Color().setLinear(1, 1, 1); + expect([white.r, white.g, white.b]).toEqual([255, 255, 255]); + }); + + it("uses the linear segment near black", () => { + // below 0.0031308 the curve is the straight 12.92x, not the power + const c = new Color().setLinear(0.002, 0.002, 0.002); + expect(c.toArray()[0]).toBeCloseTo(0.002 * 12.92, 6); + }); + + it("clamps out-of-range factors instead of producing NaN", () => { + // exporters do emit these, and a negative base under a fractional + // power is NaN, which would poison the whole colour + const c = new Color().setLinear(-0.5, 2, Number.NaN); + expect(c.r).toBe(0); + expect(c.g).toBe(255); + expect(Number.isNaN(c.b)).toBe(false); + }); + + it("passes alpha through untouched — it carries no transfer function", () => { + const c = new Color().setLinear(0.5, 0.5, 0.5, 0.25); + expect(c.alpha).toBeCloseTo(0.25, 5); + }); + + it("is monotonic across the range", () => { + // ported here when `level/gltf/srgb.js` was folded into this class + let prev = -1; + for (let i = 0; i <= 20; i++) { + const c = new Color().setLinear(i / 20, i / 20, i / 20); + expect(c.r).toBeGreaterThanOrEqual(prev); + prev = c.r; + } + }); + + it("returns itself for chaining, like its siblings", () => { + const c = new Color(); + expect(c.setLinear(0.5, 0.5, 0.5)).toBe(c); + }); + }); + describe("toHex8", () => { it("converts full alpha", () => { expect(new Color(255, 0, 0, 1).toHex8()).toEqual("#FF0000FF"); diff --git a/packages/melonjs/tests/gltf-srgb.spec.js b/packages/melonjs/tests/gltf-srgb.spec.js index 93b6b1d94..e8d2f5e5e 100644 --- a/packages/melonjs/tests/gltf-srgb.spec.js +++ b/packages/melonjs/tests/gltf-srgb.spec.js @@ -8,54 +8,13 @@ */ import { beforeAll, describe, expect, it } from "vitest"; import { Application, boot, GLTFModel, video } from "../src/index.js"; -import { linearToSrgb8 } from "../src/level/gltf/srgb.js"; - -describe("glTF linear → sRGB tint encode", () => { - it("maps the endpoints exactly", () => { - expect(linearToSrgb8(0)).toEqual(0); - expect(linearToSrgb8(1)).toEqual(255); - }); - - it("lightens mid-tones the way the sRGB transfer function does", () => { - // linear 0.5 is sRGB ~0.7354 → 188. The old `f * 255` gave 128. - expect(linearToSrgb8(0.5)).toEqual(188); - expect(linearToSrgb8(0.5)).not.toEqual(Math.round(0.5 * 255)); - }); - - it("round-trips the value the road material is authored at", () => { - // sRGB 0.29 stored as linear 0.0684 must come back out as ~0.29 - expect(linearToSrgb8(0.0684)).toBeCloseTo(0.29 * 255, -0.5); - expect(linearToSrgb8(0.3931)).toBeCloseTo(0.66 * 255, -0.5); - }); - - it("uses the linear segment near black", () => { - // below the 0.0031308 knee the curve is a plain 12.92x ramp - expect(linearToSrgb8(0.002)).toEqual(Math.round(0.002 * 12.92 * 255)); - }); - - it("clamps out-of-range factors instead of returning NaN", () => { - // `Math.pow` on a negative base is NaN, which would poison the tint; - // exporters do occasionally emit slightly out-of-range factors - expect(linearToSrgb8(-0.2)).toEqual(0); - expect(linearToSrgb8(1.4)).toEqual(255); - expect(Number.isNaN(linearToSrgb8(-0.2))).toBe(false); - }); - - it("is monotonic across the range", () => { - let prev = -1; - for (let i = 0; i <= 20; i++) { - const v = linearToSrgb8(i / 20); - expect(v).toBeGreaterThanOrEqual(prev); - prev = v; - } - }); -}); /** - * The tests above only exercise the helper — they would ALL still pass if the - * loader stopped calling it. These pin the wiring: a material factor has to - * arrive on the mesh tint sRGB-encoded, which is the thing that was actually - * broken. + * The WIRING: a material factor has to arrive on the mesh tint sRGB-encoded. + * + * The transfer function itself belongs to {@link Color#setLinear} now and is + * tested in `color.spec.ts`; those tests would all still pass if the loader + * stopped calling it, which is why this file exists separately. */ describe("glTF loader applies the encode to the mesh tint", () => { beforeAll(async () => { @@ -111,9 +70,15 @@ describe("glTF loader applies the encode to the mesh tint", () => { }; it("encodes a mid-tone factor rather than scaling it by 255", () => { - // linear 0.5 → sRGB 188, NOT 128. This is the assertion that fails if - // the loader goes back to `Math.round(f * 255)`. - expect(tintOf([0.5, 0.5, 0.5, 1])).toEqual([188, 188, 188]); + // linear 0.5 is sRGB 0.7354, NOT 0.5. This is the assertion that fails + // if the loader goes back to `Math.round(f * 255)`, which would give + // 128. + // + // 187 and not 188: `setLinear` keeps the encoded value as a float and + // the byte getters truncate, where the old 8-bit helper rounded on the + // way in. What reaches the renderer is the float, so this is the more + // accurate of the two by half a count. + expect(tintOf([0.5, 0.5, 0.5, 1])).toEqual([187, 187, 187]); }); it("carries the road material's authored green through intact", () => {