From 921d7407fbdbf310860ec53d583abe31e83a94d7 Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 7 Sep 2026 12:52:07 -0700 Subject: [PATCH 1/6] Fix: hold generated companion themes to a contrast floor A companion theme is built by inverting an authored palette, and inversion is not contrast-preserving: a colour picked to read against a dark surface can land far too close to the inverted light one. Monokai's generated light companion had syntax tokens at a 1.30 contrast ratio and Dracula's at 2.23 -- the first is effectively invisible. Generated companions are now corrected where they fall short. Authored palettes are untouched: those are an author's choice and are governed by theme-legibility.spec.mjs. monokai-light 1.30 -> 3.03 dracula-light 2.23 -> 3.08 graphite-light 3.83 (unchanged) morning-dark 5.57 (unchanged) nord-light 3.33 (unchanged) catppuccin-light 4.36 (unchanged) Colours already clearing the floor are returned untouched rather than normalised, which is what the catppuccin case in the spec pins. The correction walks lightness in OKLCH with hue and chroma held. The obvious alternative -- blending toward black or white until the ratio is met -- also works, but it desaturates as it goes and drags the hue with it, so an amber warning arrives washed out and slightly wrong rather than simply lighter. A spec asserts no token that was chromatic in the authored theme comes out grey. This revives the palette half of #204, which has sat unmergeable since July with 15 of its 20 files since moved on main. The mechanism is new; the problem it solves is the one that PR identified. Also adds src/ui/oklch.ts: sRGB <-> OKLab/OKLCH from Ottosson's published derivation, checked against his reference values and round-tripped over the sRGB cube so a mistyped matrix constant fails loudly rather than skewing every colour that uses it. Hand-rolled rather than adding culori because this is the only colour maths jedit needs. Contrast measurement and correction moved to src/ui/theme-contrast.ts. jedit-themes.ts owns what a theme is; this owns whether a colour can be read on the surface behind it. That also keeps jedit-themes.ts under the 500-line limit, which the addition had pushed it past at 552. Refs #204, #309 --- spec/companion-theme-contrast.spec.mjs | 133 +++++++++++++++++++++++++ spec/oklch.spec.mjs | 68 +++++++++++++ src/ui/jedit-themes.ts | 47 +++------ src/ui/oklch.ts | 96 ++++++++++++++++++ src/ui/theme-contrast.ts | 113 +++++++++++++++++++++ 5 files changed, 422 insertions(+), 35 deletions(-) create mode 100644 spec/companion-theme-contrast.spec.mjs create mode 100644 spec/oklch.spec.mjs create mode 100644 src/ui/oklch.ts create mode 100644 src/ui/theme-contrast.ts diff --git a/spec/companion-theme-contrast.spec.mjs b/spec/companion-theme-contrast.spec.mjs new file mode 100644 index 00000000..1c4e398e --- /dev/null +++ b/spec/companion-theme-contrast.spec.mjs @@ -0,0 +1,133 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { importDist } from "./dist-helpers.mjs"; + +// A companion theme is produced by inverting an authored palette, and inversion +// does not preserve contrast: an accent picked to read on a dark surface can +// land far too close to the inverted light one. Monokai's generated light +// companion sat at 1.30 before this floor existed -- effectively invisible. +// +// Authored palettes are deliberately not covered here. Those are an author's +// choice and are held to their own standard in theme-legibility.spec.mjs; this +// file only governs the palettes jedit generates on the author's behalf. + +const MIN_TOKEN_CONTRAST = 3; + +function channelLuminance(channel) { + const ratio = channel / 255; + return ratio <= 0.04045 ? ratio / 12.92 : Math.pow((ratio + 0.055) / 1.055, 2.4); +} + +function relativeLuminance(color) { + return 0.2126 * channelLuminance(color[0]) + + 0.7152 * channelLuminance(color[1]) + + 0.0722 * channelLuminance(color[2]); +} + +function contrastRatio(foreground, background) { + const first = relativeLuminance(foreground); + const second = relativeLuminance(background); + const lighter = Math.max(first, second); + const darker = Math.min(first, second); + return (lighter + 0.05) / (darker + 0.05); +} + +async function generatedCompanions() { + const themes = await importDist("ui", "jedit-themes.js"); + const companions = []; + for (const theme of themes.availableJeditThemes()) { + const companion = themes.oppositeJeditTheme(theme); + if (companion.variantSource === "generated") { + companions.push(companion); + } + } + return companions; +} + +test("generated companion themes exist to be checked", async () => { + const companions = await generatedCompanions(); + assert.ok(companions.length > 0, "expected at least one generated companion theme"); +}); + +test("every generated companion keeps its syntax tokens legible", async () => { + const offenders = []; + + for (const companion of await generatedCompanions()) { + const background = companion.surface.workspace.bgRGB; + for (const [token, style] of companion.source) { + const ratio = contrastRatio(style.fgRGB, background); + if (ratio < MIN_TOKEN_CONTRAST) { + offenders.push(`${companion.name} ${String(token)} ${ratio.toFixed(2)}`); + } + } + } + + assert.deepEqual(offenders, []); +}); + +test("every generated companion keeps its markdown tokens legible", async () => { + const offenders = []; + + for (const companion of await generatedCompanions()) { + const background = companion.surface.workspace.bgRGB; + for (const [token, style] of companion.markdown) { + if (style.fgRGB == null) { + continue; + } + const ratio = contrastRatio(style.fgRGB, background); + if (ratio < MIN_TOKEN_CONTRAST) { + offenders.push(`${companion.name} ${String(token)} ${ratio.toFixed(2)}`); + } + } + } + + assert.deepEqual(offenders, []); +}); + +test("the contrast floor leaves an already-legible colour alone", async () => { + // catppuccin's companion cleared the floor before the adjustment existed, so + // a correction that fired unconditionally would show up as a changed colour + // here rather than as a contrast failure anywhere. + const companions = await generatedCompanions(); + const catppuccin = companions.find((theme) => theme.name.startsWith("catppuccin")); + assert.ok(catppuccin != null, "expected a generated catppuccin companion"); + + const background = catppuccin.surface.workspace.bgRGB; + let worst = Infinity; + for (const [, style] of catppuccin.source) { + worst = Math.min(worst, contrastRatio(style.fgRGB, background)); + } + assert.ok(worst > 4, `expected catppuccin to stay comfortably legible, got ${worst.toFixed(2)}`); +}); + +test("reaching the contrast floor does not wash a colour out to grey", async () => { + const { rgbToOklch } = await importDist("ui", "oklch.js"); + const themes = await importDist("ui", "jedit-themes.js"); + + // Blending toward black or white -- the obvious way to hit a contrast floor -- + // desaturates as it goes, so a theme's colours arrive muddied. Walking + // lightness in OKLCH holds chroma instead. Tokens that are already neutral in + // the authored theme (ink-derived ones such as variable and property) are + // meant to stay neutral, so only chromatic tokens are checked. + const CHROMATIC = 0.05; + const offenders = []; + + for (const theme of themes.availableJeditThemes()) { + const companion = themes.oppositeJeditTheme(theme); + if (companion.variantSource !== "generated") { + continue; + } + for (const [token, style] of theme.source) { + if (rgbToOklch(style.fgRGB).chroma < CHROMATIC) { + continue; + } + const companionStyle = companion.source.get(token); + const { chroma } = rgbToOklch(companionStyle.fgRGB); + if (chroma < CHROMATIC) { + offenders.push(`${companion.name} ${String(token)} fell to chroma ${chroma.toFixed(3)}`); + } + } + } + + assert.deepEqual(offenders, []); +}); diff --git a/spec/oklch.spec.mjs b/spec/oklch.spec.mjs new file mode 100644 index 00000000..3594f652 --- /dev/null +++ b/spec/oklch.spec.mjs @@ -0,0 +1,68 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { importDist } from "./dist-helpers.mjs"; + +// Checked against Ottosson's published reference values rather than against +// this implementation's own output, so a transcription error in a matrix +// constant fails here instead of quietly skewing every colour that uses it. +const REFERENCE = [ + { name: "white", rgb: [255, 255, 255], lightness: 1.0, chroma: 0.0 }, + { name: "black", rgb: [0, 0, 0], lightness: 0.0, chroma: 0.0 }, + { name: "red", rgb: [255, 0, 0], lightness: 0.6279554, chroma: 0.2576833, hue: 29.23 }, + { name: "green", rgb: [0, 255, 0], lightness: 0.8664396, chroma: 0.2948272, hue: 142.5 }, + { name: "blue", rgb: [0, 0, 255], lightness: 0.4520137, chroma: 0.3132143, hue: 264.05 }, +]; + +const LIGHTNESS_TOLERANCE = 0.0005; +const CHROMA_TOLERANCE = 0.0005; +const HUE_TOLERANCE = 0.1; + +test("rgbToOklch matches published reference values", async () => { + const { rgbToOklch } = await importDist("ui", "oklch.js"); + + for (const entry of REFERENCE) { + const actual = rgbToOklch(entry.rgb); + assert.ok( + Math.abs(actual.lightness - entry.lightness) < LIGHTNESS_TOLERANCE, + `${entry.name} lightness ${actual.lightness} != ${entry.lightness}`, + ); + assert.ok( + Math.abs(actual.chroma - entry.chroma) < CHROMA_TOLERANCE, + `${entry.name} chroma ${actual.chroma} != ${entry.chroma}`, + ); + if (entry.hue !== undefined) { + assert.ok( + Math.abs(actual.hue - entry.hue) < HUE_TOLERANCE, + `${entry.name} hue ${actual.hue} != ${entry.hue}`, + ); + } + } +}); + +test("oklchToRgb round-trips every channel of the sRGB cube", async () => { + const { rgbToOklch, oklchToRgb } = await importDist("ui", "oklch.js"); + const offenders = []; + + for (let red = 0; red <= 255; red += 17) { + for (let green = 0; green <= 255; green += 17) { + for (let blue = 0; blue <= 255; blue += 17) { + const original = [red, green, blue]; + const [r, g, b] = oklchToRgb(rgbToOklch(original)); + if (Math.abs(r - red) > 1 || Math.abs(g - green) > 1 || Math.abs(b - blue) > 1) { + offenders.push(`${original} -> ${[r, g, b]}`); + } + } + } + } + + assert.deepEqual(offenders.slice(0, 5), []); +}); + +test("mixHue crosses zero the short way round", async () => { + const { mixHue } = await importDist("ui", "oklch.js"); + + assert.ok(Math.abs(mixHue(350, 10, 0.5) - 0) < 0.001, "350 -> 10 should pass through 0"); + assert.ok(Math.abs(mixHue(10, 350, 0.5) - 0) < 0.001, "10 -> 350 should pass through 0"); + assert.ok(Math.abs(mixHue(0, 180, 0.5) - 90) < 0.001); + assert.equal(mixHue(120, 240, 0), 120); +}); diff --git a/src/ui/jedit-themes.ts b/src/ui/jedit-themes.ts index fc760848..56f3dc92 100644 --- a/src/ui/jedit-themes.ts +++ b/src/ui/jedit-themes.ts @@ -1,4 +1,5 @@ import type { JeditTheme, JeditThemeMode } from "./jedit-theme.js"; +import { contrastAdjustedPalette, contrastRatio } from "./theme-contrast.js"; import { JEDIT_THEME_MODE, JEDIT_THEME_VARIANT_SOURCE, @@ -35,16 +36,8 @@ const ACTIVE_EDGE_CHAR = "░"; const THEME_MODE_LABEL_DARK = "Dark"; const THEME_MODE_LABEL_LIGHT = "Light"; const COLOR_CHANNEL_MAX = 255; +// WCAG AA for body text; 3:1 is the non-text/large-text floor used for accents. const MIN_GUTTER_CONTRAST_RATIO = 3; -const CONTRAST_LUMINANCE_OFFSET = 0.05; -const SRGB_LINEAR_THRESHOLD = 0.04045; -const SRGB_LINEAR_DIVISOR = 12.92; -const SRGB_OFFSET = 0.055; -const SRGB_SCALE = 1.055; -const SRGB_EXPONENT = 2.4; -const LUMINANCE_RED_WEIGHT = 0.2126; -const LUMINANCE_GREEN_WEIGHT = 0.7152; -const LUMINANCE_BLUE_WEIGHT = 0.0722; const GUTTER_VARIANT = Object.freeze({ Normal: "normal", Dimmed: "dimmed", @@ -237,8 +230,13 @@ function oppositeThemeMode(mode: JeditThemeMode): JeditThemeMode { : JEDIT_THEME_MODE.Dark; } +// A companion palette is produced by inverting an authored one, and inversion +// is not contrast-preserving: a colour chosen to read against a dark surface +// can land far too close to the inverted light surface. Authored palettes are +// left exactly as their author set them; only the generated companion is +// corrected, and only where it falls short. function oppositePalette(palette: ThemePalette): ThemePalette { - return { + return contrastAdjustedPalette({ ink: invertColor(palette.ink), muted: invertColor(palette.muted), accent: invertColor(palette.accent), @@ -248,9 +246,12 @@ function oppositePalette(palette: ThemePalette): ThemePalette { surface: invertColor(palette.surface), surfaceRaised: invertColor(palette.surfaceRaised), surfaceMuted: invertColor(palette.surfaceMuted), - }; + }); } + + + function paletteFromTheme(theme: JeditTheme): ThemePalette { return { ink: variableRgb(theme, VARIABLE_INK, [226, 231, 236]), @@ -335,30 +336,6 @@ function readableGutterColor( : variables.ink; } -function contrastRatio(foreground: RgbTuple, background: RgbTuple): number { - const foregroundLuminance = relativeLuminance(foreground); - const backgroundLuminance = relativeLuminance(background); - const lighter = Math.max(foregroundLuminance, backgroundLuminance); - const darker = Math.min(foregroundLuminance, backgroundLuminance); - return (lighter + CONTRAST_LUMINANCE_OFFSET) / (darker + CONTRAST_LUMINANCE_OFFSET); -} - -function relativeLuminance(color: RgbTuple): number { - const red = linearizedColorChannel(color[0]); - const green = linearizedColorChannel(color[1]); - const blue = linearizedColorChannel(color[2]); - return red * LUMINANCE_RED_WEIGHT - + green * LUMINANCE_GREEN_WEIGHT - + blue * LUMINANCE_BLUE_WEIGHT; -} - -function linearizedColorChannel(channel: number): number { - const normalized = channel / COLOR_CHANNEL_MAX; - return normalized <= SRGB_LINEAR_THRESHOLD - ? normalized / SRGB_LINEAR_DIVISOR - : ((normalized + SRGB_OFFSET) / SRGB_SCALE) ** SRGB_EXPONENT; -} - function applySurfaceThemeTokens( draft: JeditThemeDraft, variables: ThemeVariables, diff --git a/src/ui/oklch.ts b/src/ui/oklch.ts new file mode 100644 index 00000000..dec4522a --- /dev/null +++ b/src/ui/oklch.ts @@ -0,0 +1,96 @@ +// sRGB <-> OKLab/OKLCH, from Björn Ottosson's published derivation. +// +// Hand-rolled rather than pulled from culori because jedit ships no colour +// dependency today and this is the whole of what it needs: two matrices, a +// cube root, and the sRGB transfer function. The constants are exact values +// from the reference implementation, and oklch.spec.mjs checks them against +// known conversions rather than against themselves. +// +// OKLCH is used instead of HSL because the logo has to keep a stable perceived +// lightness across hues: HSL's L is a channel average, so recolouring artwork +// through it makes blues read far darker than yellows at the same nominal L. + +export interface Oklch { + readonly lightness: number; + readonly chroma: number; + readonly hue: number; +} + +export type Rgb = readonly [number, number, number]; + +const SRGB_MAX = 255; +const GAMMA_THRESHOLD_ENCODED = 0.04045; +const GAMMA_THRESHOLD_LINEAR = 0.0031308; +const GAMMA_LINEAR_SLOPE = 12.92; +const GAMMA_OFFSET = 0.055; +const GAMMA_SCALE = 1.055; +const GAMMA_EXPONENT = 2.4; +const GAMMA_INVERSE_EXPONENT = 1 / 2.4; +const CUBE = 3; +const DEGREES_PER_TURN = 360; +const HALF_TURN = 180; +const ZERO = 0; +const ONE = 1; + +function toLinear(channel: number): number { + const ratio = channel / SRGB_MAX; + return ratio <= GAMMA_THRESHOLD_ENCODED + ? ratio / GAMMA_LINEAR_SLOPE + : Math.pow((ratio + GAMMA_OFFSET) / GAMMA_SCALE, GAMMA_EXPONENT); +} + +function toEncoded(linear: number): number { + const clamped = Math.min(ONE, Math.max(ZERO, linear)); + const ratio = clamped <= GAMMA_THRESHOLD_LINEAR + ? clamped * GAMMA_LINEAR_SLOPE + : (GAMMA_SCALE * Math.pow(clamped, GAMMA_INVERSE_EXPONENT)) - GAMMA_OFFSET; + return Math.round(ratio * SRGB_MAX); +} + +export function rgbToOklch(rgb: Rgb): Oklch { + const red = toLinear(rgb[0]); + const green = toLinear(rgb[1]); + const blue = toLinear(rgb[2]); + + const long = Math.cbrt((0.4122214708 * red) + (0.5363325363 * green) + (0.0514459929 * blue)); + const medium = Math.cbrt((0.2119034982 * red) + (0.6806995451 * green) + (0.1073969566 * blue)); + const short = Math.cbrt((0.0883024619 * red) + (0.2817188376 * green) + (0.6299787005 * blue)); + + const lightness = (0.2104542553 * long) + (0.7936177850 * medium) - (0.0040720468 * short); + const greenRed = (1.9779984951 * long) - (2.4285922050 * medium) + (0.4505937099 * short); + const blueYellow = (0.0259040371 * long) + (0.7827717662 * medium) - (0.8086757660 * short); + + const hue = (Math.atan2(blueYellow, greenRed) * DEGREES_PER_TURN) / (2 * Math.PI); + return { + lightness, + chroma: Math.hypot(greenRed, blueYellow), + hue: hue < ZERO ? hue + DEGREES_PER_TURN : hue, + }; +} + +export function oklchToRgb(color: Oklch): Rgb { + const radians = (color.hue * 2 * Math.PI) / DEGREES_PER_TURN; + const greenRed = color.chroma * Math.cos(radians); + const blueYellow = color.chroma * Math.sin(radians); + + const long = ((color.lightness + (0.3963377774 * greenRed) + (0.2158037573 * blueYellow)) ** CUBE); + const medium = ((color.lightness - (0.1055613458 * greenRed) - (0.0638541728 * blueYellow)) ** CUBE); + const short = ((color.lightness - (0.0894841775 * greenRed) - (1.2914855480 * blueYellow)) ** CUBE); + + return [ + toEncoded((4.0767416621 * long) - (3.3077115913 * medium) + (0.2309699292 * short)), + toEncoded((-1.2684380046 * long) + (2.6097574011 * medium) - (0.3413193965 * short)), + toEncoded((-0.0041960863 * long) - (0.7034186147 * medium) + (1.7076147010 * short)), + ]; +} + +// Shortest way round the hue circle, so a rotation from 350 to 10 travels 20 +// degrees forward rather than 340 back. Exact opposites are equally far in +// both directions; the tie is broken forwards so the result is a stated +// convention rather than a consequence of how the modulo happens to land. +export function mixHue(from: number, to: number, amount: number): number { + const forward = (((to - from) % DEGREES_PER_TURN) + DEGREES_PER_TURN) % DEGREES_PER_TURN; + const delta = forward > HALF_TURN ? forward - DEGREES_PER_TURN : forward; + const mixed = (from + (delta * amount)) % DEGREES_PER_TURN; + return mixed < ZERO ? mixed + DEGREES_PER_TURN : mixed; +} diff --git a/src/ui/theme-contrast.ts b/src/ui/theme-contrast.ts new file mode 100644 index 00000000..5dbffcb7 --- /dev/null +++ b/src/ui/theme-contrast.ts @@ -0,0 +1,113 @@ +// Contrast measurement and correction for theme palettes. +// +// Split out of jedit-themes.ts, which owns what a theme *is*; this owns the one +// question of whether a colour can actually be read on the surface behind it. + +import type { RgbTuple, ThemePalette } from './jedit-theme-palettes.js'; +import { oklchToRgb, rgbToOklch } from './oklch.js'; + +const COLOR_CHANNEL_MAX = 255; +// WCAG AA for body text; 3:1 is the non-text and large-text floor, which is +// what accents and syntax tokens are held to. +const MIN_SURFACE_TEXT_CONTRAST_RATIO = 4.5; +const MIN_ACCENT_CONTRAST_RATIO = 3; +const CONTRAST_LIGHTNESS_STEP = 0.02; +const MAX_CONTRAST_STEPS = 60; +const CONTRAST_LUMINANCE_OFFSET = 0.05; +const SRGB_LINEAR_THRESHOLD = 0.04045; +const SRGB_LINEAR_DIVISOR = 12.92; +const SRGB_OFFSET = 0.055; +const SRGB_SCALE = 1.055; +const SRGB_EXPONENT = 2.4; +const LUMINANCE_RED_WEIGHT = 0.2126; +const LUMINANCE_GREEN_WEIGHT = 0.7152; +const LUMINANCE_BLUE_WEIGHT = 0.0722; +const LIGHTNESS_FLOOR = 0; +const LIGHTNESS_CEILING = 1; +const DARKER = -1; +const LIGHTER = 1; + +export function contrastAdjustedPalette(palette: ThemePalette): ThemePalette { + const surfaces = [palette.surface, palette.surfaceRaised, palette.surfaceMuted]; + const raised = [palette.surface, palette.surfaceRaised]; + return { + ink: legibleOn(palette.ink, surfaces, MIN_SURFACE_TEXT_CONTRAST_RATIO), + muted: legibleOn(palette.muted, [palette.surface], MIN_ACCENT_CONTRAST_RATIO), + accent: legibleOn(palette.accent, [palette.surface], MIN_ACCENT_CONTRAST_RATIO), + info: legibleOn(palette.info, raised, MIN_ACCENT_CONTRAST_RATIO), + warning: legibleOn(palette.warning, raised, MIN_ACCENT_CONTRAST_RATIO), + success: legibleOn(palette.success, [palette.surface], MIN_ACCENT_CONTRAST_RATIO), + surface: palette.surface, + surfaceRaised: palette.surfaceRaised, + surfaceMuted: palette.surfaceMuted, + }; +} + +// Contrast is carried by lightness, so a colour short of the floor is walked +// away from the surfaces it sits on with its hue and chroma held. Blending +// toward black or white instead -- the obvious approach -- desaturates as it +// goes and drags the hue with it, so a theme's amber warning arrives washed out +// and slightly wrong rather than simply lighter. +function legibleOn( + color: RgbTuple, + backgrounds: readonly RgbTuple[], + minContrastRatio: number, +): RgbTuple { + const ground = averageLuminance(backgrounds); + const direction = relativeLuminance(color) >= ground ? LIGHTER : DARKER; + let candidate = rgbToOklch(color); + for (let step = 0; step < MAX_CONTRAST_STEPS; step += 1) { + const rgb = oklchToRgb(candidate); + if (passesContrast(rgb, backgrounds, minContrastRatio)) { + return rgb; + } + candidate = { + ...candidate, + lightness: clampLightness(candidate.lightness + (direction * CONTRAST_LIGHTNESS_STEP)), + }; + } + return oklchToRgb(candidate); +} + +function clampLightness(value: number): number { + return Math.min(LIGHTNESS_CEILING, Math.max(LIGHTNESS_FLOOR, value)); +} + +function averageLuminance(colors: readonly RgbTuple[]): number { + const total = colors.reduce((sum, color) => sum + relativeLuminance(color), 0); + return total / colors.length; +} + +function passesContrast( + color: RgbTuple, + backgrounds: readonly RgbTuple[], + minContrastRatio: number, +): boolean { + return backgrounds.every( + (background) => contrastRatio(color, background) >= minContrastRatio, + ); +} + +export function contrastRatio(foreground: RgbTuple, background: RgbTuple): number { + const foregroundLuminance = relativeLuminance(foreground); + const backgroundLuminance = relativeLuminance(background); + const lighter = Math.max(foregroundLuminance, backgroundLuminance); + const darker = Math.min(foregroundLuminance, backgroundLuminance); + return (lighter + CONTRAST_LUMINANCE_OFFSET) / (darker + CONTRAST_LUMINANCE_OFFSET); +} + +function relativeLuminance(color: RgbTuple): number { + const red = linearizedColorChannel(color[0]); + const green = linearizedColorChannel(color[1]); + const blue = linearizedColorChannel(color[2]); + return red * LUMINANCE_RED_WEIGHT + + green * LUMINANCE_GREEN_WEIGHT + + blue * LUMINANCE_BLUE_WEIGHT; +} + +function linearizedColorChannel(channel: number): number { + const normalized = channel / COLOR_CHANNEL_MAX; + return normalized <= SRGB_LINEAR_THRESHOLD + ? normalized / SRGB_LINEAR_DIVISOR + : ((normalized + SRGB_OFFSET) / SRGB_SCALE) ** SRGB_EXPONENT; +} From b33edd7feb1cc9a67d065638300121db00ba47cb Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 7 Sep 2026 18:43:58 -0700 Subject: [PATCH 2/6] Fix: search both directions when correcting for contrast Review finding on this PR, and a real defect. legibleOn picked its search direction from whether the colour was lighter or darker than the average background, then walked only that way. That can commit to a dead end: a token at relative luminance 0.05 on a 0.10 surface tops out at a 3.00 ratio even at pure black, while walking the other way reaches 7.00. Asked for 4.5, the old code returned black at 3.00 and reported nothing wrong. No built-in theme hit this -- every generated companion's worst-token ratio is byte-identical before and after (monokai-light 3.03, dracula-light 3.08, graphite-light 3.83, nord-light 3.33, catppuccin-light 4.36, morning-dark 5.57). It would have bitten the first authored palette whose surfaces sat in the middle of the range. Both directions are now searched one step at a time, so whichever clears the floor first wins and the answer is also the smallest lightness change that works. A colour already clearing the floor returns untouched before any search. When neither side can clear it -- surfaces too close together to admit any passing colour -- the end with the most available contrast is returned as an honest best effort rather than a silent stop mid-walk. Two specs pin it: the 0.05-on-0.10 dead end now reaches 4.5, and a token just below a mid grey surface darkens rather than jumping across to the light side. Also extracts the OKLCH coefficients into four named matrices with a shared multiply, per the other review finding. Note the two directions cannot share coefficients as the review suggested -- they are inverse matrices, not the same values reused -- so each is transcribed rather than derived from its partner; inverting at runtime would introduce error the published values do not have. The reference-value and full-cube round-trip specs both still pass, which is what proves the refactor preserved the maths. --- spec/companion-theme-contrast.spec.mjs | 56 +++++++++++++++++++ src/ui/oklch.ts | 75 +++++++++++++++++++------- src/ui/theme-contrast.ts | 64 ++++++++++++++++------ 3 files changed, 161 insertions(+), 34 deletions(-) diff --git a/spec/companion-theme-contrast.spec.mjs b/spec/companion-theme-contrast.spec.mjs index 1c4e398e..c26f0282 100644 --- a/spec/companion-theme-contrast.spec.mjs +++ b/spec/companion-theme-contrast.spec.mjs @@ -131,3 +131,59 @@ test("reaching the contrast floor does not wash a colour out to grey", async () assert.deepEqual(offenders, []); }); + +test("the correction searches both directions, not just away from the surface", async () => { + const contrast = await importDist("ui", "theme-contrast.js"); + + // A token darker than a mid-dark surface. Walking darker tops out at 3.00 + // against this surface even at pure black, so a correction that commits to + // "away from the ground" by luminance alone can never reach the 4.5 floor -- + // while walking lighter reaches 7.00 comfortably. + const surface = [89, 89, 89]; // relative luminance 0.100 + const ink = [63, 63, 63]; // relative luminance 0.050, ratio 1.50 + + const adjusted = contrast.contrastAdjustedPalette({ + ink, + muted: ink, + accent: ink, + info: ink, + warning: ink, + success: ink, + surface, + surfaceRaised: surface, + surfaceMuted: surface, + }); + + const ratio = contrastRatio(adjusted.ink, surface); + assert.ok( + ratio >= 4.5, + `ink should have been corrected to clear 4.5:1, got ${ratio.toFixed(2)} at ${adjusted.ink}`, + ); +}); + +test("the correction takes the smaller lightness change when both directions pass", async () => { + const contrast = await importDist("ui", "theme-contrast.js"); + const { rgbToOklch } = await importDist("ui", "oklch.js"); + + // Mid grey surface: both black and white clear 3:1, but a token sitting just + // below the surface should darken rather than jump across to the light side. + const surface = [128, 128, 128]; + const ink = [110, 110, 110]; + + const adjusted = contrast.contrastAdjustedPalette({ + ink: [0, 0, 0], + muted: ink, + accent: ink, + info: ink, + warning: ink, + success: ink, + surface, + surfaceRaised: surface, + surfaceMuted: surface, + }); + + assert.ok( + rgbToOklch(adjusted.accent).lightness < rgbToOklch(ink).lightness, + `expected the nearer (darker) solution, got ${adjusted.accent}`, + ); +}); diff --git a/src/ui/oklch.ts b/src/ui/oklch.ts index dec4522a..84756b09 100644 --- a/src/ui/oklch.ts +++ b/src/ui/oklch.ts @@ -18,6 +18,53 @@ export interface Oklch { export type Rgb = readonly [number, number, number]; +// A triple in whatever space the surrounding step is working in -- linear sRGB, +// cone response (LMS), or Oklab. Named apart from Rgb so a cone-response triple +// is not mistaken for a colour. +type Triple = readonly [number, number, number]; +type Matrix3 = readonly [Triple, Triple, Triple]; + +// The four matrices of Ottosson's derivation, transcribed from the reference +// implementation. The two directions are inverses of one another, not shared +// coefficients, so each is written out rather than derived from its partner -- +// inverting at runtime would introduce error the published values do not have. +// oklch.spec.mjs checks them against his reference conversions. +const LINEAR_RGB_TO_CONE: Matrix3 = [ + [0.4122214708, 0.5363325363, 0.0514459929], + [0.2119034982, 0.6806995451, 0.1073969566], + [0.0883024619, 0.2817188376, 0.6299787005], +]; + +const CONE_TO_OKLAB: Matrix3 = [ + [0.2104542553, 0.7936177850, -0.0040720468], + [1.9779984951, -2.4285922050, 0.4505937099], + [0.0259040371, 0.7827717662, -0.8086757660], +]; + +const OKLAB_TO_CONE: Matrix3 = [ + [1, 0.3963377774, 0.2158037573], + [1, -0.1055613458, -0.0638541728], + [1, -0.0894841775, -1.2914855480], +]; + +const CONE_TO_LINEAR_RGB: Matrix3 = [ + [4.0767416621, -3.3077115913, 0.2309699292], + [-1.2684380046, 2.6097574011, -0.3413193965], + [-0.0041960863, -0.7034186147, 1.7076147010], +]; + +function transform(matrix: Matrix3, input: Triple): Triple { + return [ + dot(matrix[0], input), + dot(matrix[1], input), + dot(matrix[2], input), + ]; +} + +function dot(row: Triple, input: Triple): number { + return (row[0] * input[0]) + (row[1] * input[1]) + (row[2] * input[2]); +} + const SRGB_MAX = 255; const GAMMA_THRESHOLD_ENCODED = 0.04045; const GAMMA_THRESHOLD_LINEAR = 0.0031308; @@ -48,17 +95,11 @@ function toEncoded(linear: number): number { } export function rgbToOklch(rgb: Rgb): Oklch { - const red = toLinear(rgb[0]); - const green = toLinear(rgb[1]); - const blue = toLinear(rgb[2]); - - const long = Math.cbrt((0.4122214708 * red) + (0.5363325363 * green) + (0.0514459929 * blue)); - const medium = Math.cbrt((0.2119034982 * red) + (0.6806995451 * green) + (0.1073969566 * blue)); - const short = Math.cbrt((0.0883024619 * red) + (0.2817188376 * green) + (0.6299787005 * blue)); - - const lightness = (0.2104542553 * long) + (0.7936177850 * medium) - (0.0040720468 * short); - const greenRed = (1.9779984951 * long) - (2.4285922050 * medium) + (0.4505937099 * short); - const blueYellow = (0.0259040371 * long) + (0.7827717662 * medium) - (0.8086757660 * short); + const linear: Triple = [toLinear(rgb[0]), toLinear(rgb[1]), toLinear(rgb[2])]; + const cone = transform(LINEAR_RGB_TO_CONE, linear); + // The cube root is what makes the space perceptual rather than linear. + const compressed: Triple = [Math.cbrt(cone[0]), Math.cbrt(cone[1]), Math.cbrt(cone[2])]; + const [lightness, greenRed, blueYellow] = transform(CONE_TO_OKLAB, compressed); const hue = (Math.atan2(blueYellow, greenRed) * DEGREES_PER_TURN) / (2 * Math.PI); return { @@ -73,15 +114,11 @@ export function oklchToRgb(color: Oklch): Rgb { const greenRed = color.chroma * Math.cos(radians); const blueYellow = color.chroma * Math.sin(radians); - const long = ((color.lightness + (0.3963377774 * greenRed) + (0.2158037573 * blueYellow)) ** CUBE); - const medium = ((color.lightness - (0.1055613458 * greenRed) - (0.0638541728 * blueYellow)) ** CUBE); - const short = ((color.lightness - (0.0894841775 * greenRed) - (1.2914855480 * blueYellow)) ** CUBE); + const compressed = transform(OKLAB_TO_CONE, [color.lightness, greenRed, blueYellow]); + const cone: Triple = [compressed[0] ** CUBE, compressed[1] ** CUBE, compressed[2] ** CUBE]; + const linear = transform(CONE_TO_LINEAR_RGB, cone); - return [ - toEncoded((4.0767416621 * long) - (3.3077115913 * medium) + (0.2309699292 * short)), - toEncoded((-1.2684380046 * long) + (2.6097574011 * medium) - (0.3413193965 * short)), - toEncoded((-0.0041960863 * long) - (0.7034186147 * medium) + (1.7076147010 * short)), - ]; + return [toEncoded(linear[0]), toEncoded(linear[1]), toEncoded(linear[2])]; } // Shortest way round the hue circle, so a rotation from 350 to 10 travels 20 diff --git a/src/ui/theme-contrast.ts b/src/ui/theme-contrast.ts index 5dbffcb7..4b3bf951 100644 --- a/src/ui/theme-contrast.ts +++ b/src/ui/theme-contrast.ts @@ -4,7 +4,7 @@ // question of whether a colour can actually be read on the surface behind it. import type { RgbTuple, ThemePalette } from './jedit-theme-palettes.js'; -import { oklchToRgb, rgbToOklch } from './oklch.js'; +import { oklchToRgb, rgbToOklch, type Oklch } from './oklch.js'; const COLOR_CHANNEL_MAX = 255; // WCAG AA for body text; 3:1 is the non-text and large-text floor, which is @@ -53,30 +53,64 @@ function legibleOn( backgrounds: readonly RgbTuple[], minContrastRatio: number, ): RgbTuple { - const ground = averageLuminance(backgrounds); - const direction = relativeLuminance(color) >= ground ? LIGHTER : DARKER; - let candidate = rgbToOklch(color); - for (let step = 0; step < MAX_CONTRAST_STEPS; step += 1) { - const rgb = oklchToRgb(candidate); + const origin = rgbToOklch(color); + if (passesContrast(color, backgrounds, minContrastRatio)) { + return color; + } + // Both directions are searched rather than only the one leading away from the + // background's luminance. Picking a direction from the background alone can + // choose a dead end: a token at luminance 0.05 on a 0.10 surface only reaches + // 3.0 at pure black, but 7.0 going the other way. Whichever side clears the + // floor first wins, so the answer is also the smallest change that works. + for (let step = 1; step <= MAX_CONTRAST_STEPS; step += 1) { + const offset = step * CONTRAST_LIGHTNESS_STEP; + const nearer = nearestPassing(origin, offset, backgrounds, minContrastRatio); + if (nearer != null) { + return nearer; + } + } + return furthestFrom(origin, backgrounds); +} + +function nearestPassing( + origin: Oklch, + offset: number, + backgrounds: readonly RgbTuple[], + minContrastRatio: number, +): RgbTuple | undefined { + for (const direction of [DARKER, LIGHTER]) { + const lightness = clampLightness(origin.lightness + (direction * offset)); + const rgb = oklchToRgb({ ...origin, lightness }); if (passesContrast(rgb, backgrounds, minContrastRatio)) { return rgb; } - candidate = { - ...candidate, - lightness: clampLightness(candidate.lightness + (direction * CONTRAST_LIGHTNESS_STEP)), - }; } - return oklchToRgb(candidate); + return undefined; +} + +// Nothing on either side cleared the floor, which happens when the surfaces +// themselves are too close together to admit a passing colour. Returning the +// end with the most contrast available is the honest best effort; the spec +// suite is what catches a theme this actually bites. +function furthestFrom(origin: Oklch, backgrounds: readonly RgbTuple[]): RgbTuple { + const darkest = oklchToRgb({ ...origin, lightness: LIGHTNESS_FLOOR }); + const lightest = oklchToRgb({ ...origin, lightness: LIGHTNESS_CEILING }); + return worstContrast(darkest, backgrounds) >= worstContrast(lightest, backgrounds) + ? darkest + : lightest; +} + +function worstContrast(color: RgbTuple, backgrounds: readonly RgbTuple[]): number { + return backgrounds.reduce( + (worst, background) => Math.min(worst, contrastRatio(color, background)), + Number.POSITIVE_INFINITY, + ); } function clampLightness(value: number): number { return Math.min(LIGHTNESS_CEILING, Math.max(LIGHTNESS_FLOOR, value)); } -function averageLuminance(colors: readonly RgbTuple[]): number { - const total = colors.reduce((sum, color) => sum + relativeLuminance(color), 0); - return total / colors.length; -} function passesContrast( color: RgbTuple, From a4e491ef129232b849138cc329aa92ae63ddf349 Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 7 Sep 2026 18:56:21 -0700 Subject: [PATCH 3/6] Test: pin the correction to the independently computed nearest solution Review finding on this PR. The previous assertion only checked that the adjusted accent came out darker than it started, which would also have passed for a correction that darkened all the way to black -- the exact overshoot the test was supposed to rule out. The expected landing point is now computed in the spec by scanning outward from the origin on a 0.001 grid, both directions, and taking the first lightness that clears the floor. The assertion allows one search step of slack, since the implementation samples a coarser grid and can overshoot the true optimum by at most the step it moves in. The scan derives the optimum without knowing the implementation's step size, so tuning that constant does not silently loosen the test. --- spec/companion-theme-contrast.spec.mjs | 41 +++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/spec/companion-theme-contrast.spec.mjs b/spec/companion-theme-contrast.spec.mjs index c26f0282..1c6fc1a3 100644 --- a/spec/companion-theme-contrast.spec.mjs +++ b/spec/companion-theme-contrast.spec.mjs @@ -163,12 +163,32 @@ test("the correction searches both directions, not just away from the surface", test("the correction takes the smaller lightness change when both directions pass", async () => { const contrast = await importDist("ui", "theme-contrast.js"); - const { rgbToOklch } = await importDist("ui", "oklch.js"); + const { rgbToOklch, oklchToRgb } = await importDist("ui", "oklch.js"); - // Mid grey surface: both black and white clear 3:1, but a token sitting just - // below the surface should darken rather than jump across to the light side. + // Mid grey surface: both black and white clear 3:1, so "walk away from the + // ground" is not a sufficient answer -- the correction has to pick the nearer + // side. Asserting merely "it got darker" would also pass for a correction + // that darkened all the way to black, so the expected landing point is + // computed here independently of the implementation. const surface = [128, 128, 128]; const ink = [110, 110, 110]; + const MIN_ACCENT_CONTRAST = 3; + + const origin = rgbToOklch(ink); + const passesAt = (lightness) => + contrastRatio(oklchToRgb({ ...origin, lightness }), surface) >= MIN_ACCENT_CONTRAST; + + // Finest-grain scan outward from the origin, both sides, independent of the + // implementation's step size. + const GRAIN = 0.001; + let optimalDelta; + for (let delta = 0; delta <= 1 && optimalDelta === undefined; delta += GRAIN) { + if (passesAt(Math.max(0, origin.lightness - delta)) + || passesAt(Math.min(1, origin.lightness + delta))) { + optimalDelta = delta; + } + } + assert.ok(optimalDelta !== undefined, "a passing colour should exist on this surface"); const adjusted = contrast.contrastAdjustedPalette({ ink: [0, 0, 0], @@ -182,8 +202,21 @@ test("the correction takes the smaller lightness change when both directions pas surfaceMuted: surface, }); + const actualDelta = Math.abs(rgbToOklch(adjusted.accent).lightness - origin.lightness); + + assert.ok( + contrastRatio(adjusted.accent, surface) >= MIN_ACCENT_CONTRAST, + `accent should clear ${MIN_ACCENT_CONTRAST}:1, got ${contrastRatio(adjusted.accent, surface).toFixed(2)}`, + ); + // One search step of slack: the walk samples a grid, so it can overshoot the + // true optimum by at most the step it moves in. + const STEP_SLACK = 0.025; + assert.ok( + actualDelta <= optimalDelta + STEP_SLACK, + `expected a change near the ${optimalDelta.toFixed(3)} optimum, moved ${actualDelta.toFixed(3)}`, + ); assert.ok( - rgbToOklch(adjusted.accent).lightness < rgbToOklch(ink).lightness, + rgbToOklch(adjusted.accent).lightness < origin.lightness, `expected the nearer (darker) solution, got ${adjusted.accent}`, ); }); From 5ecfff8253cda4e7e7729bbb3b91bf61dc5d481d Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 7 Sep 2026 19:06:07 -0700 Subject: [PATCH 4/6] Fix: hold companion tokens legible on every surface they render on Review finding from Codex on this PR, and correct. The correction checked accents and syntax tokens against palette.surface alone, but a token is drawn on more than the workspace background: source-viewer keeps the syntax foreground while the current line repaints the background beneath it, and the settings drawer reuses the comment foreground over the drawer background. So the floor was being met on a surface the token does not always appear on. monokai's light companion reported 3.03:1 on the workspace while rendering at 2.75:1 on the current line and 2.51:1 in the drawer. Both under the 3:1 floor this PR exists to enforce, and both invisible to the spec that shipped with it. Every token is now held against every surface in the palette. Worst ratio across workspace, current line, drawer, header and footer, per companion: graphite-light 3.34 morning-dark 4.42 monokai-light 3.06 dracula-light 3.11 nord-light 3.07 catppuccin-light 3.15 The new spec walks all five rendering surfaces rather than the workspace background only, so a token that clears the floor in one place and fails in another is now a failure rather than a pass. --- spec/companion-theme-contrast.spec.mjs | 42 ++++++++++++++++++++++++++ src/ui/theme-contrast.ts | 20 +++++++----- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/spec/companion-theme-contrast.spec.mjs b/spec/companion-theme-contrast.spec.mjs index 1c6fc1a3..abdab9ad 100644 --- a/spec/companion-theme-contrast.spec.mjs +++ b/spec/companion-theme-contrast.spec.mjs @@ -49,6 +49,48 @@ test("generated companion themes exist to be checked", async () => { assert.ok(companions.length > 0, "expected at least one generated companion theme"); }); +// A syntax foreground keeps its colour when the current line repaints the +// background beneath it, and the settings drawer reuses the comment foreground +// over the drawer background. Checking only the workspace background passed +// tokens that were then rendered somewhere darker: monokai's light companion +// reported 3.03:1 on the workspace while sitting at 2.75:1 on the current line +// and 2.51:1 in the drawer. +function renderingSurfaces(theme) { + return [ + ["workspace", theme.surface.workspace.bgRGB], + ["currentLine", theme.surface.currentLine.bgRGB], + ["drawer", theme.surface.drawer.bgRGB], + ["header", theme.surface.header.bgRGB], + ["footer", theme.surface.footer.bgRGB], + ]; +} + +test("every generated companion keeps its tokens legible on every surface it renders on", async () => { + const offenders = []; + + for (const companion of await generatedCompanions()) { + for (const [name, background] of renderingSurfaces(companion)) { + for (const [token, style] of companion.source) { + const ratio = contrastRatio(style.fgRGB, background); + if (ratio < MIN_TOKEN_CONTRAST) { + offenders.push(`${companion.name} ${String(token)} on ${name} ${ratio.toFixed(2)}`); + } + } + for (const [token, style] of companion.markdown) { + if (style.fgRGB == null) { + continue; + } + const ratio = contrastRatio(style.fgRGB, background); + if (ratio < MIN_TOKEN_CONTRAST) { + offenders.push(`${companion.name} ${String(token)} on ${name} ${ratio.toFixed(2)}`); + } + } + } + } + + assert.deepEqual(offenders, []); +}); + test("every generated companion keeps its syntax tokens legible", async () => { const offenders = []; diff --git a/src/ui/theme-contrast.ts b/src/ui/theme-contrast.ts index 4b3bf951..d6054268 100644 --- a/src/ui/theme-contrast.ts +++ b/src/ui/theme-contrast.ts @@ -27,16 +27,22 @@ const LIGHTNESS_CEILING = 1; const DARKER = -1; const LIGHTER = 1; -export function contrastAdjustedPalette(palette: ThemePalette): ThemePalette { +export // Every token is held against every surface it can be drawn on, not just the +// workspace background. A syntax foreground keeps its colour when the current +// line repaints the background beneath it, and the settings drawer reuses the +// comment foreground over the drawer background, so checking only +// palette.surface passed tokens that were then rendered somewhere darker -- +// monokai's light companion sat at 2.75:1 on the current line and 2.51:1 in +// the drawer while reporting 3.03:1 on the workspace. +function contrastAdjustedPalette(palette: ThemePalette): ThemePalette { const surfaces = [palette.surface, palette.surfaceRaised, palette.surfaceMuted]; - const raised = [palette.surface, palette.surfaceRaised]; return { ink: legibleOn(palette.ink, surfaces, MIN_SURFACE_TEXT_CONTRAST_RATIO), - muted: legibleOn(palette.muted, [palette.surface], MIN_ACCENT_CONTRAST_RATIO), - accent: legibleOn(palette.accent, [palette.surface], MIN_ACCENT_CONTRAST_RATIO), - info: legibleOn(palette.info, raised, MIN_ACCENT_CONTRAST_RATIO), - warning: legibleOn(palette.warning, raised, MIN_ACCENT_CONTRAST_RATIO), - success: legibleOn(palette.success, [palette.surface], MIN_ACCENT_CONTRAST_RATIO), + muted: legibleOn(palette.muted, surfaces, MIN_ACCENT_CONTRAST_RATIO), + accent: legibleOn(palette.accent, surfaces, MIN_ACCENT_CONTRAST_RATIO), + info: legibleOn(palette.info, surfaces, MIN_ACCENT_CONTRAST_RATIO), + warning: legibleOn(palette.warning, surfaces, MIN_ACCENT_CONTRAST_RATIO), + success: legibleOn(palette.success, surfaces, MIN_ACCENT_CONTRAST_RATIO), surface: palette.surface, surfaceRaised: palette.surfaceRaised, surfaceMuted: palette.surfaceMuted, From f44d884f072c5fba53f4423fdbb559c2c72a014a Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 7 Sep 2026 19:17:47 -0700 Subject: [PATCH 5/6] Fix: load meshes on demand, keep the frame clock honest, stop the material lie Three findings from Codex on this PR. All three were real. P1: no built-in scene could be opened. Startup deliberately loads no title geometry, so the model carries an empty mesh library and the scene picker hands that same empty library to the loader. Every scene that references bunny, teapot or dragon then failed to decode -- including continuum-gate, the one the picker offers first. The snapshot comment already promised these "load on demand"; that half was never wired. The loader now fills the gaps itself from a memoised library, parsed at most once and only when a scene is actually opened, so the startup saving that emptied the library is kept. The caller's library still wins, so a scene opened with an explicitly supplied mesh keeps it and only gaps are filled. Note three scenes -- neon-orbit, mirror-hall, aurora-vault -- still fail to decode. They declare "kind": "mesh" with no mesh field at all, and they fail identically against a fully loaded library, so that is a defect in the scene data and not in loading. Filed separately. The spec asserts specifically that no scene fails for want of an unloaded mesh, so it stays pointed at this bug rather than silently passing if a new scene breaks loading. P2: the frame clock billed idle time as one frame. The idle gate returns the model untouched so nothing re-renders, which also stops lastFrameMs advancing. Whatever switched animation back on -- perf overlay, profiler, legacy backdrop -- then handed the first active frame the entire idle interval as its duration. Measured: 30016 ms as a single frame after a 30 second idle. That is instantly over budget, which trips the backdrop's low-rate flag and leaves the animation frozen from then on, and it corrupts the first profiler record. The baseline is reset on the inactive-to-active edge, not on every idle tick, because advancing it during idle would mean returning a new model and defeating the render gate that made the workspace idle in the first place. P2: pressing m with no scene loaded switched the ray-traced backdrop on and toasted that a material preset had been applied. Nothing was applied -- the generated backdrop takes its materials from the theme and never reads titleMeshMaterialIndex -- so it reported a change that could not have happened, and started an animation loop to do it. It now says a scene must be loaded and leaves the backdrop alone. Verified by mutation: restoring the old behaviour fails both new cases. One import slot was needed for the on-demand mesh library; node:fs already exposes the promises API, so the two fs imports in the scene loader became one rather than the file taking on tracked debt. --- spec/scene-picker-mesh-loading.spec.mjs | 74 ++++++++++++++++ spec/title-material-key.spec.mjs | 59 +++++++++++++ spec/workspace-frame-clock.spec.mjs | 84 +++++++++++++++++++ src/adapters/title-scene-loader.ts | 15 ++-- src/adapters/workspace-title-meshes.ts | 17 ++++ src/app/workspace/runtime.ts | 26 +++++- .../workspace/title-screen-key-bindings.ts | 16 ++-- 7 files changed, 279 insertions(+), 12 deletions(-) create mode 100644 spec/scene-picker-mesh-loading.spec.mjs create mode 100644 spec/title-material-key.spec.mjs create mode 100644 spec/workspace-frame-clock.spec.mjs diff --git a/spec/scene-picker-mesh-loading.spec.mjs b/spec/scene-picker-mesh-loading.spec.mjs new file mode 100644 index 00000000..1310c5ec --- /dev/null +++ b/spec/scene-picker-mesh-loading.spec.mjs @@ -0,0 +1,74 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { importDist } from "./dist-helpers.mjs"; + +// Startup deliberately loads no title geometry, so the model carries an empty +// mesh library. The scene picker hands that same empty library to the loader, +// which means every built-in scene referencing bunny, teapot or dragon threw +// "mesh is not loaded" and could not be opened at all. The loader fills the +// gaps itself, on demand, so the startup saving is kept without breaking the +// scenes it was meant to leave working. + +const EMPTY_LIBRARY = Object.freeze({}); + +// neon-orbit, mirror-hall and aurora-vault declare "kind": "mesh" with no mesh +// field at all. They fail identically with a fully loaded library, so that is a +// defect in the scene data rather than anything to do with on-demand loading -- +// tracked separately. Asserting on the error text keeps this spec pointed at +// the loading bug: if one of those scenes were ever fixed, or a new scene broke +// mesh loading, this notices. +const MESH_NOT_LOADED = /mesh asset is not loaded/; + +test("no built-in scene fails for want of an unloaded mesh", async () => { + const [loader, port] = await Promise.all([ + importDist("adapters", "title-scene-loader.js"), + importDist("ports", "title-scene-loader.js"), + ]); + const scenePort = loader.createTitleSceneLoaderPort(); + const unloaded = []; + let decoded = 0; + + for (const name of port.BUILT_IN_TITLE_SCENE_NAMES) { + try { + const scene = await scenePort.loadBuiltInTitleScene(name, EMPTY_LIBRARY); + assert.ok(Array.isArray(scene.objects), `${name} produced no objects`); + decoded += 1; + } catch (error) { + if (MESH_NOT_LOADED.test(error.message)) { + unloaded.push(`${name}: ${error.message}`); + } + } + } + + assert.deepEqual(unloaded, []); + assert.ok(decoded >= 12, `expected most scenes to decode, only ${decoded} did`); +}); + +test("the default scene opens from an empty mesh library", async () => { + const [loader, port] = await Promise.all([ + importDist("adapters", "title-scene-loader.js"), + importDist("ports", "title-scene-loader.js"), + ]); + const scenePort = loader.createTitleSceneLoaderPort(); + + // The picker offers this one first, so it was the most likely thing a reader + // would try and the most visible instance of the failure. + const scene = await scenePort.loadBuiltInTitleScene( + port.DEFAULT_BUILT_IN_TITLE_SCENE_NAME, + EMPTY_LIBRARY, + ); + + assert.ok(scene.objects.length > 0); +}); + +test("a caller-supplied mesh still wins over the on-demand one", async () => { + const loader = await importDist("adapters", "title-scene-loader.js"); + const scenePort = loader.createTitleSceneLoaderPort(); + + const marker = { vertices: [], triangles: [], marker: "caller" }; + const scene = await scenePort.loadBuiltInTitleScene("bunny.jedit-scene", { + bunny: marker, + }); + + assert.ok(scene.objects.length > 0); +}); diff --git a/spec/title-material-key.spec.mjs b/spec/title-material-key.spec.mjs new file mode 100644 index 00000000..7b904401 --- /dev/null +++ b/spec/title-material-key.spec.mjs @@ -0,0 +1,59 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { importDist } from "./dist-helpers.mjs"; +import { mockKeyBindingContext, mockTitleScreenModel } from "./workspace-helpers.mjs"; + +// Pressing `m` with no scene loaded used to switch the ray-traced backdrop on +// and toast that a material preset had been applied. Nothing was applied: the +// generated backdrop derives its materials from the theme and never reads +// titleMeshMaterialIndex. It reported a change that could not have happened, +// and switched on an animation loop to do it. + +const MATERIAL_KEY = { key: "m", ctrl: false, alt: false, shift: false }; + +test("the material key does not claim success with no scene loaded", async () => { + const [keys, titleScreen] = await Promise.all([ + importDist("app", "workspace", "title-screen-key-bindings.js"), + importDist("ui", "title-screen.js"), + ]); + const model = mockTitleScreenModel(titleScreen, { sceneOverride: undefined }); + + const [next] = keys.updateTitleScreenKey(MATERIAL_KEY, model, mockKeyBindingContext()); + + assert.equal( + next.titleBackdropKind, + model.titleBackdropKind, + "the material key must not switch the legacy backdrop on", + ); + const toast = next.notifications?.items?.at(-1); + assert.ok(toast != null, "expected the reader to be told why nothing happened"); + assert.match( + `${toast.title} ${toast.message}`, + /scene/i, + `expected a message about loading a scene, got "${toast.message}"`, + ); +}); + +test("the material key still cycles presets when a scene is loaded", async () => { + const [keys, titleScreen] = await Promise.all([ + importDist("app", "workspace", "title-screen-key-bindings.js"), + importDist("ui", "title-screen.js"), + ]); + const scene = { + camera: { position: [0, 0, 1], target: [0, 0, 0], up: [0, 1, 0], fov: 60 }, + objects: [], + environment: {}, + }; + const model = mockTitleScreenModel(titleScreen, { + sceneOverride: scene, + titleMeshMaterialIndex: 0, + }); + + const [next] = keys.updateTitleScreenKey(MATERIAL_KEY, model, mockKeyBindingContext()); + + assert.notEqual( + next.titleMeshMaterialIndex, + model.titleMeshMaterialIndex, + "a loaded scene should still cycle the preset", + ); +}); diff --git a/spec/workspace-frame-clock.spec.mjs b/spec/workspace-frame-clock.spec.mjs new file mode 100644 index 00000000..9edd2a04 --- /dev/null +++ b/spec/workspace-frame-clock.spec.mjs @@ -0,0 +1,84 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { importDist } from "./dist-helpers.mjs"; +import { mockRuntime } from "./workspace-helpers.mjs"; + +function idleWorkspaceModel(titleScreen) { + return { + time: 0, + lastFrameMs: 0, + frameTimeMs: 0, + frameTimeHistory: [], + startupIntroComplete: true, + perfVisible: false, + profiler: { active: false }, + titleBackdropKind: titleScreen.TITLE_BACKDROP_KIND.StaticLogo, + }; +} + +// The idle gate returns the same model so nothing re-renders, which also means +// lastFrameMs stops advancing while the workspace sits still. If animation is +// later switched back on, the first active tick would otherwise bill the whole +// idle interval as a single frame: instantly over budget, which trips the +// backdrop's low-rate flag and leaves the animation frozen on every frame after. + +const IDLE_MS = 30_000; +const PLAUSIBLE_FRAME_MS = 100; + +async function runtimeAt(clock) { + const runtimeModule = await importDist("app", "workspace", "runtime.js"); + return runtimeModule.createWorkspaceRuntime({ + ...mockRuntime(), + nowMs: () => clock.now, + }); +} + +test("resuming animation after an idle gap starts a fresh frame clock", async () => { + const titleScreen = await importDist("ui", "title-screen.js"); + const clock = { now: 0 }; + const runtime = await runtimeAt(clock); + + const idle = { + ...idleWorkspaceModel(titleScreen), + perfVisible: false, + lastFrameMs: 0, + }; + + // Sit idle. Ticks keep arriving; the gate returns the same model each time. + clock.now = IDLE_MS; + const [stillIdle] = runtime.update({ type: "time-tick", time: 1 }, idle); + assert.equal(stillIdle, idle, "an idle tick must not produce a new model"); + + // The user switches the perf overlay on, which reactivates animation. + const [watching] = runtime.update({ type: "toggle-perf" }, stillIdle); + assert.equal(watching.perfVisible, true); + + // The next tick is the first active frame. + clock.now = IDLE_MS + 16; + const [animating] = runtime.update({ type: "time-tick", time: 2 }, watching); + + assert.ok( + animating.frameTimeMs < PLAUSIBLE_FRAME_MS, + `first resumed frame billed ${animating.frameTimeMs}ms of idle time as one frame`, + ); +}); + +test("frame timing is untouched while animation stays active", async () => { + const titleScreen = await importDist("ui", "title-screen.js"); + const clock = { now: 0 }; + const runtime = await runtimeAt(clock); + + const watching = { + ...idleWorkspaceModel(titleScreen), + perfVisible: true, + lastFrameMs: 0, + }; + + clock.now = 16; + const [first] = runtime.update({ type: "time-tick", time: 1 }, watching); + assert.equal(first.frameTimeMs, 16); + + clock.now = 33; + const [second] = runtime.update({ type: "time-tick", time: 2 }, first); + assert.equal(second.frameTimeMs, 17, "an ordinary frame must be measured normally"); +}); diff --git a/src/adapters/title-scene-loader.ts b/src/adapters/title-scene-loader.ts index 3f6a4d73..3229141f 100644 --- a/src/adapters/title-scene-loader.ts +++ b/src/adapters/title-scene-loader.ts @@ -1,5 +1,7 @@ -import * as fs from "node:fs/promises"; -import { existsSync, readFileSync } from "node:fs"; +// The promises API is reached through node:fs rather than a second +// node:fs/promises import, so the sync and async reads in this module cost +// one import between them. +import { existsSync, promises as fs, readFileSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { SceneDecodeError, SceneLoadError } from "../domain/errors.js"; @@ -15,7 +17,6 @@ import { TITLE_SCENE_DEFAULT_CAMERA_TARGET, titleSceneCameraPlacementFromPosition, } from "../ui/title-scene-camera.js"; -import type { TitleMeshLibrary } from "../ui/title-mesh-library.js"; import { titleSceneObjectFootprintCenterAt } from "../ui/title-scene-transform.js"; import { BUILT_IN_TITLE_SCENE_NAMES, @@ -24,6 +25,7 @@ import { } from "../ports/title-scene-loader.js"; import { decodeTitleSceneEnvironment } from "./title-scene-environment-decoder.js"; import { decodeSceneObject } from "./title-scene-object-decoder.js"; +import { withBuiltInTitleMeshes, type TitleMeshLibrary } from "./workspace-title-meshes.js"; import { arrayAt, objectAt, @@ -46,6 +48,7 @@ export interface TitleSceneLoaderOptions { const EMPTY_DIRECTION_LENGTH = 0; const BUILT_IN_TITLE_SCENE_SET = new Set(BUILT_IN_TITLE_SCENE_NAMES); + const BUILT_IN_SCENE_CANDIDATE_URLS = [ (name: BuiltInTitleSceneName): URL => new URL(`../scenes/${name}`, import.meta.url), @@ -70,7 +73,7 @@ export function loadBuiltInTitleSceneSync( } return parseTitleSceneText( readFileSync(resolveBuiltInTitleScenePath(name, undefined), "utf8"), - meshes, + withBuiltInTitleMeshes(meshes), ); } @@ -98,7 +101,7 @@ export async function loadBuiltInTitleScene( } return loadTitleSceneFromFile( resolveBuiltInTitleScenePath(name, undefined), - meshes, + withBuiltInTitleMeshes(meshes), ); } @@ -128,7 +131,7 @@ export function createTitleSceneLoaderPort( } return loadTitleSceneFromFile( resolveBuiltInTitleScenePath(name, options.builtInSceneDirectories), - meshes, + withBuiltInTitleMeshes(meshes), ); }, }; diff --git a/src/adapters/workspace-title-meshes.ts b/src/adapters/workspace-title-meshes.ts index 00fbbc5b..8b654bf4 100644 --- a/src/adapters/workspace-title-meshes.ts +++ b/src/adapters/workspace-title-meshes.ts @@ -16,6 +16,23 @@ import { loadTitleTeapotMeshSource, } from "./title-bunny-mesh.js"; +export type { TitleMeshLibrary }; + +// Startup loads no title geometry, so the workspace model carries an empty mesh +// library and hands it straight to the scene loader. Every built-in scene +// references bunny, teapot or dragon, so without this they all failed to +// decode -- including continuum-gate, the one the picker offers first. The +// meshes are parsed at most once, and only when a scene is actually opened, +// which is what preserves the startup saving that emptied the library. +let onDemandMeshes: TitleMeshLibrary | undefined; + +export function withBuiltInTitleMeshes(meshes: TitleMeshLibrary): TitleMeshLibrary { + onDemandMeshes ??= loadStartupTitleMeshes(); + // The caller's library wins, so a scene opened with an explicitly supplied + // mesh keeps it and only the gaps are filled. + return { ...onDemandMeshes, ...meshes }; +} + export function loadStartupTitleMeshes(): TitleMeshLibrary { return { bunny: loadStartupTitleMesh( diff --git a/src/app/workspace/runtime.ts b/src/app/workspace/runtime.ts index 256981bd..1b8d087c 100644 --- a/src/app/workspace/runtime.ts +++ b/src/app/workspace/runtime.ts @@ -89,7 +89,31 @@ function updateWorkspaceRuntime( msg: WorkspaceRuntimeMsg, model: WorkspaceModel, ): WorkspaceRuntimeResult { - return syncWorkspaceRuntimeResult(updateWorkspaceRuntimeState(deps, msg, model)); + return syncWorkspaceRuntimeResult( + rebaseFrameClock(deps, model, updateWorkspaceRuntimeState(deps, msg, model)), + ); +} + +// While the workspace is idle the tick handler returns the model untouched, so +// lastFrameMs stops advancing along with everything else. Whatever switches +// animation back on -- the perf overlay, the profiler, the legacy backdrop -- +// would otherwise hand the first active frame the whole idle interval as its +// duration: instantly over budget, which trips the backdrop's low-rate flag and +// leaves the animation frozen from then on. +// +// The baseline is reset on the inactive-to-active edge rather than on every +// idle tick, because advancing it during idle would mean returning a new model +// and defeating the render gate that made the workspace idle in the first place. +function rebaseFrameClock( + deps: WorkspaceRuntimeDependencies, + previous: WorkspaceModel, + result: WorkspaceRuntimeResult, +): WorkspaceRuntimeResult { + const [next, commands] = result; + if (workspaceAnimationIsActive(previous) || !workspaceAnimationIsActive(next)) { + return result; + } + return [{ ...next, lastFrameMs: deps.nowMs() }, commands]; } function updateWorkspaceRuntimeState( diff --git a/src/app/workspace/title-screen-key-bindings.ts b/src/app/workspace/title-screen-key-bindings.ts index c0e8e04a..6af91384 100644 --- a/src/app/workspace/title-screen-key-bindings.ts +++ b/src/app/workspace/title-screen-key-bindings.ts @@ -26,6 +26,8 @@ import { WorkspaceKeys } from "./workspace-key.js"; const TITLE_SHADER_TOAST_TITLE = "Title shader"; const TITLE_ASCII_PALETTE_TOAST_TITLE = "ASCII palette"; +const TITLE_MESH_MATERIAL_NEEDS_SCENE = + "Load a scene first (ctrl+l) -- the generated backdrop takes its materials from the theme."; const TITLE_MESH_MATERIAL_TOAST_TITLE = "Title material"; const TITLE_SHADER_BRAILLE_LABEL = "Braille"; const TITLE_SHADER_ASCII_LABEL = "ASCII"; @@ -120,15 +122,19 @@ function updateTitleMeshMaterialKey( model.titleMeshMaterialIndex, ); const preset = titleMeshMaterialPresetAt(titleMeshMaterialIndex); - const sceneOverride = - model.sceneOverride == null - ? undefined - : applyTitleMeshMaterial(model.sceneOverride, preset); + // With no scene loaded there is nothing to apply a material to: the generated + // backdrop takes its materials from the theme and never reads + // titleMeshMaterialIndex. Cycling the index anyway used to switch the legacy + // backdrop on and toast that the preset had been applied, so the reader was + // told a change had happened that could not have happened. + if (model.sceneOverride == null) { + return pushTitleMeshMaterialToast(model, TITLE_MESH_MATERIAL_NEEDS_SCENE, context); + } return pushTitleMeshMaterialToast( activateLegacyTitleBackdrop({ ...model, titleMeshMaterialIndex, - ...(sceneOverride == null ? {} : { sceneOverride }), + sceneOverride: applyTitleMeshMaterial(model.sceneOverride, preset), }), preset.name, context, From 5e6d30abeea69a66e29c5ca4457673c440ba900f Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 7 Sep 2026 19:23:05 -0700 Subject: [PATCH 6/6] Revert "Fix: load meshes on demand, keep the frame clock honest, stop the material lie" This reverts commit f44d884f072c5fba53f4423fdbb559c2c72a014a. --- spec/scene-picker-mesh-loading.spec.mjs | 74 ---------------- spec/title-material-key.spec.mjs | 59 ------------- spec/workspace-frame-clock.spec.mjs | 84 ------------------- src/adapters/title-scene-loader.ts | 15 ++-- src/adapters/workspace-title-meshes.ts | 17 ---- src/app/workspace/runtime.ts | 26 +----- .../workspace/title-screen-key-bindings.ts | 16 ++-- 7 files changed, 12 insertions(+), 279 deletions(-) delete mode 100644 spec/scene-picker-mesh-loading.spec.mjs delete mode 100644 spec/title-material-key.spec.mjs delete mode 100644 spec/workspace-frame-clock.spec.mjs diff --git a/spec/scene-picker-mesh-loading.spec.mjs b/spec/scene-picker-mesh-loading.spec.mjs deleted file mode 100644 index 1310c5ec..00000000 --- a/spec/scene-picker-mesh-loading.spec.mjs +++ /dev/null @@ -1,74 +0,0 @@ -import assert from "node:assert/strict"; -import test from "node:test"; -import { importDist } from "./dist-helpers.mjs"; - -// Startup deliberately loads no title geometry, so the model carries an empty -// mesh library. The scene picker hands that same empty library to the loader, -// which means every built-in scene referencing bunny, teapot or dragon threw -// "mesh is not loaded" and could not be opened at all. The loader fills the -// gaps itself, on demand, so the startup saving is kept without breaking the -// scenes it was meant to leave working. - -const EMPTY_LIBRARY = Object.freeze({}); - -// neon-orbit, mirror-hall and aurora-vault declare "kind": "mesh" with no mesh -// field at all. They fail identically with a fully loaded library, so that is a -// defect in the scene data rather than anything to do with on-demand loading -- -// tracked separately. Asserting on the error text keeps this spec pointed at -// the loading bug: if one of those scenes were ever fixed, or a new scene broke -// mesh loading, this notices. -const MESH_NOT_LOADED = /mesh asset is not loaded/; - -test("no built-in scene fails for want of an unloaded mesh", async () => { - const [loader, port] = await Promise.all([ - importDist("adapters", "title-scene-loader.js"), - importDist("ports", "title-scene-loader.js"), - ]); - const scenePort = loader.createTitleSceneLoaderPort(); - const unloaded = []; - let decoded = 0; - - for (const name of port.BUILT_IN_TITLE_SCENE_NAMES) { - try { - const scene = await scenePort.loadBuiltInTitleScene(name, EMPTY_LIBRARY); - assert.ok(Array.isArray(scene.objects), `${name} produced no objects`); - decoded += 1; - } catch (error) { - if (MESH_NOT_LOADED.test(error.message)) { - unloaded.push(`${name}: ${error.message}`); - } - } - } - - assert.deepEqual(unloaded, []); - assert.ok(decoded >= 12, `expected most scenes to decode, only ${decoded} did`); -}); - -test("the default scene opens from an empty mesh library", async () => { - const [loader, port] = await Promise.all([ - importDist("adapters", "title-scene-loader.js"), - importDist("ports", "title-scene-loader.js"), - ]); - const scenePort = loader.createTitleSceneLoaderPort(); - - // The picker offers this one first, so it was the most likely thing a reader - // would try and the most visible instance of the failure. - const scene = await scenePort.loadBuiltInTitleScene( - port.DEFAULT_BUILT_IN_TITLE_SCENE_NAME, - EMPTY_LIBRARY, - ); - - assert.ok(scene.objects.length > 0); -}); - -test("a caller-supplied mesh still wins over the on-demand one", async () => { - const loader = await importDist("adapters", "title-scene-loader.js"); - const scenePort = loader.createTitleSceneLoaderPort(); - - const marker = { vertices: [], triangles: [], marker: "caller" }; - const scene = await scenePort.loadBuiltInTitleScene("bunny.jedit-scene", { - bunny: marker, - }); - - assert.ok(scene.objects.length > 0); -}); diff --git a/spec/title-material-key.spec.mjs b/spec/title-material-key.spec.mjs deleted file mode 100644 index 7b904401..00000000 --- a/spec/title-material-key.spec.mjs +++ /dev/null @@ -1,59 +0,0 @@ -import assert from "node:assert/strict"; -import test from "node:test"; -import { importDist } from "./dist-helpers.mjs"; -import { mockKeyBindingContext, mockTitleScreenModel } from "./workspace-helpers.mjs"; - -// Pressing `m` with no scene loaded used to switch the ray-traced backdrop on -// and toast that a material preset had been applied. Nothing was applied: the -// generated backdrop derives its materials from the theme and never reads -// titleMeshMaterialIndex. It reported a change that could not have happened, -// and switched on an animation loop to do it. - -const MATERIAL_KEY = { key: "m", ctrl: false, alt: false, shift: false }; - -test("the material key does not claim success with no scene loaded", async () => { - const [keys, titleScreen] = await Promise.all([ - importDist("app", "workspace", "title-screen-key-bindings.js"), - importDist("ui", "title-screen.js"), - ]); - const model = mockTitleScreenModel(titleScreen, { sceneOverride: undefined }); - - const [next] = keys.updateTitleScreenKey(MATERIAL_KEY, model, mockKeyBindingContext()); - - assert.equal( - next.titleBackdropKind, - model.titleBackdropKind, - "the material key must not switch the legacy backdrop on", - ); - const toast = next.notifications?.items?.at(-1); - assert.ok(toast != null, "expected the reader to be told why nothing happened"); - assert.match( - `${toast.title} ${toast.message}`, - /scene/i, - `expected a message about loading a scene, got "${toast.message}"`, - ); -}); - -test("the material key still cycles presets when a scene is loaded", async () => { - const [keys, titleScreen] = await Promise.all([ - importDist("app", "workspace", "title-screen-key-bindings.js"), - importDist("ui", "title-screen.js"), - ]); - const scene = { - camera: { position: [0, 0, 1], target: [0, 0, 0], up: [0, 1, 0], fov: 60 }, - objects: [], - environment: {}, - }; - const model = mockTitleScreenModel(titleScreen, { - sceneOverride: scene, - titleMeshMaterialIndex: 0, - }); - - const [next] = keys.updateTitleScreenKey(MATERIAL_KEY, model, mockKeyBindingContext()); - - assert.notEqual( - next.titleMeshMaterialIndex, - model.titleMeshMaterialIndex, - "a loaded scene should still cycle the preset", - ); -}); diff --git a/spec/workspace-frame-clock.spec.mjs b/spec/workspace-frame-clock.spec.mjs deleted file mode 100644 index 9edd2a04..00000000 --- a/spec/workspace-frame-clock.spec.mjs +++ /dev/null @@ -1,84 +0,0 @@ -import assert from "node:assert/strict"; -import test from "node:test"; -import { importDist } from "./dist-helpers.mjs"; -import { mockRuntime } from "./workspace-helpers.mjs"; - -function idleWorkspaceModel(titleScreen) { - return { - time: 0, - lastFrameMs: 0, - frameTimeMs: 0, - frameTimeHistory: [], - startupIntroComplete: true, - perfVisible: false, - profiler: { active: false }, - titleBackdropKind: titleScreen.TITLE_BACKDROP_KIND.StaticLogo, - }; -} - -// The idle gate returns the same model so nothing re-renders, which also means -// lastFrameMs stops advancing while the workspace sits still. If animation is -// later switched back on, the first active tick would otherwise bill the whole -// idle interval as a single frame: instantly over budget, which trips the -// backdrop's low-rate flag and leaves the animation frozen on every frame after. - -const IDLE_MS = 30_000; -const PLAUSIBLE_FRAME_MS = 100; - -async function runtimeAt(clock) { - const runtimeModule = await importDist("app", "workspace", "runtime.js"); - return runtimeModule.createWorkspaceRuntime({ - ...mockRuntime(), - nowMs: () => clock.now, - }); -} - -test("resuming animation after an idle gap starts a fresh frame clock", async () => { - const titleScreen = await importDist("ui", "title-screen.js"); - const clock = { now: 0 }; - const runtime = await runtimeAt(clock); - - const idle = { - ...idleWorkspaceModel(titleScreen), - perfVisible: false, - lastFrameMs: 0, - }; - - // Sit idle. Ticks keep arriving; the gate returns the same model each time. - clock.now = IDLE_MS; - const [stillIdle] = runtime.update({ type: "time-tick", time: 1 }, idle); - assert.equal(stillIdle, idle, "an idle tick must not produce a new model"); - - // The user switches the perf overlay on, which reactivates animation. - const [watching] = runtime.update({ type: "toggle-perf" }, stillIdle); - assert.equal(watching.perfVisible, true); - - // The next tick is the first active frame. - clock.now = IDLE_MS + 16; - const [animating] = runtime.update({ type: "time-tick", time: 2 }, watching); - - assert.ok( - animating.frameTimeMs < PLAUSIBLE_FRAME_MS, - `first resumed frame billed ${animating.frameTimeMs}ms of idle time as one frame`, - ); -}); - -test("frame timing is untouched while animation stays active", async () => { - const titleScreen = await importDist("ui", "title-screen.js"); - const clock = { now: 0 }; - const runtime = await runtimeAt(clock); - - const watching = { - ...idleWorkspaceModel(titleScreen), - perfVisible: true, - lastFrameMs: 0, - }; - - clock.now = 16; - const [first] = runtime.update({ type: "time-tick", time: 1 }, watching); - assert.equal(first.frameTimeMs, 16); - - clock.now = 33; - const [second] = runtime.update({ type: "time-tick", time: 2 }, first); - assert.equal(second.frameTimeMs, 17, "an ordinary frame must be measured normally"); -}); diff --git a/src/adapters/title-scene-loader.ts b/src/adapters/title-scene-loader.ts index 3229141f..3f6a4d73 100644 --- a/src/adapters/title-scene-loader.ts +++ b/src/adapters/title-scene-loader.ts @@ -1,7 +1,5 @@ -// The promises API is reached through node:fs rather than a second -// node:fs/promises import, so the sync and async reads in this module cost -// one import between them. -import { existsSync, promises as fs, readFileSync } from "node:fs"; +import * as fs from "node:fs/promises"; +import { existsSync, readFileSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { SceneDecodeError, SceneLoadError } from "../domain/errors.js"; @@ -17,6 +15,7 @@ import { TITLE_SCENE_DEFAULT_CAMERA_TARGET, titleSceneCameraPlacementFromPosition, } from "../ui/title-scene-camera.js"; +import type { TitleMeshLibrary } from "../ui/title-mesh-library.js"; import { titleSceneObjectFootprintCenterAt } from "../ui/title-scene-transform.js"; import { BUILT_IN_TITLE_SCENE_NAMES, @@ -25,7 +24,6 @@ import { } from "../ports/title-scene-loader.js"; import { decodeTitleSceneEnvironment } from "./title-scene-environment-decoder.js"; import { decodeSceneObject } from "./title-scene-object-decoder.js"; -import { withBuiltInTitleMeshes, type TitleMeshLibrary } from "./workspace-title-meshes.js"; import { arrayAt, objectAt, @@ -48,7 +46,6 @@ export interface TitleSceneLoaderOptions { const EMPTY_DIRECTION_LENGTH = 0; const BUILT_IN_TITLE_SCENE_SET = new Set(BUILT_IN_TITLE_SCENE_NAMES); - const BUILT_IN_SCENE_CANDIDATE_URLS = [ (name: BuiltInTitleSceneName): URL => new URL(`../scenes/${name}`, import.meta.url), @@ -73,7 +70,7 @@ export function loadBuiltInTitleSceneSync( } return parseTitleSceneText( readFileSync(resolveBuiltInTitleScenePath(name, undefined), "utf8"), - withBuiltInTitleMeshes(meshes), + meshes, ); } @@ -101,7 +98,7 @@ export async function loadBuiltInTitleScene( } return loadTitleSceneFromFile( resolveBuiltInTitleScenePath(name, undefined), - withBuiltInTitleMeshes(meshes), + meshes, ); } @@ -131,7 +128,7 @@ export function createTitleSceneLoaderPort( } return loadTitleSceneFromFile( resolveBuiltInTitleScenePath(name, options.builtInSceneDirectories), - withBuiltInTitleMeshes(meshes), + meshes, ); }, }; diff --git a/src/adapters/workspace-title-meshes.ts b/src/adapters/workspace-title-meshes.ts index 8b654bf4..00fbbc5b 100644 --- a/src/adapters/workspace-title-meshes.ts +++ b/src/adapters/workspace-title-meshes.ts @@ -16,23 +16,6 @@ import { loadTitleTeapotMeshSource, } from "./title-bunny-mesh.js"; -export type { TitleMeshLibrary }; - -// Startup loads no title geometry, so the workspace model carries an empty mesh -// library and hands it straight to the scene loader. Every built-in scene -// references bunny, teapot or dragon, so without this they all failed to -// decode -- including continuum-gate, the one the picker offers first. The -// meshes are parsed at most once, and only when a scene is actually opened, -// which is what preserves the startup saving that emptied the library. -let onDemandMeshes: TitleMeshLibrary | undefined; - -export function withBuiltInTitleMeshes(meshes: TitleMeshLibrary): TitleMeshLibrary { - onDemandMeshes ??= loadStartupTitleMeshes(); - // The caller's library wins, so a scene opened with an explicitly supplied - // mesh keeps it and only the gaps are filled. - return { ...onDemandMeshes, ...meshes }; -} - export function loadStartupTitleMeshes(): TitleMeshLibrary { return { bunny: loadStartupTitleMesh( diff --git a/src/app/workspace/runtime.ts b/src/app/workspace/runtime.ts index 1b8d087c..256981bd 100644 --- a/src/app/workspace/runtime.ts +++ b/src/app/workspace/runtime.ts @@ -89,31 +89,7 @@ function updateWorkspaceRuntime( msg: WorkspaceRuntimeMsg, model: WorkspaceModel, ): WorkspaceRuntimeResult { - return syncWorkspaceRuntimeResult( - rebaseFrameClock(deps, model, updateWorkspaceRuntimeState(deps, msg, model)), - ); -} - -// While the workspace is idle the tick handler returns the model untouched, so -// lastFrameMs stops advancing along with everything else. Whatever switches -// animation back on -- the perf overlay, the profiler, the legacy backdrop -- -// would otherwise hand the first active frame the whole idle interval as its -// duration: instantly over budget, which trips the backdrop's low-rate flag and -// leaves the animation frozen from then on. -// -// The baseline is reset on the inactive-to-active edge rather than on every -// idle tick, because advancing it during idle would mean returning a new model -// and defeating the render gate that made the workspace idle in the first place. -function rebaseFrameClock( - deps: WorkspaceRuntimeDependencies, - previous: WorkspaceModel, - result: WorkspaceRuntimeResult, -): WorkspaceRuntimeResult { - const [next, commands] = result; - if (workspaceAnimationIsActive(previous) || !workspaceAnimationIsActive(next)) { - return result; - } - return [{ ...next, lastFrameMs: deps.nowMs() }, commands]; + return syncWorkspaceRuntimeResult(updateWorkspaceRuntimeState(deps, msg, model)); } function updateWorkspaceRuntimeState( diff --git a/src/app/workspace/title-screen-key-bindings.ts b/src/app/workspace/title-screen-key-bindings.ts index 6af91384..c0e8e04a 100644 --- a/src/app/workspace/title-screen-key-bindings.ts +++ b/src/app/workspace/title-screen-key-bindings.ts @@ -26,8 +26,6 @@ import { WorkspaceKeys } from "./workspace-key.js"; const TITLE_SHADER_TOAST_TITLE = "Title shader"; const TITLE_ASCII_PALETTE_TOAST_TITLE = "ASCII palette"; -const TITLE_MESH_MATERIAL_NEEDS_SCENE = - "Load a scene first (ctrl+l) -- the generated backdrop takes its materials from the theme."; const TITLE_MESH_MATERIAL_TOAST_TITLE = "Title material"; const TITLE_SHADER_BRAILLE_LABEL = "Braille"; const TITLE_SHADER_ASCII_LABEL = "ASCII"; @@ -122,19 +120,15 @@ function updateTitleMeshMaterialKey( model.titleMeshMaterialIndex, ); const preset = titleMeshMaterialPresetAt(titleMeshMaterialIndex); - // With no scene loaded there is nothing to apply a material to: the generated - // backdrop takes its materials from the theme and never reads - // titleMeshMaterialIndex. Cycling the index anyway used to switch the legacy - // backdrop on and toast that the preset had been applied, so the reader was - // told a change had happened that could not have happened. - if (model.sceneOverride == null) { - return pushTitleMeshMaterialToast(model, TITLE_MESH_MATERIAL_NEEDS_SCENE, context); - } + const sceneOverride = + model.sceneOverride == null + ? undefined + : applyTitleMeshMaterial(model.sceneOverride, preset); return pushTitleMeshMaterialToast( activateLegacyTitleBackdrop({ ...model, titleMeshMaterialIndex, - sceneOverride: applyTitleMeshMaterial(model.sceneOverride, preset), + ...(sceneOverride == null ? {} : { sceneOverride }), }), preset.name, context,