Make player colours distinct under colour vision deficiency - #4932
Make player colours distinct under colour vision deficiency#4932thedavidyoungblood wants to merge 10 commits into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThe PR adds observer-aware color simulation, CIEDE2000 validation, deterministic LCH palette generation, shared registry-backed allocation, and configurable theme settings. Human, nation, and bot allocators coordinate color assignments across large lobbies. ChangesColor allocation system
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsTheme
participant ColorAllocator
participant ColorRegistry
participant ColorVision
SettingsTheme->>ColorVision: parse configured observers
SettingsTheme->>ColorRegistry: create shared registry
SettingsTheme->>ColorAllocator: create human, nation, and bot allocators
ColorAllocator->>ColorRegistry: register and score candidates
ColorRegistry->>ColorVision: evaluate observer views
ColorRegistry-->>ColorAllocator: distinctness scores
ColorAllocator->>ColorRegistry: commit or reserve selected colors
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
src/client/theme/ColorAllocator.ts (2)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCombine the two
extendcalls.
extendaccepts a list. One call reads better and matches howsrc/client/theme/ColorGenerator.tsdoes it.♻️ Proposed change
-extend([lchPlugin]); -extend([labPlugin]); +extend([lchPlugin, labPlugin]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/theme/ColorAllocator.ts` around lines 23 - 24, Combine the consecutive extend calls for lchPlugin and labPlugin into a single extend invocation, passing both plugins in one list, matching the pattern used by ColorGenerator.
237-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
selectDistinctColorIndexhas no production caller and is only referenced bytests/Colors.test.ts.ColorAllocatorusesbestUnusedinstead. Remove the function and its test to avoid keeping a weaker duplicate selection rule around.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/theme/ColorAllocator.ts` around lines 237 - 261, Remove the unused selectDistinctColorIndex function from ColorAllocator.ts and delete its corresponding references and test cases in Colors.test.ts. Keep ColorAllocator's existing bestUnused selection logic unchanged.src/client/theme/ColorRegistry.ts (3)
128-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the pool-refresh loop shared by
commitandreserve.Lines 134-144 repeat lines 86-96 exactly. Both loops walk every pool, skip used candidates, and lower
nearest. One private helper removes the copy and keeps the two entry points honest if the scoring rule changes later.♻️ Proposed helper
+ /** Lower every tracked pool's score against one colour now in play. */ + private refreshPools(against: Candidate): void { + for (const pool of this.pools) { + for (const other of pool) { + if (other.used) { + continue; + } + const value = distance(other.labs, against.labs); + if (value < other.nearest) { + other.nearest = value; + } + } + } + } + /** Put a colour into play and refresh every tracked pool against it. */ commit(candidate: Candidate): void { candidate.used = true; this.inPlay.push(candidate); - for (const pool of this.pools) { - for (const other of pool) { - if (other.used) { - continue; - } - const value = distance(other.labs, candidate.labs); - if (value < other.nearest) { - other.nearest = value; - } - } - } + this.refreshPools(candidate); } @@ reserve(colors: Colord[]): void { for (const color of colors) { const reserved = this.candidate(color); this.inPlay.push(reserved); // Refresh pools here too, so reserving works whatever order the // allocators happen to be constructed in. - for (const pool of this.pools) { - for (const other of pool) { - if (other.used) { - continue; - } - const value = distance(other.labs, reserved.labs); - if (value < other.nearest) { - other.nearest = value; - } - } - } + this.refreshPools(reserved); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/theme/ColorRegistry.ts` around lines 128 - 146, Extract the duplicated pool-refresh logic from reserve and commit into a shared private helper on ColorRegistry. Have both entry points invoke that helper with the newly selected or reserved candidate, preserving the existing used-candidate skip and nearest-distance update behavior.
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the equal-length requirement on
distance.The loop iterates
first.lengthand indexessecond[i]. If the two arrays came from registries with different observer lists,second[i]isundefinedanddeltaE2000throws onundefined.a. Nothing in the current code mixes registries, so this is not a live defect.distanceis exported, though, so a one-line note on the invariant helps the next caller.Also worth a note:
worsthere means the smallest separation, which is the correct reading for a distinctness metric. A short comment saying so stops a future reader "fixing"<into>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/theme/ColorRegistry.ts` around lines 20 - 29, Document the `distance` function’s invariants: callers must provide equal-length `first` and `second` arrays, and `worst` intentionally tracks the smallest delta-E separation despite its name. Add concise comments without changing the existing loop or comparison behavior.
107-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
abortBelowparameter.Current callers pass a single
labsargument, so the lower-bound shortcut is never requested. RemoveabortBelow, the matching doc caveat, and keepdistanceToInPlayreturning the exact distance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/theme/ColorRegistry.ts` around lines 107 - 119, Remove the unused abortBelow parameter and its early-return shortcut from distanceToInPlay, along with the matching documentation caveat. Keep the method accepting only labs and iterating through all inPlay candidates so it returns the exact minimum distance.src/client/theme/ColorDistance.ts (1)
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the doc block onto the function, and drop the local aliases.
The block comment sits above
const DEG, so tooling attaches it toDEGinstead ofdeltaE2000. Readers hovering the exported function see nothing.The locals
radanddegadd a step with no benefit. Module-level constants are already resolved once.♻️ Proposed tidy-up
-/** - * CIEDE2000 colour difference between two LAB colours, on the usual 0–100 - * scale. - * - * ... (rest of the block) - */ const DEG = 180 / Math.PI; const RAD = Math.PI / 180; @@ +/** + * CIEDE2000 colour difference between two LAB colours, on the usual 0–100 + * scale. + * + * ... (move the existing text here) + */ export function deltaE2000(first: LabaColor, second: LabaColor): number { - const rad = RAD; - const deg = DEG; -Then use
RADandDEGdirectly in the body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/theme/ColorDistance.ts` around lines 3 - 31, Move the CIEDE2000 documentation block directly above the exported deltaE2000 function so tooling associates it with that API. Remove the local rad and deg aliases inside deltaE2000, and update the function body to use the module-level RAD and DEG constants directly.tests/Colors.test.ts (2)
366-448: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test that pins fallback-before-synthesis.
The suite proves two ends of the range well: 48 players stay on shipped colours (lines 407-418), and 125 players stay unique and separated (lines 396-401, 420-423). Nothing checks the step between them, where primary is below the floor but fallback still holds a good colour.
That gap is why the
??behaviour inselect()atsrc/client/theme/ColorAllocator.tsline 150 went unnoticed. A direct test would be small: give the allocator a tiny primary palette and a fallback palette holding one clearly distant colour, then assert the fallback colour is issued before any synthesized colour appears.💚 Sketch of the missing test
+ test("prefers a fallback colour over a synthesised one", () => { + // Primary is three near-identical reds, so every primary candidate falls + // below the floor after the first allocation. The fallback holds one + // clearly distant colour, which must be issued before anything synthesised. + const primary = [colord("`#ff0000`"), colord("`#fe0000`"), colord("`#fd0000`")]; + const fallback = [colord("`#0000ff`")]; + const allocator = new ColorAllocator(primary, fallback, { + observers: ["normal"], + distinctnessFloor: 20, + }); + const issued = Array.from({ length: 2 }, (_, i) => + allocator.assignColor(`player_${i}`).toHex(), + ); + expect(issued).toContain(colord("`#0000ff`").toHex()); + });Do you want me to open an issue to track this test, or draft the full test once the
select()behaviour is settled?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Colors.test.ts` around lines 366 - 448, Add a focused test in the “ColorAllocator distinctness guarantees” suite that uses a tiny primary palette and a fallback palette containing one clearly distant color, then assigns enough colors to make the primary candidate fail the distinctness floor. Assert the fallback color is selected before any synthesized color, covering the fallback-before-synthesis behavior of ColorAllocator.select.
545-578: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the SettingsTheme wiring tests for
colorblind.
defaultandcolorblindare the onlyPALETTE_NAMES, but theSettingsTheme allocator wiringtests only usedefault. Add the bot-palette and 125-human uniqueness checks forcolorblindas well, preferably by parameterising the theme name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Colors.test.ts` around lines 545 - 578, Parameterize the “SettingsTheme allocator wiring” tests over both PALETTE_NAMES, default and colorblind, so each theme runs the bot palette reuse and 125-human color uniqueness checks. Build each SettingsTheme and expected bot palette from the current parameterized theme name while preserving the existing assertions.src/client/theme/ColorVision.ts (1)
21-34: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a fixed-length row type for the CVD matrices.
The Machado coefficients and test hex values match the severity-1.0 matrices. Keep the values, but replace
readonly number[]with a 9-element tuple type so a missing or extra coefficient errors before runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/theme/ColorVision.ts` around lines 21 - 34, Update the CVD_MATRICES type to use a readonly 9-element tuple for each matrix instead of readonly number[], preserving all existing Machado coefficients and ensuring missing or extra values are rejected at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/theme/ColorAllocator.ts`:
- Around line 137-156: Update src/client/theme/ColorAllocator.ts lines 137-156
in select() to retain the bestUnused(this.primary) result from the
distinctness-floor check, evaluate both primary and fallback candidates, and
choose the roomiest curated candidate before comparing it with generate();
update tests/Colors.test.ts lines 366-448 to cover near-identical primary
colours, one distant fallback colour, and a high distinctnessFloor, asserting
the fallback is returned before any synthesized colour.
In `@src/client/theme/ColorGenerator.ts`:
- Around line 94-104: Update sequenceColor to clamp the generated chroma before
constructing the colord value, reducing it as needed until the resulting toLch()
remains within the envelope’s chroma bounds. Preserve the generated hue and
lightness, then continue downstream scoring with toLab() so clipped saturated
colors do not produce duplicate candidates or exceed the palette chroma ceiling.
---
Nitpick comments:
In `@src/client/theme/ColorAllocator.ts`:
- Around line 23-24: Combine the consecutive extend calls for lchPlugin and
labPlugin into a single extend invocation, passing both plugins in one list,
matching the pattern used by ColorGenerator.
- Around line 237-261: Remove the unused selectDistinctColorIndex function from
ColorAllocator.ts and delete its corresponding references and test cases in
Colors.test.ts. Keep ColorAllocator's existing bestUnused selection logic
unchanged.
In `@src/client/theme/ColorDistance.ts`:
- Around line 3-31: Move the CIEDE2000 documentation block directly above the
exported deltaE2000 function so tooling associates it with that API. Remove the
local rad and deg aliases inside deltaE2000, and update the function body to use
the module-level RAD and DEG constants directly.
In `@src/client/theme/ColorRegistry.ts`:
- Around line 128-146: Extract the duplicated pool-refresh logic from reserve
and commit into a shared private helper on ColorRegistry. Have both entry points
invoke that helper with the newly selected or reserved candidate, preserving the
existing used-candidate skip and nearest-distance update behavior.
- Around line 20-29: Document the `distance` function’s invariants: callers must
provide equal-length `first` and `second` arrays, and `worst` intentionally
tracks the smallest delta-E separation despite its name. Add concise comments
without changing the existing loop or comparison behavior.
- Around line 107-119: Remove the unused abortBelow parameter and its
early-return shortcut from distanceToInPlay, along with the matching
documentation caveat. Keep the method accepting only labs and iterating through
all inPlay candidates so it returns the exact minimum distance.
In `@src/client/theme/ColorVision.ts`:
- Around line 21-34: Update the CVD_MATRICES type to use a readonly 9-element
tuple for each matrix instead of readonly number[], preserving all existing
Machado coefficients and ensuring missing or extra values are rejected at
compile time.
In `@tests/Colors.test.ts`:
- Around line 366-448: Add a focused test in the “ColorAllocator distinctness
guarantees” suite that uses a tiny primary palette and a fallback palette
containing one clearly distant color, then assigns enough colors to make the
primary candidate fail the distinctness floor. Assert the fallback color is
selected before any synthesized color, covering the fallback-before-synthesis
behavior of ColorAllocator.select.
- Around line 545-578: Parameterize the “SettingsTheme allocator wiring” tests
over both PALETTE_NAMES, default and colorblind, so each theme runs the bot
palette reuse and 125-human color uniqueness checks. Build each SettingsTheme
and expected bot palette from the current parameterized theme name while
preserving the existing assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c4d78c2e-55b1-49f5-a4cf-cc72748aaae6
📒 Files selected for processing (10)
src/client/render/gl/RenderSettings.tssrc/client/render/gl/colorblind-theme.jsonsrc/client/render/gl/default-theme.jsonsrc/client/theme/ColorAllocator.tssrc/client/theme/ColorDistance.tssrc/client/theme/ColorGenerator.tssrc/client/theme/ColorRegistry.tssrc/client/theme/ColorVision.tssrc/client/theme/ThemeProvider.tstests/Colors.test.ts
| export function sequenceColor(index: number, envelope: ColorEnvelope): Colord { | ||
| const lightnessRange = envelope.lightnessMax - envelope.lightnessMin; | ||
| const chromaRange = envelope.chromaMax - envelope.chromaMin; | ||
| const h = fraction(0.5 + ALPHA_HUE * index) * 360; | ||
| const l = | ||
| envelope.lightnessMin + | ||
| fraction(0.5 + ALPHA_LIGHTNESS * index) * lightnessRange; | ||
| const c = | ||
| envelope.chromaMin + fraction(0.5 + ALPHA_CHROMA * index) * chromaRange; | ||
| return colord({ l, c, h }); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Measure how far sequenceColor output drifts from its envelope after sRGB clipping.
set -euo pipefail
fd -t f 'ColorGenerator.ts' src
fd -t f 'default-theme.json' src
node -e '
const { colord, extend } = require("colord");
const lch = require("colord/plugins/lch").default;
extend([lch]);
const theme = require("./src/client/render/gl/default-theme.json");
const TRIM = 0.05, FLOOR = 30, CEIL = 88;
const clamp = (v, lo, hi) => Math.min(hi, Math.max(lo, v));
const range = (vals) => {
const s = [...vals].sort((a, b) => a - b);
const d = Math.floor(s.length * TRIM);
return [s[d], s[Math.max(d, s.length - 1 - d)]];
};
const envelope = (hexes) => {
const v = hexes.map((h) => colord(h).toLch());
const [lLow, lHigh] = range(v.map((x) => x.l));
const [cLow, cHigh] = range(v.map((x) => x.c));
return {
lightnessMin: clamp(lLow, FLOOR, CEIL),
lightnessMax: clamp(lHigh, FLOOR, CEIL),
chromaMin: Math.max(0, cLow),
chromaMax: Math.max(cHigh, cLow + 12),
};
};
const P = 1.2207440846057596;
const frac = (x) => x - Math.floor(x);
const seq = (i, e) => colord({
h: frac(0.5 + i / P) * 360,
l: e.lightnessMin + frac(0.5 + i / (P * P)) * (e.lightnessMax - e.lightnessMin),
c: e.chromaMin + frac(0.5 + i / (P * P * P)) * (e.chromaMax - e.chromaMin),
});
for (const name of ["humanColors", "nationColors", "botColors"]) {
const e = envelope(theme[name]);
const seen = new Set();
let outsideL = 0, outsideC = 0;
for (let i = 0; i < 2048; i++) {
const c = seq(i, e);
seen.add(c.toHex());
const back = c.toLch();
if (back.l < e.lightnessMin - 1 || back.l > e.lightnessMax + 1) outsideL++;
if (back.c < e.chromaMin - 1 || back.c > e.chromaMax + 1) outsideC++;
}
console.log(name, JSON.stringify(e));
console.log(" unique of 2048:", seen.size, "lightness drift:", outsideL, "chroma drift:", outsideC);
}
'Repository: openfrontio/OpenFrontIO
Length of output: 1088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
fd -t f 'ColorGenerator.ts|default-theme.json|Colors.test.ts' .
printf '\nColorGenerator outline:\n'
ast-grep outline src/client/theme/ColorGenerator.ts || true
printf '\nRelevant ColorGenerator lines:\n'
sed -n '1,160p' src/client/theme/ColorGenerator.ts
printf '\nReferenced theme snippets:\n'
python3 - <<'PY'
import json
from pathlib import Path
p = Path('src/client/render/gl/default-theme.json')
data=json.loads(p.read_text())
for name in ["humanColors","nationColors","botColors"]:
v=data.get(name, [])
print(name, len(v), v[:18], v[-18:])
PY
printf '\nRelevant test lines:\n'
sed -n '290,330p' tests/Colors.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 7535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the declared colord version and whether node_modules has the package.
printf 'package colord declaration:\n'
node - <<'JS'
const fs = require('fs');
for (const f of ['package.json','package-lock.json','yarn.lock','pnpm-lock.yaml']) {
const p = require('path').resolve(f);
if (fs.existsSync(p)) {
const s = fs.readFileSync(p, 'utf8');
if (f.endsWith('.json')) {
try {
const data = JSON.parse(s);
console.log(f, data.dependencies?.colord || data.devDependencies?.colord || data.resolutions?.colord || data.pnpm?.overrides?.colord || 'none');
} catch{}
} else {
const m = s.match(/colord@[\w@~^<>=]+|["']colord["']:\s*version:\s*["']([^"']+)["']|colord:\s+\^?([0-9.]+)/);
console.log(f, m ? m[0].slice(0, 120) : 'none');
}
}
}
JS
printf '\nNode availability/lockfile status:\n'
if [ -d node_modules/colord ]; then node -e "const {colord, extend} = require('colord'); const lch = require('colord/plugins/lch').default; extend([lch]); console.log(colord({l:100, c:120, h:0}).toHex());"; else echo "node_modules/colord absent"; fi
printf '\nSearch for colord import usage in ColorGenerator tests:\n'
rg -n "colord|toLch|sequenceColor|sequence_color|envelope" tests/src/client/theme src/client/theme tests -g '*.ts' || trueRepository: openfrontio/OpenFrontIO
Length of output: 9175
🌐 Web query:
colord 2.9.3 LCH plugin clamps rgb on toRgb toHex convert out of gamut
💡 Result:
In colord 2.9.3, the LCH plugin and the underlying conversion logic do not perform gamut mapping for out-of-gamut colors when converting to formats like RGB or Hex [1]. Colord uses RGBA as its internal color representation [1]. Because the internal state is fundamentally bound to the sRGB color space, any conversion from wide-gamut spaces (like LCH) to sRGB-based formats (like.toRgb or.toHex) essentially results in naive clipping [1]. This means that colors exceeding the sRGB gamut are clamped to the nearest representable sRGB color rather than being mapped using sophisticated algorithms (such as the chroma-reduction method described in CSS Color Module Level 4) [1]. Users experiencing "wrong" color output or unexpected precision loss when working with LCH in colord are encountering this limitation [1]. If your workflow requires high-fidelity color conversion that handles out-of-gamut colors without naive clipping—or if you need native support for gamut mapping algorithms—you may need to use a library designed with wide-gamut support, such as colordx, which is an API-compatible fork of colord that explicitly implements CSS Color 4 gamut mapping and native wide-gamut color space support [2][3][4].
Citations:
- 1: I get wrong color output when input color is LCH omgovich/colord#103
- 2: https://registry.npmjs.org/@colordx/core
- 3: https://www.npmjs.com/package/@colordx/core
- 4: https://colordx.dev/
🌐 Web query:
colord 2.9.3 toLch hex rgb gamma corrected internal conversion
💡 Result:
In the JavaScript library colord (version 2.9.3), internal color space conversions—including those for LCH, LAB, and XYZ—are performed with a D50 white point, consistent with modern CSS color specifications [1][2]. The library utilizes RGBA as its internal intermediary format for color calculations [3]. Because of this, when converting to or from color spaces like LCH, precision may be limited to the sRGB color space [3]. When performing conversions between color spaces, the library follows logic ported from the CSS Color Module Level 4 Specification [4][5][6]. Note that there are two distinct projects often referred to as "colord": 1. The JavaScript library (github.com/omgovich/colord), which provides the.toLch method and is the subject of the details above [4][3][5]. 2. The system-level color management daemon (freedesktop.org/software/colord), which is a separate C-based project for Linux systems and uses Little CMS (lcms2) for ICC color profile transformations, rather than the JavaScript-based conversion logic [7][8]. If you are using the JavaScript colord library, its internal architecture uses sRGB-based RGBA as the core, meaning operations are not natively high-precision P3 or other wide-gamut spaces [3]. Users requiring high-precision handling across non-sRGB color spaces often look to alternative libraries designed specifically for that purpose [3].
Citations:
- 1: https://github.com/omgovich/colord/blob/master/CHANGELOG.md
- 2: https://npmx.dev/package-changelog/colord/v/2.9.3
- 3: I get wrong color output when input color is LCH omgovich/colord#103
- 4: https://github.com/omgovich/colord
- 5: https://npmx.dev/package/colord/v/2.9.3
- 6: https://github.com/omgovich/colord/blob/master/README.md
- 7: https://github.com/hughsie/colord/blob/master/lib/colord/cd-transform.c
- 8: https://www.freedesktop.org/software/colord/gtk-doc/colord-cd-color.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant allocator lines:\n'
sed -n '150,190p' src/client/theme/ColorAllocator.ts
printf '\nRelevant registry lines:\n'
sed -n '1,120p' src/client/theme/ColorRegistry.ts
printf '\nTest lines around envelope assertions:\n'
sed -n '340,370p' tests/Colors.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 6735
Clamp saturated sequenceColor inputs before using the clipped colour
colord({ l, c, h }) stores RGBA internally, so high-chroma LCH values from the envelope clip when converted to sRGB. This can make distinct sequence indices score the same 8-bit lab candidate, and it lets high-chroma envelopes drift past the palette’s chroma ceiling. Clamp chroma until toLch() stays in the generated envelope, then continue to score with toLab().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/theme/ColorGenerator.ts` around lines 94 - 104, Update
sequenceColor to clamp the generated chroma before constructing the colord
value, reducing it as needed until the resulting toLch() remains within the
envelope’s chroma bounds. Preserve the generated hue and lightness, then
continue downstream scoring with toLab() so clipped saturated colors do not
produce duplicate candidates or exceed the palette chroma ceiling.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Visual evidenceGenerated straight from the code in this PR, so they reproduce exactly rather 1. A 100-player lobby, before and after. Four blocks: before / after under
The first block degenerates into a run of near-identical dark reds once the 2. Palette character per player type. Players, nations, tribes — shipped
The intended hierarchy survives: players vivid, nations restrained, tribes 3. Nation colours under each vision model — normal, deuteranopia,
This one does not flatter the change, deliberately. Under deuteranopia and These show colour allocation, not the rendered map — territory fills draw at |
Colour allocation compares each new colour against thousands of fixed candidates. colord's delta() converts both operands from sRGB to LAB on every call, so those conversions dominated: a 125-player lobby spent 2.2s in allocation. Converting once per candidate and taking LAB directly cuts that to 0.8s with no change in the colours chosen. The reference implementation is also more accurate. colord's delta() disagrees with the CIEDE2000 formula by up to 2.5 on some near-neutral pairs; this one matches the Sharma, Wu & Dalal (2005) test data to five decimal places.
The chroma weighting terms raise values to the seventh power, which ran as a Math.pow call for every candidate comparison. Replacing it with multiplication and hoisting the 25^7 constant halves the cost of the first generated-colour allocation.
Human, nation and bot colours were allocated from separate palettes that never compared against one another. A player looking at the map cannot tell those types apart, so the separation that mattered was the one nobody was measuring: a nation could land 0.61 from a human, and in the colourblind theme a human could be handed a bot's exact colour. A shared registry now holds every colour in play. Bot palettes are reserved rather than allocated, since hundreds of bots share a small palette by design, but their colours are still on the map so everyone else keeps clear of them. Synthesised colours come from a low-discrepancy sequence rather than a grid sweep, covering the same volume with a third of the entries. Worst separation across a full World game, by pair: humans/nations 0.61 -> 3.63, humans/bots 1.98 -> 10.34, nations/bots 0.32 -> 3.45, and allocation 880ms -> 540ms.
Each player type's palette has its own look, and that look carries meaning: humans are vivid (mean chroma 53), nations noticeably more restrained (33), bots nearly grey (17). Players read those differences without being told which type they are looking at. Generation swept one fixed region of LCH regardless of who it was serving. Because most nation colours in a large game are synthesised, nations drifted 11 points darker and 15 points more saturated, and the human-to-nation chroma gap collapsed from 20 to 8 - nations started looking like players. Each allocator now derives its own region from the palette it extends, so a synthesised nation colour stays a nation colour. The gap holds at 19. The cost is separation: nations sit at 2.4-2.5 rather than 3.4, still above the just-noticeable threshold but with less headroom, since the nation palette occupies a narrow band by design.
select() fell through to bestUnused(primary) ?? bestUnused(fallback), so the fallback palette was never considered while the primary held any unused colour. The allocator could then synthesise a colour no better than one the fallback already offered. Worst-case separation in a 125-player lobby improves from 3.18 to 3.40 (default theme) and 2.67 to 2.95 (colourblind). Also from review: merge the two colord extend calls, extract the pool-refresh loop shared by commit() and reserve(), move the CIEDE2000 doc block onto the function it describes, give the CVD matrices a fixed-length tuple type, and document the equal-observer invariant on distance(). Tests now cover the roomiest-choice rule and run the theme wiring checks over both palettes.
fb0e528 to
97a4a08
Compare
|
Thanks — the Fixed
This was not cosmetic. Worst-case separation in a 125-player lobby:
Also applied: merged the two Adjusted rather than taken as writtenThe suggested test asserted the fallback colour is issued before anything Not applied, with reasonsClamping chroma in Removing Removing Rebased onto current |
|
Hi there, by coincidence I noticed this issue/PR just as I was thinking about color-related issues. How does this PR fare when put in context of team games? Does team game colors still suffer from unacceptable color similarity? From #4949, while I believe I have normal vision, I cannot help but notice that:
It seems like a good idea to let this PR also tackle this team-based color similarity. |
3 participants
Resolves #4928
Player colours could be identical or indistinguishable, worst under colour
vision deficiency. A full 125-player lobby issued 118 distinct colours to 125
players — seven pairs sharing a byte-identical colour.
ColorAllocatoralready maximised the minimum CIEDE2000 distance to coloursalready handed out. The approach was right; this closes five gaps in it.
What was wrong
fallbackColors, built as[...colors, ...fallback]— including coloursalready issued.
performance, i.e. for players 51–125 of a full lobby.
palette but not the metric.
to the default theme's, and containing
#ffcdfftwice.allocated from separate palettes through separate allocators, so a nation
could land 0.61 from a human — invisible to every existing check, and the
separation players actually see.
Also:
botColorsandnationColorswere each passed as their own fallback, soevery entry appeared twice in the fallback list.
What changed
case across all of them.
across all players at once rather than per class.
distance to the nearest colour in play, updated incrementally — O(candidates)
per allocation instead of O(candidates × assigned). A full World game got
faster: 880ms → ~540ms.
from a low-discrepancy (R3) sweep of LCH space.
palette has its own look — humans vivid (mean chroma 53), nations restrained
(33), bots nearly grey (17) — and players read that without being told. A first
attempt swept one fixed region for everyone and drifted nations 15 points more
saturated, collapsing the human-to-nation chroma gap from 20 to 8. Each
allocator now derives its region from its own palette; the gap holds at 19.
distinct, and trying would crowd out the players it matters most to tell
apart. Their colours are reserved so everyone else stays clear of them.
observersanddistinctnessFloorare theme JSON data, so the behaviour istunable per theme without code changes.
Results
Worst separation between any two players, across every vision model the theme
checks:
Between classes, World map (8 humans, 72 nations, 400 bots):
Every pair now clears the ~2.3 just-noticeable threshold; three did not before.
Trade-offs I would rather you saw than discovered
from the shipped palette; the rest are synthesised. Keeping them inside the
muted nation band is also what makes the nation pairs the tightest (2.48 /
2.52). Letting them saturate would reach ~3.4 but make nations read like
players.
distinctnessFlooris the dial, and it is data — say the word and Iwill change the number.
threshold but not comfortably. Patterns are the obvious next axis, but they
are purchasable cosmetics, so that is a product decision — noted in Players can be given identical or indistinguishable colours, especially under colour vision deficiency #4928 as
future work, deliberately not attempted here.
Colord.delta()disagrees with the CIEDE2000 reference formula by up to2.5 on near-neutral pairs (
#e6fffavs#ffcce5: colord 31.60, reference29.13). The previous allocator used it. Replaced with an implementation
validated against the Sharma, Wu & Dalal (2005) reference data, which also
avoids re-converting sRGB→LAB on every comparison.
Testing
tests/Colors.test.tsgoes from 10 to 41 tests. The file's diff is purelyadditive — no existing test was modified or removed.
New coverage: CVD simulation against published reference values; CIEDE2000
against the Sharma reference dataset; sequence determinism and spread; no
duplicate colour across 125 allocations; the distinctness floor honoured;
determinism across runs; nothing synthesised at 48 players; cross-pool
separation above threshold with a shared registry; no colour issued twice across
pools; synthesised colours staying inside their palette's character; theme JSONs
declaring both new fields and containing no duplicate entries.
npm test,npm run lintandtsc --noEmitclean. Three pre-existingfs.globSynctimeouts inMapConsistency/MapManifestFlags/NationNameoccur identically on unmodified
mainunder full-suite concurrency and pass inisolation.
Scope
Client-side only:
src/client/theme/plus the two theme JSONs. Nothing insrc/core— the simulation never reads colours. No new dependency; the CVDmatrices are inlined constants.
Discord: dr-youvi-avant