Add Graphics Settings modal with live name-label tuning#4065
Merged
Conversation
Contributor
WalkthroughAdds a graphics settings modal UI, schema and storage for graphics overrides, a helper to generate RenderSettings from overrides, renderer and runner wiring to apply overrides live, settings-menu navigation, and English localization plus DOM insertion. ChangesGraphics Settings Modal and Integration
Sequence DiagramsequenceDiagram
participant GraphicsSettingsModal
participant UserSettings
participant ClientGameRunner
participant WebGLView
GraphicsSettingsModal->>UserSettings: setGraphicsOverrides({ name: { nameScaleFactor, cullThreshold } })
UserSettings->>ClientGameRunner: emit USER_SETTINGS_CHANGED_EVENT:settings.graphics
ClientGameRunner->>WebGLView: generateRenderSettings(userSettings.graphicsOverrides()) -> apply name settings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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: 8
🤖 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/ClientGameRunner.ts`:
- Around line 495-498: You register a global listener with
globalThis.addEventListener(`${USER_SETTINGS_CHANGED_EVENT}:${GRAPHICS_KEY}`,
applyGraphicsOverrides) but never remove it; store the handler reference (e.g.,
const graphicsSettingsHandler = applyGraphicsOverrides or a bound wrapper) in
the ClientGameRunner scope and use
globalThis.removeEventListener(`${USER_SETTINGS_CHANGED_EVENT}:${GRAPHICS_KEY}`,
graphicsSettingsHandler) in the same teardown/stop method (e.g.,
ClientGameRunner.stop or dispose) so the listener is unregistered when the
runner is stopped and rejoining won't stack handlers.
In `@src/client/hud/GameRenderer.ts`:
- Around line 194-202: The code queries document.querySelector for
"graphics-settings-modal" into graphicsSettingsModal but then unconditionally
accesses graphicsSettingsModal.userSettings and .eventBus, which will throw if
the element wasn't found; update the wiring to first check that
graphicsSettingsModal is an instance of GraphicsSettingsModal (or non-null)
before assigning userSettings and eventBus (return early or skip assignments
when the guard fails), keeping the existing console.error log when the modal is
missing; reference the graphicsSettingsModal variable and the userSettings and
eventBus property assignments in your change.
In `@src/client/hud/layers/GraphicsSettingsModal.ts`:
- Around line 152-153: The img alt attribute in GraphicsSettingsModal is
hardcoded as alt="graphicsSettings"; replace it to call translateText(...) with
a new key (e.g. "graphics.settings.iconAlt") and add that key/value to
resources/lang/en.json; update the JSX in GraphicsSettingsModal to use
translateText("graphics.settings.iconAlt") for the alt prop so all user-visible
text goes through translateText.
- Around line 12-21: The schema GraphicsOverridesSchema currently allows any
numeric value for nameScaleFactor and cullThreshold; update the zod types in the
GraphicsOverridesSchema definition to enforce the same slider bounds (e.g.,
z.number().min(MIN_NAME_SCALE).max(MAX_NAME_SCALE) for nameScaleFactor and
z.number().min(MIN_CULL_THRESHOLD).max(MAX_CULL_THRESHOLD) for cullThreshold),
and ensure persisted values are validated/parsed with this schema before being
applied so out-of-range localStorage edits are clamped or rejected.
In `@src/client/hud/layers/SettingsModal.ts`:
- Around line 187-190: The onGraphicsSettingsButtonClick currently calls
closeModal(), which triggers pauseGame(false) inside closeModal() and
unintentionally resumes gameplay; remove the call to closeModal() or change
closeModal to accept a flag to skip resuming (e.g., closeModal({ resume: false
})) and update its internal logic (where pauseGame(false) is called) to only
resume when the flag allows; keep the ShowGraphicsSettingsModalEvent emission so
the graphics modal opens without unpausing the game.
In `@src/client/render/gl/RenderSettings.ts`:
- Line 1: RenderSettings.ts currently imports the GraphicsOverrides type from
the HUD component module (GraphicsSettingsModal), creating cross-layer coupling;
extract the GraphicsOverrides type (the contract/interface) into a new shared
settings/contracts module (e.g., settings/contracts or settings/types) and
export it there, then update RenderSettings.ts to import GraphicsOverrides from
that new shared module and also update GraphicsSettingsModal to import the same
shared type so both modules reference the shared contract instead of
hud/layers/GraphicsSettingsModal.
In `@src/core/game/UserSettings.ts`:
- Around line 1-4: UserSettings is importing GraphicsOverrides and
GraphicsOverridesSchema from a client HUD module, which breaks core→client
layering; extract the GraphicsOverrides type and GraphicsOverridesSchema
validator into a non-UI shared module (e.g., a new src/shared or src/core/types
validator file) or re-create an equivalent core-local type/validator, then
update UserSettings to import GraphicsOverrides and GraphicsOverridesSchema from
that new shared/core file instead of
"../../client/hud/layers/GraphicsSettingsModal"; ensure the new schema has the
same runtime shape and export names so functions in UserSettings continue to use
GraphicsOverrides and GraphicsOverridesSchema unchanged.
- Around line 362-377: Add unit tests for UserSettings.graphicsOverrides and
UserSettings.setGraphicsOverrides to satisfy the requirement that changes to
src/core/ are covered: write tests that (1) persist a valid GraphicsOverrides
via setGraphicsOverrides and confirm graphicsOverrides() returns the same object
(round-trip), (2) store invalid JSON under the GRAPHICS_KEY and confirm
graphicsOverrides() returns {}, (3) store syntactically valid JSON that fails
GraphicsOverridesSchema validation and confirm graphicsOverrides() returns {},
and (4) confirm an absent key returns {}. Target the UserSettings class and its
graphicsOverrides, setGraphicsOverrides methods and assert behavior against
GraphicsOverridesSchema and GRAPHICS_KEY; add these tests to the src/core/ test
suite using the project's existing test harness.
🪄 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: 2c912933-b74b-4e25-ae4c-925eb587072b
📒 Files selected for processing (9)
index.htmlresources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/hud/GameRenderer.tssrc/client/hud/layers/GraphicsSettingsModal.tssrc/client/hud/layers/SettingsModal.tssrc/client/render/gl/RenderSettings.tssrc/client/render/gl/index.tssrc/core/game/UserSettings.ts
Comment on lines
+12
to
+21
| export const GraphicsOverridesSchema = z | ||
| .object({ | ||
| name: z | ||
| .object({ | ||
| nameScaleFactor: z.number(), | ||
| cullThreshold: z.number(), | ||
| }) | ||
| .partial(), | ||
| }) | ||
| .partial(); |
Contributor
There was a problem hiding this comment.
Add bounds checks for persisted override numbers.
nameScaleFactor and cullThreshold currently accept any numeric value. If localStorage is edited manually, extreme values can be applied and destabilize rendering behavior. Please enforce the same min/max limits used by the sliders before values are applied.
🤖 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/hud/layers/GraphicsSettingsModal.ts` around lines 12 - 21, The
schema GraphicsOverridesSchema currently allows any numeric value for
nameScaleFactor and cullThreshold; update the zod types in the
GraphicsOverridesSchema definition to enforce the same slider bounds (e.g.,
z.number().min(MIN_NAME_SCALE).max(MAX_NAME_SCALE) for nameScaleFactor and
z.number().min(MIN_CULL_THRESHOLD).max(MAX_CULL_THRESHOLD) for cullThreshold),
and ensure persisted values are validated/parsed with this schema before being
applied so out-of-range localStorage edits are clamped or rejected.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
graphic-settings
branch
from
cfa83ac to
5b9ddd0
Compare
Contributor
|
Actionable comments posted: 0 |
evanpelle
changed the title
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/hud/layers/GraphicsSettingsModal.ts (1)
42-46: 💤 Low valuePrefer
@state()for internal-only reactive fields.
shouldPauseandwasPausedWhenOpenedare set from event data and used internally. Using@propertyexposes them as HTML attributes, which could lead to unintended external manipulation. For consistency withisVisible(line 36) and to signal these are private state, use@state()instead.♻️ Suggested change
- `@property`({ type: Boolean }) - shouldPause = false; - - `@property`({ type: Boolean }) - wasPausedWhenOpened = false; + `@state`() + private shouldPause = false; + + `@state`() + private wasPausedWhenOpened = 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 `@src/client/hud/layers/GraphicsSettingsModal.ts` around lines 42 - 46, Change the two internal-only reactive fields shouldPause and wasPausedWhenOpened from `@property` to `@state` in the GraphicsSettingsModal component so they are treated as private reactive state (match how isVisible is declared); update the decorator import so that state is imported from the same decorator module used in the file (replace or add state from 'lit/decorators' as appropriate) and keep any other `@property` usages unchanged.
🤖 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.
Nitpick comments:
In `@src/client/hud/layers/GraphicsSettingsModal.ts`:
- Around line 42-46: Change the two internal-only reactive fields shouldPause
and wasPausedWhenOpened from `@property` to `@state` in the GraphicsSettingsModal
component so they are treated as private reactive state (match how isVisible is
declared); update the decorator import so that state is imported from the same
decorator module used in the file (replace or add state from 'lit/decorators' as
appropriate) and keep any other `@property` usages unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f882adf6-d49b-4694-8df6-ca98580d6702
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/hud/layers/GraphicsSettingsModal.tssrc/client/hud/layers/HeadsUpMessage.tssrc/client/hud/layers/SettingsModal.tssrc/client/render/gl/GraphicsOverrides.tssrc/client/render/gl/RenderSettings.tssrc/client/render/gl/index.tssrc/core/game/UserSettings.ts
✅ Files skipped from review due to trivial changes (1)
- src/client/render/gl/GraphicsOverrides.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/render/gl/index.ts
- src/client/render/gl/RenderSettings.ts
- src/core/game/UserSettings.ts
- src/client/hud/layers/SettingsModal.ts
- src/client/ClientGameRunner.ts
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.
Description:
localStorageundersettings.graphics, validated by a Zod schema (GraphicsOverridesSchema). Future graphics knobs just extend the schema + slider list.How it fits together
generateRenderSettings(overrides)(RenderSettings.ts) — pure function: clonesrender-settings.jsondefaults, layers overrides on top, returns a freshRenderSettings.UserSettings.graphicsOverrides()/setGraphicsOverrides()— read/write the blob; falls back to{}on a missing/corrupt entry.ClientGameRunnerlistens forUSER_SETTINGS_CHANGED_EVENT:settings.graphics, regenerates, andObject.assigns each category into the liveview.getSettings()slice so passes pick up the new values on the next frame (no renderer reconstruction).render-settings.jsonso there's no duplication.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan