Feature/colorblind mode#4150
Open
noahschmal wants to merge 6 commits into
Open
Conversation
Adds an accessibility.colorblind flag to GraphicsOverridesSchema, a toggle in the main Settings modal (persisted via graphicsOverrides), en.json strings, and a generateRenderSettings hook where the colorblind color overrides will be applied. No rendering changes yet.
Fill the generateRenderSettings colorblind hook with an Okabe-Ito blue/orange palette: alt-view affiliation borders (self/ally blue, enemy orange) and normal-view friend/enemy border tints (friendly blue, enemy orange at a stronger ratio). Propagate the affiliation and mapOverlay slices through applyGraphicsOverrides so the renderer picks them up.
Introduce an abstract BaseTheme that owns the color allocators and the territory/team color dispatch (overridable); concrete themes supply only color data (palettes, team-color variations, terrain) via hooks. Move the hardcoded team-color switch out of ColorAllocator — now a pure distinct-pool engine — into the theme, so team colors are theme-based. Add ColorblindTheme. Light/dark render identically; team-color tests relocated onto PastelTheme.
Give the colorblind theme its own visual tuning on top of the swapped palettes: - Borders derive from each territory's fill but are darkened *relative* to the fill's own lightness (l * 0.6) rather than by a fixed amount, so every boundary reads as a consistently darker line without dark fills collapsing to near-black. - Terrain elevation bands are separated by lightness (dark plains -> mid highland -> near-white mountain) instead of the green->brown->gray hue ramp, which is hard to distinguish under red-green CVD. - Friend/foe border tint ratios raised to 0.85 so the blue/orange relationship cue dominates the darkened border instead of relying on subtle hue.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Colorblind Mode toggle and accessibility flag, applies colorblind-safe affiliation/map tints in render overrides, refactors theme infrastructure into BaseTheme and a simplified ColorAllocator, introduces ColorblindTheme and colorblind palettes, updates ThemeProvider wiring, and adjusts color-related tests. ChangesColorblind Mode Feature
Sequence DiagramsequenceDiagram
participant User
participant UserSettingModal
participant GraphicsOverrides
participant ThemeProvider
participant RenderOverrides
participant ColorblindTheme
User->>UserSettingModal: toggles Colorblind Mode
UserSettingModal->>GraphicsOverrides: setGraphicsOverrides({accessibility:{colorblind}})
User->>ThemeProvider: requests current theme
ThemeProvider->>ColorblindTheme: returns when accessibility.colorblind
GraphicsOverrides->>RenderOverrides: applyGraphicsOverrides(accessibility.colorblind)
RenderOverrides->>RenderOverrides: set Okabe–Ito affiliation/mapOverlay colors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/theme/PastelTheme.ts (1)
73-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd exhaustive check for terrain type switch.
If
TerrainTypeenum gains new values, the function could returnundefineddespite theColordreturn type. TypeScript's control-flow analysis doesn't catch this without an explicit default or exhaustive check.Suggested fix
case TerrainType.Mountain: return colord({ r: 230 + mag / 2, g: 230 + mag / 2, b: 230 + mag / 2, }); + default: { + const _exhaustive: never = gm.terrainType(tile); + return this.shore; // fallback + } } }🤖 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/PastelTheme.ts` around lines 73 - 110, The switch in terrainColor(gm: GameMap, tile: TileRef): Colord does not exhaustively handle all TerrainType values and can return undefined if the enum gains members; update the function to include an explicit exhaustive/default branch (e.g., a default case or a final throw) that either returns a safe Colord fallback or throws a descriptive error including the unexpected terrain value, so the function always returns a Colord and TypeScript's control-flow is satisfied.
🧹 Nitpick comments (3)
src/client/theme/BaseTheme.ts (2)
117-128: 💤 Low valueOrphaned comment creates confusion.
The comment on lines 126-127 appears after
break;but describes theelse ifbranch below it. Move it to the correct location.Suggested fix
if (loopCount > maxIterations) { // Prevent runaway loops console.warn(`Infinite loop detected during structure color calculation. Light color: ${colord(lightLAB).toRgbString()}, Dark color: ${colord(darkLAB).toRgbString()}, Contrast: ${contrast}`); break; - - // Increase the light color if the "loop limit" has been reach - // (probably due to the dark color already being as dark as it can be) } else if (loopCount > loopLimit) { + // Increase the light color if the loop limit has been reached + // (probably because the dark color is already as dark as it can be) lightLAB.l = this.clamp(lightLAB.l + luminanceChange);🤖 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/BaseTheme.ts` around lines 117 - 128, The orphaned comment after the break in the contrast-adjustment while loop should be moved to directly precede the else if (loopCount > loopLimit) branch so it describes that branch correctly; locate the while loop in BaseTheme.ts (the block using variables contrast, contrastTarget, loopCount, maxIterations, loopLimit and the else if (loopCount > loopLimit) clause) and cut the comment currently placed immediately after break; then paste it above the else if (loopCount > loopLimit) condition so the comment clearly explains the "Increase the light color..." behavior for loopLimit.
17-51: 🏗️ Heavy liftConsider composition over inheritance for theme architecture.
The abstract class pattern creates a three-level hierarchy (BaseTheme → PastelTheme → ColorblindTheme). A composition approach with separate palette and behavior objects would be more flexible and easier to test:
interface ThemePalettes { humanPalette: Colord[]; botPalette: Colord[]; nationPalette: Colord[]; fallbackPalette: Colord[]; teamColorVariations: (team: Team) => Colord[]; } interface ThemeBehaviors { borderColor: (territoryColor: Colord) => Colord; terrainColor: (gm: GameMap, tile: TileRef) => Colord; }This lets you mix and match palettes with behaviors without deep inheritance chains. For example, a future "dark colorblind" theme becomes trivial.
That said, the current implementation works and ships the feature. This is a good-to-have improvement for a follow-up.
🤖 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/BaseTheme.ts` around lines 17 - 51, The BaseTheme inheritance creates rigid three-level hierarchies; refactor to composition by extracting palette and behavior responsibilities into plain interfaces (e.g., ThemePalettes and ThemeBehaviors) and have a Theme implementation compose those instead of subclassing BaseTheme: move palette providers (humanPalette, botPalette, nationPalette, fallbackPalette, teamColorVariations) into a ThemePalettes object and move rendering/behavior methods (borderColor, terrainColor, spawn highlight logic that uses _spawnHighlightColor/_spawnHighlightSelfColor/_spawnHighlightTeamColor/_spawnHighlightEnemyColor and teamPlayerColors) into a ThemeBehaviors object; replace the BaseTheme constructor wiring of humanColorAllocator, botColorAllocator, nationColorAllocator to accept palettes from the injected ThemePalettes and ensure existing consumers of BaseTheme now receive a composed Theme instance that delegates to the palettes/behaviors, keeping the ColorAllocator usage and teamPlayerColors map intact for backward compatibility.tests/Colors.test.ts (1)
86-114: ⚡ Quick winConsider adding a test for ColorblindTheme color differences.
The tests verify the theme API contract through PastelTheme, which is good. Since ColorblindTheme overrides the palette methods to provide CVD-safe colors, consider adding a test that verifies ColorblindTheme returns different colors than PastelTheme for at least one team. This would confirm the colorblind theme actually applies a distinct palette.
📝 Example test to add
test("ColorblindTheme uses different colors than PastelTheme", () => { const pastel = new PastelTheme(); const colorblind = new ColorblindTheme(); const pastelBlue = pastel.teamColor(ColoredTeams.Blue); const colorblindBlue = colorblind.teamColor(ColoredTeams.Blue); expect(pastelBlue.isEqual(colorblindBlue)).toBe(false); });🤖 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 86 - 114, Add a test that instantiates PastelTheme and ColorblindTheme and asserts that ColorblindTheme.teamColor(ColoredTeams.Blue) is different from PastelTheme.teamColor(ColoredTeams.Blue) (use the Color.isEqual method and expect(...).toBe(false)); place the test alongside the existing PastelTheme tests so it verifies ColorblindTheme actually provides a distinct palette.
🤖 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/ColorblindTheme.ts`:
- Around line 76-101: The terrainColor method in ColorblindTheme does not handle
all TerrainType variants and can return undefined for new enum values; update
terrainColor (in class ColorblindTheme) to include an exhaustive/default branch
(similar to PastelTheme) that either returns a safe fallback Colord (e.g., a
neutral color like this.shore or a defined default color) or throws a
descriptive error for unknown TerrainType, ensuring every switch path returns a
Colord.
---
Outside diff comments:
In `@src/client/theme/PastelTheme.ts`:
- Around line 73-110: The switch in terrainColor(gm: GameMap, tile: TileRef):
Colord does not exhaustively handle all TerrainType values and can return
undefined if the enum gains members; update the function to include an explicit
exhaustive/default branch (e.g., a default case or a final throw) that either
returns a safe Colord fallback or throws a descriptive error including the
unexpected terrain value, so the function always returns a Colord and
TypeScript's control-flow is satisfied.
---
Nitpick comments:
In `@src/client/theme/BaseTheme.ts`:
- Around line 117-128: The orphaned comment after the break in the
contrast-adjustment while loop should be moved to directly precede the else if
(loopCount > loopLimit) branch so it describes that branch correctly; locate the
while loop in BaseTheme.ts (the block using variables contrast, contrastTarget,
loopCount, maxIterations, loopLimit and the else if (loopCount > loopLimit)
clause) and cut the comment currently placed immediately after break; then paste
it above the else if (loopCount > loopLimit) condition so the comment clearly
explains the "Increase the light color..." behavior for loopLimit.
- Around line 17-51: The BaseTheme inheritance creates rigid three-level
hierarchies; refactor to composition by extracting palette and behavior
responsibilities into plain interfaces (e.g., ThemePalettes and ThemeBehaviors)
and have a Theme implementation compose those instead of subclassing BaseTheme:
move palette providers (humanPalette, botPalette, nationPalette,
fallbackPalette, teamColorVariations) into a ThemePalettes object and move
rendering/behavior methods (borderColor, terrainColor, spawn highlight logic
that uses
_spawnHighlightColor/_spawnHighlightSelfColor/_spawnHighlightTeamColor/_spawnHighlightEnemyColor
and teamPlayerColors) into a ThemeBehaviors object; replace the BaseTheme
constructor wiring of humanColorAllocator, botColorAllocator,
nationColorAllocator to accept palettes from the injected ThemePalettes and
ensure existing consumers of BaseTheme now receive a composed Theme instance
that delegates to the palettes/behaviors, keeping the ColorAllocator usage and
teamPlayerColors map intact for backward compatibility.
In `@tests/Colors.test.ts`:
- Around line 86-114: Add a test that instantiates PastelTheme and
ColorblindTheme and asserts that ColorblindTheme.teamColor(ColoredTeams.Blue) is
different from PastelTheme.teamColor(ColoredTeams.Blue) (use the Color.isEqual
method and expect(...).toBe(false)); place the test alongside the existing
PastelTheme tests so it verifies ColorblindTheme actually provides a distinct
palette.
🪄 Autofix (Beta)
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
Run ID: 9d87b54f-df31-44b9-8f22-edfab5725b22
📒 Files selected for processing (11)
resources/lang/en.jsonsrc/client/UserSettingModal.tssrc/client/render/gl/GraphicsOverrides.tssrc/client/render/gl/RenderOverrides.tssrc/client/theme/BaseTheme.tssrc/client/theme/ColorAllocator.tssrc/client/theme/ColorblindTheme.tssrc/client/theme/Colors.tssrc/client/theme/PastelTheme.tssrc/client/theme/ThemeProvider.tstests/Colors.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
- Add JSDoc docstrings across the theme classes/methods, ColorAllocator helpers, RenderOverrides functions, and the colorblind settings methods to satisfy the docstring-coverage threshold. - Add an exhaustiveness guard to the terrainColor switch in PastelTheme and ColorblindTheme so a new TerrainType becomes a compile error rather than a silent undefined return. - Fix an orphaned comment in BaseTheme.structureColors. - Add a test verifying ColorblindTheme applies a palette distinct from PastelTheme.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/theme/ColorAllocator.ts (1)
68-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn type says
number | null, but null is never returned.The function only throws (when there are no assigned colors) or returns
maxIndex(a number). Sonullis impossible, which makes the?? 0guard at the call site (line 54) dead code and the type a bit confusing to read. Suggest dropping| nullto match real behavior.♻️ Make the type match behavior
-export function selectDistinctColorIndex( - availableColors: Colord[], - assignedColors: Colord[], -): number | null { +export function selectDistinctColorIndex( + availableColors: Colord[], + assignedColors: Colord[], +): number {Then the call site can drop the
?? 0:- selectedIndex = - selectDistinctColorIndex(this.availableColors, assignedColors) ?? 0; + selectedIndex = selectDistinctColorIndex( + this.availableColors, + assignedColors, + );🤖 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 68 - 90, The function selectDistinctColorIndex declares a return type of number | null but never returns null (it throws when assignedColors is empty and otherwise returns maxIndex), which mismatches its behavior; update the signature to return number (remove | null) and adjust any call sites (e.g., where ?? 0 is used) accordingly, ensuring selectDistinctColorIndex, its parameters availableColors and assignedColors, and its returned maxIndex are consistently typed as number throughout the codebase.
🤖 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.
Outside diff comments:
In `@src/client/theme/ColorAllocator.ts`:
- Around line 68-90: The function selectDistinctColorIndex declares a return
type of number | null but never returns null (it throws when assignedColors is
empty and otherwise returns maxIndex), which mismatches its behavior; update the
signature to return number (remove | null) and adjust any call sites (e.g.,
where ?? 0 is used) accordingly, ensuring selectDistinctColorIndex, its
parameters availableColors and assignedColors, and its returned maxIndex are
consistently typed as number throughout the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a089c44c-e4a3-4460-b149-fc98e8044316
📒 Files selected for processing (7)
src/client/UserSettingModal.tssrc/client/render/gl/RenderOverrides.tssrc/client/theme/BaseTheme.tssrc/client/theme/ColorAllocator.tssrc/client/theme/ColorblindTheme.tssrc/client/theme/PastelTheme.tstests/Colors.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/UserSettingModal.ts
- tests/Colors.test.ts
- src/client/theme/PastelTheme.ts
- src/client/theme/BaseTheme.ts
The function only throws or returns a number index, so the `| null` return type and the `?? 0` guard at the call site were dead. Narrow the return type to `number` and remove the now-unnecessary fallback and the null assertion in the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add approved & assigned issue number here:
Resolves #2549
Description:
Adds colorblind mode. Similar to dark mode, it exists as a toggle in settings. When enabled, it swaps the game's theme (which is refactored to extend from a theme base class) to use more colorblind-friendly colors and brightness variations. Borders are darkened, and terrarin is separated by lightness. Friendly/Foe colors and switched to blue/orange instead of red/green.
The theme refactor supports adding new themes without having to reimplement the color distribution system. New themes can extend the BaseTheme and supply the data, such as palettes, team-color variations, and terrain.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
jetaviz