Conversation
Contributor
WalkthroughThe PR refactors WebGL render settings from a mode-driven day/night model to a direct lighting control model. The ChangesLighting Settings Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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
🧹 Nitpick comments (1)
src/client/render/gl/DarkModeOverride.ts (1)
1-12: ⚡ Quick winMove
DARK_AMBIENTintorender-settings.json.The logic is correct and easy to read. One small thing:
0.35is a lighting tuning value, so it fits better next to the otherlightingknobs in the JSON config rather than as a hardcoded constant here. You could read it from the passedsettingsor a dedicated json field.As per coding guidelines: "All per-pass tuning constants (alpha, radii, colors, etc.) should be defined in
gl/render-settings.json".🤖 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/DarkModeOverride.ts` around lines 1 - 12, The hardcoded DARK_AMBIENT constant should be moved into the render-settings JSON and read from the RenderSettings object; update the RenderSettings typing (e.g. add lighting.darkAmbient or lighting.overrideAmbient) and the JSON default in gl/render-settings.json, then change applyDarkModeOverride to use settings.lighting.darkAmbient instead of DARK_AMBIENT and keep the existing logic (only apply when isDark is true and enable lighting). This ensures the per-pass tuning value is configurable while leaving applyDarkModeOverride, settings.lighting.ambient and settings.lighting.enabled intact.
🤖 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 479-482: The global listener added in ClientGameRunner using
globalThis.addEventListener(`${USER_SETTINGS_CHANGED_EVENT}:settings.territoryPatterns`,
...) is not bound to graphicsListenerAbort.signal so it can outlive stop() and
hold a stale view; update the addEventListener call that wraps
view.setShowPatterns to pass the same abort signal
(graphicsListenerAbort.signal) as an option, or alternatively remove that
listener inside stop(), ensuring the handler uses the same
graphicsListenerAbort.signal as other graphics listeners to avoid leaking the
view reference.
---
Nitpick comments:
In `@src/client/render/gl/DarkModeOverride.ts`:
- Around line 1-12: The hardcoded DARK_AMBIENT constant should be moved into the
render-settings JSON and read from the RenderSettings object; update the
RenderSettings typing (e.g. add lighting.darkAmbient or
lighting.overrideAmbient) and the JSON default in gl/render-settings.json, then
change applyDarkModeOverride to use settings.lighting.darkAmbient instead of
DARK_AMBIENT and keep the existing logic (only apply when isDark is true and
enable lighting). This ensures the per-pass tuning value is configurable while
leaving applyDarkModeOverride, settings.lighting.ambient and
settings.lighting.enabled intact.
🪄 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: 2aed21bb-85d0-4ff0-ad2e-8f1edc22ece9
📒 Files selected for processing (12)
src/client/ClientGameRunner.tssrc/client/render/gl/DarkModeOverride.tssrc/client/render/gl/RenderSettings.tssrc/client/render/gl/Renderer.tssrc/client/render/gl/debug/Layout.tssrc/client/render/gl/index.tssrc/client/render/gl/passes/FalloutLightPass.tssrc/client/render/gl/passes/LightmapPass.tssrc/client/render/gl/passes/NightCompositePass.tssrc/client/render/gl/passes/PointLightPass.tssrc/client/render/gl/render-settings.jsontests/GraphicsOverrides.test.ts
Comment on lines
479
to
482
| globalThis.addEventListener( | ||
| `${USER_SETTINGS_CHANGED_EVENT}:settings.territoryPatterns`, | ||
| (e) => view.setShowPatterns((e as CustomEvent<string>).detail === "true"), | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check where graphicsListenerAbort is created/aborted and whether the
# territoryPatterns listener is removed anywhere.
rg -nP -C3 'graphicsListenerAbort|territoryPatterns|setShowPatterns' src/client/ClientGameRunner.tsRepository: openfrontio/OpenFrontIO
Length of output: 1702
Fix: clean up the territoryPatterns global listener in ClientGameRunner
src/client/ClientGameRunner.ts lines 479-482 add the globalThis.addEventListener(...:settings.territoryPatterns, ...) handler without graphicsListenerAbort.signal, while stop() only aborts graphicsListenerAbort. So this handler can outlive stop() and keep a stale view reference. Bind it to the same abort signal (or remove the listener in stop()).
🤖 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/ClientGameRunner.ts` around lines 479 - 482, The global listener
added in ClientGameRunner using
globalThis.addEventListener(`${USER_SETTINGS_CHANGED_EVENT}:settings.territoryPatterns`,
...) is not bound to graphicsListenerAbort.signal so it can outlive stop() and
hold a stale view; update the addEventListener call that wraps
view.setShowPatterns to pass the same abort signal
(graphicsListenerAbort.signal) as an option, or alternatively remove that
listener inside stop(), ensuring the handler uses the same
graphicsListenerAbort.signal as other graphics listeners to avoid leaking the
view reference.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/render/gl/passes/NightCompositePass.ts (1)
2-6: ⚡ Quick winUpdate comments to reflect the lighting model.
The header comments reference "day/night" terminology, but the implementation now uses the
lightingmodel (ambient + enabled). Consider updating to avoid confusion.📝 Suggested comment updates
/** - * NightCompositePass — scene capture + day/night composite. + * NightCompositePass — scene capture + lighting composite. * * Owns the scene capture FBO: terrain + territory render into it when - * day/night is enabled. Composites the captured scene with a blurred + * lighting is enabled. Composites the captured scene with a blurred * lightmap: output = scene * min(ambient + lightmap, 1.2). * - * At full daytime (ambient ≈ 1.0) the composite is a visual identity — + * At full ambient (≈ 1.0) the composite is a visual identity — * multiplication by ~1.0 — so the pass runs continuously with no threshold. */🤖 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/NightCompositePass.ts` around lines 2 - 6, Update the header comment for NightCompositePass to stop using "day/night" wording and instead describe the current lighting model; mention that this pass owns the scene-capture FBO and that the composite mixes the captured scene with the blurred lightmap using the lighting model (e.g., output = scene * min(ambient + enabledLighting, 1.2) or similar terminology used in the implementation), so the comment matches the actual symbols and formula used by NightCompositePass.
🤖 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/render/gl/passes/NightCompositePass.ts`:
- Around line 2-6: Update the header comment for NightCompositePass to stop
using "day/night" wording and instead describe the current lighting model;
mention that this pass owns the scene-capture FBO and that the composite mixes
the captured scene with the blurred lightmap using the lighting model (e.g.,
output = scene * min(ambient + enabledLighting, 1.2) or similar terminology used
in the implementation), so the comment matches the actual symbols and formula
used by NightCompositePass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b0f164d-edd5-49f5-8d2d-29c57e38cc83
📒 Files selected for processing (12)
src/client/ClientGameRunner.tssrc/client/render/gl/RenderOverrides.tssrc/client/render/gl/RenderSettings.tssrc/client/render/gl/Renderer.tssrc/client/render/gl/debug/Layout.tssrc/client/render/gl/index.tssrc/client/render/gl/passes/FalloutLightPass.tssrc/client/render/gl/passes/LightmapPass.tssrc/client/render/gl/passes/NightCompositePass.tssrc/client/render/gl/passes/PointLightPass.tssrc/client/render/gl/render-settings.jsontests/GraphicsOverrides.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/client/render/gl/render-settings.json
- src/client/render/gl/passes/PointLightPass.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/render/gl/passes/FalloutLightPass.ts
- src/client/render/gl/passes/LightmapPass.ts
- src/client/render/gl/debug/Layout.ts
- src/client/ClientGameRunner.ts
- src/client/render/gl/Renderer.ts
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
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:
RenderSettings.dayNight.mode("light" | "dark") is gone — passes read neutral values (lighting.ambient: number,lighting.enabled: boolean).render-settings.jsonholds the light-mode baseline. Dark mode is just another override layer, applied the same way as graphics settings (darkNames,classicIcons, etc.).src/client/render/gl/RenderOverrides.tsexposes two in-place mutators with matching shapes:applyGraphicsOverrides(settings, overrides)— replaces the oldgenerateRenderSettingsapplyDarkModeOverride(settings, isDark)ClientGameRunnerregenerates the live settings each time the user setting changes viadeepAssign(live, createRenderSettings())+ the override chain. No per-slice copy list, no intermediate object — adding a new override that touches a new section just works.dayNight→lighting; collapsednightAmbient/dayAmbientinto singleambient; renamedenableLightCompositing→enabled.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan