Color: setLinear, and the glTF sRGB bridge moves onto it - #1708
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
NaN inputs still poison renderer-facing color data, and some public documentation incorrectly recommends conversion for emissive factors.
Review effort: Balanced
Findings: 2
Open (5)
What changed in this PR
Moves linear-to-sRGB conversion into the public Color#setLinear API and updates glTF tint handling.
Changes:
- Adds
Color#setLinearwith focused tests. - Migrates glTF loaders and removes the private helper.
- Documents correct
baseColorFactorhandling.
| File | Description |
|---|---|
packages/melonjs/src/math/color.ts |
Adds linear-to-sRGB conversion API. |
packages/melonjs/src/level/gltf/GLTFScene.js |
Uses setLinear for scene tints. |
packages/melonjs/src/level/gltf/GLTFModel.js |
Uses setLinear for model tints. |
packages/melonjs/src/level/gltf/srgb.js |
Removes obsolete helper. |
packages/melonjs/tests/color.spec.ts |
Tests the new API. |
packages/melonjs/tests/gltf-srgb.spec.js |
Updates loader integration coverage. |
packages/melonjs/skills/melonjs-3d-assets/SKILL.md |
Documents glTF color-space handling. |
packages/melonjs/CHANGELOG.md |
Announces the API. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| linearToSrgb8(f[1]), | ||
| linearToSrgb8(f[2]), | ||
| ); | ||
| mesh.tint.setLinear(f[0], f[1], f[2]); |
| 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); |
| ## [20.8.0] (melonJS 2) - _unreleased_ | ||
|
|
||
| ### Added | ||
| - `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 |
| * A color manipulation object. | ||
| * @category Math | ||
| */ | ||
| /** |
Comment on lines
+362
to
+369
| * 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. |
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) <[email protected]> Claude-Session: https://claude.ai/code/session_01NGvtaUNATVCVxD2qcbiY4t
obiot
force-pushed
the
refactor/color-setlinear
branch
from
October 3, 2026 07:53
e239df8 to
427edb5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
src/level/gltf/srgb.jswas a private 26-line module holding one function,used only by the glTF loader. The conversion it does is a
Colorconcern, nota glTF one, so it moves onto
Colorand the module goes.Why the conversion exists
glTF defines
baseColorFactorandemissiveFactoras linear (spec§3.9.2), while a melonJS
tintis sRGB — the same space as a css colour or aPNG texel. Handing the linear number straight through renders every untextured
material markedly too dark, by 60 to 70 counts per channel in the midtones.
It is easy to leave in, because the result still looks coherent, just moody,
so lighting gets tuned against the wrong values.
The API
Color#setLinear(r, g, b, alpha)is the sibling of the existingsetFloat,which takes the same
0..1range but treats it as already sRGB. The two arenot interchangeable: a linear
0.42is sRGB0.68, not0.42. Alphacarries no transfer function and is taken as-is.
linearToSrgbis exported@internal, so it is stripped from the publisheddeclarations and
setLinearis the only public way in.One behavioural difference
The old helper rounded to an 8-bit integer (
Math.round(s * 255)) beforeColorstored the value as a float.setLinearwrites straight intonormalizedRGBA, so nothing is rounded through 8 bits on the way in. Thischanges existing glTF material colours very slightly — worth a look before
merge, since it is the only part of this that moves pixels.
Both the clamp and the transfer function are carried over unchanged: out-of-
range factors are clamped first, because a negative base under a fractional
power is
NaNand would poison the whole colour rather than one channel.Tests
tests/color.spec.tscovers the transfer function at its breakpoints, thelinear-versus-
setFloatdistinction, and clamping.tests/gltf-srgb.spec.jsno longer reaches into a private module and tests the loader through the
public API instead.
Full suite green, eslint 0 errors, biome clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NGvtaUNATVCVxD2qcbiY4t