Add a graphics setting to render names/troops in a thin Arial bitmap font#4288
Open
evanpelle wants to merge 10 commits into
Open
Add a graphics setting to render names/troops in a thin Arial bitmap font#4288evanpelle wants to merge 10 commits into
evanpelle wants to merge 10 commits into
Conversation
NamePass can now render player names and troop counts in an Arial bitmap font as an alternative to the overpass-bold MSDF font, toggled live via settings.name.classicFont (mirrors the structure-level font toggle). The Arial atlas is built once at runtime with canvas 2D (there is no Arial asset to ship): each glyph is rasterized, its tight bbox found by a pixel scan, and the glyphs shelf-packed into a coverage atlas. It emits a ParsedAtlas with the same em size (48) and baseline (36) as the MSDF atlas, so it flows through the existing glyph-table/metrics/layout code and leaves name sizing, hit-testing, flag offsets and the icon/status passes unchanged. The name shader gains a uClassic branch that tints the coverage mask with the fill color (no outline, matching the old DOM-rendered names). On toggle, all name/troop lines are re-laid-out since glyph advances differ between the fonts. Exposed as name.classicFont in GraphicsOverrides (default false = MSDF) with a Classic Names toggle in the graphics settings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a "classic names" toggle that switches player-name labels between the existing MSDF overpass atlas and a pre-built Arial bitmap atlas. The change extends the render-settings type and override schema, refactors atlas loading to support two fonts, rewrites ChangesClassic names font toggle
Sequence Diagram(s)sequenceDiagram
participant User
participant GraphicsSettingsModal
participant GraphicsOverrides
participant NamePass
participant TextProgram
User->>GraphicsSettingsModal: click "Classic names" toggle
GraphicsSettingsModal->>GraphicsOverrides: patch name.classicFont = true
Note over NamePass: next draw() call
NamePass->>NamePass: syncFont() detects classicFont change
NamePass->>NamePass: ensureArialFont() — lazy preload arial-atlas.json
NamePass->>TextProgram: setArialFont(metricsTex, arialAtlas)
NamePass->>NamePass: switchFont() — swap glyph/kern tables, re-layout all slots
NamePass->>TextProgram: isReady(true)
NamePass->>TextProgram: draw(..., classic=true)
TextProgram->>TextProgram: select arial FontGpu record, bind textures
TextProgram->>TextProgram: instanced draw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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/render/gl/RenderOverrides.ts`:
- Around line 31-32: The hardcoded `false` default on line 32 in the assignment
`settings.name.classicFont = overrides.name?.classicFont ?? false;` bypasses the
default from render-settings.json. Remove the `?? false` fallback and only
assign the setting if an override is explicitly provided, preserving the
existing resolved setting otherwise. Additionally, in GraphicsSettingsModal.ts,
update the `currentClassicNames()` function to fallback to
`renderDefaults.name.classicFont` instead of hardcoded `false`, ensuring UI
state matches the renderer defaults and respects render-settings.json as the
single source of truth for default values.
🪄 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: 354130ed-ab83-435c-8cdd-7ee9ac41b69b
⛔ Files ignored due to path filters (1)
src/client/render/gl/shaders/name/name.frag.glslis excluded by!**/*.glsl
📒 Files selected for processing (9)
resources/lang/en.jsonsrc/client/hud/layers/GraphicsSettingsModal.tssrc/client/render/gl/GraphicsOverrides.tssrc/client/render/gl/RenderOverrides.tssrc/client/render/gl/RenderSettings.tssrc/client/render/gl/passes/name-pass/ArialAtlas.tssrc/client/render/gl/passes/name-pass/TextProgram.tssrc/client/render/gl/passes/name-pass/index.tssrc/client/render/gl/render-settings.json
Comment on lines
+31
to
+32
| // Classic names: Arial bitmap font for names/troops (default false = MSDF). | ||
| settings.name.classicFont = overrides.name?.classicFont ?? false; |
Contributor
There was a problem hiding this comment.
Do not hardcode the default for name.classicFont in override application.
Line 32 forces false when the user has no override, which bypasses the default from render-settings.json. Keep the existing resolved setting unless an override is explicitly provided.
Suggested fix
- settings.name.classicFont = overrides.name?.classicFont ?? false;
+ if (overrides.name?.classicFont !== undefined) {
+ settings.name.classicFont = overrides.name.classicFont;
+ }Also align src/client/hud/layers/GraphicsSettingsModal.ts currentClassicNames() to fallback to renderDefaults.name.classicFont (not false) so UI state matches renderer defaults.
As per coding guidelines, src/client/render/gl/render-settings.json should be the single source of truth for render tuning/default values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Classic names: Arial bitmap font for names/troops (default false = MSDF). | |
| settings.name.classicFont = overrides.name?.classicFont ?? false; | |
| // Classic names: Arial bitmap font for names/troops (default false = MSDF). | |
| if (overrides.name?.classicFont !== undefined) { | |
| settings.name.classicFont = overrides.name.classicFont; | |
| } |
🤖 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/render/gl/RenderOverrides.ts` around lines 31 - 32, The hardcoded
`false` default on line 32 in the assignment `settings.name.classicFont =
overrides.name?.classicFont ?? false;` bypasses the default from
render-settings.json. Remove the `?? false` fallback and only assign the setting
if an override is explicitly provided, preserving the existing resolved setting
otherwise. Additionally, in GraphicsSettingsModal.ts, update the
`currentClassicNames()` function to fallback to
`renderDefaults.name.classicFont` instead of hardcoded `false`, ensuring UI
state matches the renderer defaults and respects render-settings.json as the
single source of truth for default values.
Source: Coding guidelines
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Arial has no face lighter than Regular, so erode the rasterized coverage (~1px) to shave stroke width for a thinner look. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Antialias the coverage mask's 0.5 contour in screen space (fwidth) so large names stay ~1px-sharp instead of blurring when drawn big. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t thicker Rasterize the Arial atlas 2x finer than the em metrics so large names have straighter, cleaner edges (the 48px atlas wobbled under the screen-space edge sharpening). The name shader now derives glyph UVs from glyph-size / atlas-pixels-per-em, so metrics stay in 48-em space and flag/icon sizing is unchanged; for the MSDF atlas this is identical to the old precomputed UV. Also lighten the coverage erosion so strokes are a bit thicker. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the runtime-generated bitmap Arial with a real MSDF atlas in the exact overpass format (resources/atlases/arial-atlas.{png,json}), generated from Arimo (Apache-licensed, Arial-metric clone). NamePass loads both MSDF atlases and switches per draw using each atlas's own metrics (em size, baseline, atlas dims, distance range); the flag/status/debug passes follow via setFont. Crisp at any zoom — no blur, wobble, or thinning hacks.
Removes the runtime bitmap generator, the uClassic shader branch, and the supersample/erosion code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/render/gl/passes/name-pass/AtlasData.ts`:
- Around line 28-39: The preload function caches promises using the nullish
coalescing assignment operator (??=) in the atlasPromises object, but if a fetch
fails, the rejected promise remains cached permanently, preventing retry
attempts. Add a catch handler to the promise chain within the preload function
that deletes the cached promise entry (atlasPromises[file]) when an error
occurs, allowing subsequent calls to attempt a fresh fetch instead of reusing
the rejected promise.
🪄 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: 45be41dd-ec6a-4e94-b5a5-1ea57d4e1472
⛔ Files ignored due to path filters (1)
resources/atlases/arial-atlas.pngis excluded by!**/*.png
📒 Files selected for processing (9)
resources/atlases/arial-atlas.jsonsrc/client/ClientGameRunner.tssrc/client/render/gl/index.tssrc/client/render/gl/passes/name-pass/AtlasData.tssrc/client/render/gl/passes/name-pass/DebugProgram.tssrc/client/render/gl/passes/name-pass/IconProgram.tssrc/client/render/gl/passes/name-pass/StatusIconProgram.tssrc/client/render/gl/passes/name-pass/TextProgram.tssrc/client/render/gl/passes/name-pass/index.ts
✅ Files skipped from review due to trivial changes (2)
- src/client/render/gl/index.ts
- src/client/render/gl/passes/name-pass/IconProgram.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/render/gl/passes/name-pass/index.ts
Comment on lines
+28
to
+39
| function preload(file: string): Promise<void> { | ||
| atlasPromises[file] ??= fetch(assetUrl(`atlases/${file}`)) | ||
| .then((response) => { | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch msdf-atlas.json: ${response.status}`); | ||
| throw new Error(`Failed to fetch ${file}: ${response.status}`); | ||
| } | ||
| return response.json(); | ||
| }) | ||
| .then((json) => { | ||
| atlasData = json as RawMsdfAtlas; | ||
| atlasData[file] = json as RawMsdfAtlas; | ||
| }); | ||
| return atlasDataPromise; | ||
| return atlasPromises[file]; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether preload() currently has rejection recovery for atlasPromises cache.
python - <<'PY'
from pathlib import Path
p = Path("src/client/render/gl/passes/name-pass/AtlasData.ts").read_text()
start = p.find("function preload(file: string): Promise<void> {")
end = p.find("function parse(file: string): ParsedAtlas {")
seg = p[start:end]
print("Has .catch in preload:", ".catch(" in seg)
print("Clears atlasPromises[file]:", "delete atlasPromises[file]" in seg)
print("Clears atlasData[file]:", "delete atlasData[file]" in seg)
PYRepository: openfrontio/OpenFrontIO
Length of output: 160
Clear rejected preload promises so atlas fetches can recover on retry.
The preload function caches the first Promise permanently using ??=. If the fetch fails once, all later calls reuse the same rejected Promise, preventing the atlas from being retried in the same session. Verification confirmed no rejection handler exists.
Suggested fix
function preload(file: string): Promise<void> {
atlasPromises[file] ??= fetch(assetUrl(`atlases/${file}`))
.then((response) => {
if (!response.ok) {
throw new Error(`Failed to fetch ${file}: ${response.status}`);
}
return response.json();
})
.then((json) => {
atlasData[file] = json as RawMsdfAtlas;
- });
+ })
+ .catch((error) => {
+ delete atlasPromises[file];
+ delete atlasData[file];
+ throw error;
+ });
return atlasPromises[file];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function preload(file: string): Promise<void> { | |
| atlasPromises[file] ??= fetch(assetUrl(`atlases/${file}`)) | |
| .then((response) => { | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch msdf-atlas.json: ${response.status}`); | |
| throw new Error(`Failed to fetch ${file}: ${response.status}`); | |
| } | |
| return response.json(); | |
| }) | |
| .then((json) => { | |
| atlasData = json as RawMsdfAtlas; | |
| atlasData[file] = json as RawMsdfAtlas; | |
| }); | |
| return atlasDataPromise; | |
| return atlasPromises[file]; | |
| function preload(file: string): Promise<void> { | |
| atlasPromises[file] ??= fetch(assetUrl(`atlases/${file}`)) | |
| .then((response) => { | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch ${file}: ${response.status}`); | |
| } | |
| return response.json(); | |
| }) | |
| .then((json) => { | |
| atlasData[file] = json as RawMsdfAtlas; | |
| }) | |
| .catch((error) => { | |
| delete atlasPromises[file]; | |
| delete atlasData[file]; | |
| throw error; | |
| }); | |
| return atlasPromises[file]; | |
| } |
🤖 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/render/gl/passes/name-pass/AtlasData.ts` around lines 28 - 39, The
preload function caches promises using the nullish coalescing assignment
operator (??=) in the atlasPromises object, but if a fetch fails, the rejected
promise remains cached permanently, preventing retry attempts. Add a catch
handler to the promise chain within the preload function that deletes the cached
promise entry (atlasPromises[file]) when an error occurs, allowing subsequent
calls to attempt a fresh fetch instead of reusing the rejected promise.
The Arial MSDF atlas (~423KB png) is now fetched + uploaded the first time name.classicFont is enabled, rather than eagerly at game start. Default players who never switch fonts download nothing extra. On first toggle, names keep rendering in MSDF until the atlas is ready, then switch (no blank flash); TextProgram.setArialFont now resolves when the image is uploaded so NamePass can defer the switch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/render/gl/passes/name-pass/index.ts`:
- Around line 276-297: The `ensureArialFont()` method's promise chain can
complete after the pass is disposed, potentially creating WebGL resources or
calling methods on deleted GL programs. Add a disposal state check before
executing critical operations in the promise chain—specifically before calling
`buildGlyphMetricsTex()`, `textProgram.setArialFont()`, and `switchFont()`—to
ensure these operations only proceed if the pass has not been disposed. Consider
introducing a disposal flag or similar state mechanism that gets set in the
`dispose()` method and checked within the promise chain's `.then()` handlers to
prevent further execution if disposal has occurred.
🪄 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: dd0acae6-5be8-4e4a-b771-ffec26a54a3e
📒 Files selected for processing (3)
src/client/ClientGameRunner.tssrc/client/render/gl/passes/name-pass/TextProgram.tssrc/client/render/gl/passes/name-pass/index.ts
✅ Files skipped from review due to trivial changes (1)
- src/client/ClientGameRunner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/render/gl/passes/name-pass/TextProgram.ts
Comment on lines
+276
to
+297
| private ensureArialFont(): void { | ||
| if (this.arialLoaded || this.arialLoading) return; | ||
| this.arialLoading = true; | ||
| preloadArialAtlasData() | ||
| .then(() => { | ||
| const atlas = parseArialAtlasData(); | ||
| this.arialGlyph = buildGlyphTables(atlas.chars); | ||
| this.arialKern = buildKernTable(atlas.kernings); | ||
| this.arialMetricsTex = buildGlyphMetricsTex(this.gl, atlas); | ||
| this.arialFontSize = atlas.fontSize; | ||
| this.arialBase = atlas.base; | ||
| return this.textProgram.setArialFont(this.arialMetricsTex, atlas); | ||
| }) | ||
| .then(() => { | ||
| this.arialLoaded = true; | ||
| // Switch now if it's (still) the selected font. | ||
| if (this.settings.name.classicFont) this.switchFont(true); | ||
| }) | ||
| .catch((err) => { | ||
| console.error("Failed to load Arial atlas:", err); | ||
| // Leave arialLoading=true so we don't spin retrying a hard failure. | ||
| }); |
Contributor
There was a problem hiding this comment.
Stop the Arial load when the pass is disposed.
ensureArialFont() can finish after dispose() has deleted the GL programs/textures. Then it may create arialMetricsTex or call textProgram.setArialFont(...) on a disposed pass, leaking WebGL resources or causing GL errors.
Proposed fix
export class NamePass {
@@
private arialLoaded = false;
private arialLoading = false;
+ private disposed = false;
@@
private ensureArialFont(): void {
- if (this.arialLoaded || this.arialLoading) return;
+ if (this.disposed || this.arialLoaded || this.arialLoading) return;
this.arialLoading = true;
preloadArialAtlasData()
.then(() => {
+ if (this.disposed) return false;
const atlas = parseArialAtlasData();
+ const metricsTex = buildGlyphMetricsTex(this.gl, atlas);
+ if (this.disposed) {
+ this.gl.deleteTexture(metricsTex);
+ return false;
+ }
this.arialGlyph = buildGlyphTables(atlas.chars);
this.arialKern = buildKernTable(atlas.kernings);
- this.arialMetricsTex = buildGlyphMetricsTex(this.gl, atlas);
+ this.arialMetricsTex = metricsTex;
this.arialFontSize = atlas.fontSize;
this.arialBase = atlas.base;
- return this.textProgram.setArialFont(this.arialMetricsTex, atlas);
+ return this.textProgram
+ .setArialFont(this.arialMetricsTex, atlas)
+ .then(() => true);
})
- .then(() => {
+ .then((ready) => {
+ if (!ready || this.disposed) return;
this.arialLoaded = true;
// Switch now if it's (still) the selected font.
if (this.settings.name.classicFont) this.switchFont(true);
@@
dispose(): void {
+ this.disposed = true;
const gl = this.gl;Also applies to: 834-842
🤖 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/render/gl/passes/name-pass/index.ts` around lines 276 - 297, The
`ensureArialFont()` method's promise chain can complete after the pass is
disposed, potentially creating WebGL resources or calling methods on deleted GL
programs. Add a disposal state check before executing critical operations in the
promise chain—specifically before calling `buildGlyphMetricsTex()`,
`textProgram.setArialFont()`, and `switchFont()`—to ensure these operations only
proceed if the pass has not been disposed. Consider introducing a disposal flag
or similar state mechanism that gets set in the `dispose()` method and checked
within the promise chain's `.then()` handlers to prevent further execution if
disposal has occurred.
The names font setting now reads 'Name font: Overpass / Arial' instead of 'Classic names: On / Off', matching the colored-names toggle's value style. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Regenerate arial-atlas.{png,json} from a full Arimo TTF (latin + latin-ext) covering the same 32-383 range as overpass (319 glyphs, no Latin-Extended gaps), and make Arial the default name font (name.classicFont defaults true). Overpass stays for WorldTextPass, the structure-level MSDF option, and as the alternate name font.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… alternate Revert the default to overpass (more readable for map labels) and leave Arial as the lazy-loaded opt-in 'classic' look, so only players who choose it download its atlas. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What
Player names and troop counts can now render in a thin Arial bitmap font as a live-switchable alternative to the overpass-bold MSDF font — the same pattern as the structure-level font toggle.
Migrating Arial to a bitmap
There's no Arial asset to ship and no offline bmfont tooling, so
name-pass/ArialAtlas.tsbuilds the atlas once at startup with canvas 2D: each glyph (codepoints 32–383) is rasterized (100 48px Arial— thin weight), its tight bounding box found by scanning pixels, and the glyphs shelf-packed into a coverage atlas. It emits aParsedAtlaswith the same em size (48) and baseline (36) as the MSDF atlas, so it flows through the existing glyph-table / metrics / layout code and leaves name sizing, hit-testing, flag offsets, and the icon/status passes unchanged — only the glyph shapes and advances differ.On a user's machine with Arial installed this renders real Arial; otherwise canvas falls back to the system sans-serif. (Standard Arial has no dedicated thin face, so
100resolves to Regular where no thin face exists — still much lighter than the MSDF bold.)How
name.frag.glslgains auClassicbranch that tints the coverage mask with the existing fill color, no outline (matching the old DOM-rendered names).TextProgramis dual-font: holds both atlases + glyph-metric textures, selects per draw, and setsuClassic+ per-font atlas scalars.NamePassbuilds the Arial bundle and, whenname.classicFontflips, swaps the active glyph tables and re-lays-out every name/troop line (advances differ between the fonts).name.classicFontinGraphicsOverrides(defaultfalse= MSDF) with aClassic namestoggle in the graphics settings.Testing
tsc --noEmit, ESLint, and Prettier pass; rebased clean onto the latestmain.🤖 Generated with Claude Code