-
Notifications
You must be signed in to change notification settings - Fork 135
Overhaul Settings Management System #1243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Overhaul Settings Management System #1243
Conversation
Major expansion of settings management to expose 80+ new settingsManager parameters to users, increasing from 15 (4.7%) to 95+ (30%) exposed settings. **Modified Plugins:** - settings-menu: Added Advanced Orbital and Advanced UI Settings sections - Orbital: covariance confidence level, cone distance, orbit segments/fade, line scan settings - UI: splash screen, logos, sensor/map loading, map resolution - graphics-menu: Replaced stub with fully functional Graphics Settings Menu - Earth rendering: textures, grayscale, lighting, political/clouds maps - Atmosphere & Effects: atmosphere, aurora, godrays (samples, exposure, density) - Sun settings: draw, texture, size - Space: Milky Way, skybox, planets - Satellite rendering: min/max size, vertex shader size - Performance: small images, earth segments **New Plugins:** - performance-menu: Performance & Resource Management - Performance modes: low perf, draw less - Rendering limits: missiles, FOV markers, analyst sats, OEM sats, debris - FPS throttling: min draw time, sun/moon throttle, velocity throttle - data-menu: Data Sources & Catalogs - TLE sources: external URL, external-only mode, supplement mode - Catalogs: debris, JSC, extra catalog toggles - Site data: control sites, launch sites, sensors - Servers: telemetry and user server URLs - camera-menu: Camera Controls & Movement - View: field of view, movement speed, decay factor - Zoom: speed, max distance, near level, rotation/snap behavior - Auto camera: pan/rotate speeds - FPS mode: forward/side/vertical speeds, pitch/yaw/rotate rates All settings include: - Real-time sync with settingsManager - Local storage persistence - Input validation with appropriate ranges - Sound effects on toggle - Reset to defaults functionality - Toast notifications on save Ref: #issue-number
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR significantly expands the settings management system by exposing 80+ new settingsManager parameters, increasing coverage from 15 to 95+ settings (4.7% to 30%). The changes introduce four new settings menu plugins and enhance the existing settings menu.
Key Changes:
- Added four new plugin modules: Camera Menu, Performance Menu, Data Menu, and a fully functional Graphics Menu (replacing stub)
- Expanded Settings Menu with Advanced Orbital and Advanced UI Settings sections
- Implemented comprehensive UI controls with real-time sync, validation, persistence, and reset functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/plugins.ts | Registers three new menu plugins (camera, performance, data) |
| src/plugins/settings-menu/settings-menu.ts | Adds Advanced Orbital and Advanced UI settings sections with 12 new configurable parameters |
| src/plugins/performance-menu/performance-menu.ts | New plugin for performance/resource management with rendering limits and FPS throttling |
| src/plugins/graphics-menu/graphics-menu.ts | Replaces stub with full graphics settings implementation covering Earth, atmosphere, sun, space, and satellite rendering |
| src/plugins/data-menu/data-menu.ts | New plugin for data sources and catalog configuration including TLE sources and server URLs |
| src/plugins/camera-menu/camera-menu.ts | New plugin for camera controls covering view, zoom, auto-camera, and FPS mode settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!isNaN(coneDistanceFromEarth)) { | ||
| settingsManager.coneDistanceFromEarth = coneDistanceFromEarth as Kilometers; |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing range validation for coneDistanceFromEarth. While the HTML input has min="-100" max="1000", the JavaScript validation (line 835-837) doesn't enforce these bounds. This could allow invalid values if the input is manipulated. Add range validation: if (!isNaN(coneDistanceFromEarth) && coneDistanceFromEarth >= -100 && coneDistanceFromEarth <= 1000)
| if (!isNaN(coneDistanceFromEarth)) { | |
| settingsManager.coneDistanceFromEarth = coneDistanceFromEarth as Kilometers; | |
| if (!isNaN(coneDistanceFromEarth) && coneDistanceFromEarth >= -100 && coneDistanceFromEarth <= 1000) { | |
| settingsManager.coneDistanceFromEarth = coneDistanceFromEarth as Kilometers; | |
| } else { | |
| (<HTMLInputElement>getEl('settings-coneDistanceFromEarth')).value = settingsManager.coneDistanceFromEarth.toString(); | |
| uiManagerInstance.toast('Invalid cone distance from Earth value!', ToastMsgType.critical); |
| { init: () => new CameraMenuPlugin().init(), config: plugins.SettingsMenuPlugin }, | ||
| { init: () => new PerformanceMenuPlugin().init(), config: plugins.SettingsMenuPlugin }, | ||
| { init: () => new DataMenuPlugin().init(), config: plugins.SettingsMenuPlugin }, |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new plugins (CameraMenuPlugin, PerformanceMenuPlugin, DataMenuPlugin) are all incorrectly registered with config: plugins.SettingsMenuPlugin instead of their own config. This will cause all three plugins to share the same configuration settings, which is likely incorrect. Each plugin should have its own configuration entry.
| { init: () => new CameraMenuPlugin().init(), config: plugins.SettingsMenuPlugin }, | |
| { init: () => new PerformanceMenuPlugin().init(), config: plugins.SettingsMenuPlugin }, | |
| { init: () => new DataMenuPlugin().init(), config: plugins.SettingsMenuPlugin }, | |
| { init: () => new CameraMenuPlugin().init(), config: plugins.CameraMenuPlugin }, | |
| { init: () => new PerformanceMenuPlugin().init(), config: plugins.PerformanceMenuPlugin }, | |
| { init: () => new DataMenuPlugin().init(), config: plugins.DataMenuPlugin }, |
| import { parseRgba } from '@app/engine/utils/rgba'; | ||
| import { rgbCss } from '@app/engine/utils/rgbCss'; | ||
| import { SettingsManager } from '@app/settings/settings'; | ||
| import { OrbitCruncherMsgType } from '@app/webworker/orbit-cruncher-interfaces'; |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The import of OrbitCruncherMsgType has been moved from line 19 to line 15, but it appears to be unused in the visible diff. If this import is not used in this file, it should be removed. Otherwise, moving it without a clear reason may indicate inconsistent import organization.
| import { OrbitCruncherMsgType } from '@app/webworker/orbit-cruncher-interfaces'; |
| static syncOnLoad() { | ||
| const performanceSettings = [ | ||
| { id: 'performance-lowPerf', setting: 'lowPerf' }, | ||
| { id: 'performance-drawLess', setting: 'isDrawLess' }, | ||
| { id: 'performance-disableCanvas', setting: 'isDisableCanvas' }, | ||
| ]; | ||
|
|
||
| performanceSettings.forEach(({ id, setting }) => { | ||
| const element = <HTMLInputElement>getEl(id); | ||
|
|
||
| if (element) { | ||
| element.checked = settingsManager[setting]; | ||
| } | ||
| }); |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing global settingsManager declaration. The code uses settingsManager directly (line 168, 176, etc.) without importing it or declaring it. This will cause a TypeScript error unless settingsManager is a global variable. Consider adding const settingsManager = SettingsManager.getInstance(); or importing it properly.
| static syncOnLoad() { | ||
| const graphicsSettings = [ | ||
| { id: 'graphics-blackEarth', setting: 'isBlackEarth' }, | ||
| { id: 'graphics-grayScale', setting: 'isEarthGrayScale' }, | ||
| { id: 'graphics-ambientLighting', setting: 'isEarthAmbientLighting' }, | ||
| { id: 'graphics-politicalMap', setting: 'isDrawPoliticalMap' }, | ||
| { id: 'graphics-cloudsMap', setting: 'isDrawCloudsMap' }, | ||
| { id: 'graphics-specMap', setting: 'isDrawSpecMap' }, | ||
| { id: 'graphics-bumpMap', setting: 'isDrawBumpMap' }, | ||
| { id: 'graphics-aurora', setting: 'isDrawAurora' }, | ||
| { id: 'graphics-disableGodrays', setting: 'isDisableGodrays' }, | ||
| { id: 'graphics-drawSun', setting: 'isDrawSun' }, | ||
| { id: 'graphics-sunTexture', setting: 'isUseSunTexture' }, | ||
| { id: 'graphics-milkyWay', setting: 'isDrawMilkyWay' }, | ||
| { id: 'graphics-hiresMilkyWay', setting: 'hiresMilkWay' }, | ||
| { id: 'graphics-graySkybox', setting: 'isGraySkybox' }, | ||
| { id: 'graphics-disableSkybox', setting: 'isDisableSkybox' }, | ||
| { id: 'graphics-disablePlanets', setting: 'isDisablePlanets' }, | ||
| { id: 'graphics-smallImages', setting: 'smallImages' }, | ||
| ]; | ||
|
|
||
| graphicsSettings.forEach(({ id, setting }) => { | ||
| const element = <HTMLInputElement>getEl(id); | ||
|
|
||
| if (element) { | ||
| element.checked = settingsManager[setting]; | ||
| } | ||
| }); |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing global settingsManager declaration. The code uses settingsManager directly without importing it or declaring it. This will cause a TypeScript error unless settingsManager is a global variable. Consider adding const settingsManager = SettingsManager.getInstance(); or importing it properly.
| static syncOnLoad() { | ||
| const cameraSettings = [ | ||
| { id: 'camera-disableControls', setting: 'disableCameraControls' }, | ||
| { id: 'camera-zoomStopsRotation', setting: 'isZoomStopsRotation' }, | ||
| { id: 'camera-zoomStopsSnapped', setting: 'isZoomStopsSnappedOnSat' }, | ||
| ]; | ||
|
|
||
| cameraSettings.forEach(({ id, setting }) => { | ||
| const element = <HTMLInputElement>getEl(id); | ||
|
|
||
| if (element) { | ||
| element.checked = settingsManager[setting]; | ||
| } | ||
| }); |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing global settingsManager declaration. The code uses settingsManager directly without importing it or declaring it. This will cause a TypeScript error unless settingsManager is a global variable. Consider adding const settingsManager = SettingsManager.getInstance(); or importing it properly.
|
|
||
| const godraysSamples = parseInt((<HTMLInputElement>getEl('graphics-godraysSamples')).value); | ||
|
|
||
| if (!isNaN(godraysSamples)) { |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The godraysSamples validation on line 465 only checks if the value is not NaN, but doesn't validate the range. The HTML restricts values to [24, 36, 52, 80], but the validation allows any integer value. This inconsistency could allow invalid values if the input is manipulated. Add range validation: if (!isNaN(godraysSamples) && [24, 36, 52, 80].includes(godraysSamples))
| if (!isNaN(godraysSamples)) { | |
| if (!isNaN(godraysSamples) && [24, 36, 52, 80].includes(godraysSamples)) { |
| static syncOnLoad() { | ||
| const dataSettings = [ | ||
| { id: 'data-externalTLEsOnly', setting: 'dataSources.externalTLEsOnly' }, | ||
| { id: 'data-isSupplementExternal', setting: 'dataSources.isSupplementExternal' }, | ||
| { id: 'data-useDebrisCatalog', setting: 'isUseDebrisCatalog' }, | ||
| { id: 'data-enableJscCatalog', setting: 'isEnableJscCatalog' }, | ||
| { id: 'data-disableExtraCatalog', setting: 'isDisableExtraCatalog' }, | ||
| { id: 'data-disableControlSites', setting: 'isDisableControlSites' }, | ||
| { id: 'data-disableLaunchSites', setting: 'isDisableLaunchSites' }, | ||
| { id: 'data-disableSensors', setting: 'isDisableSensors' }, | ||
| ]; | ||
|
|
||
| dataSettings.forEach(({ id, setting }) => { | ||
| const element = <HTMLInputElement>getEl(id); | ||
|
|
||
| if (element) { | ||
| const settingPath = setting.split('.'); | ||
|
|
||
| if (settingPath.length === 2) { | ||
| element.checked = settingsManager[settingPath[0]][settingPath[1]]; | ||
| } else { | ||
| element.checked = settingsManager[setting]; | ||
| } | ||
| } | ||
| }); |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing global settingsManager declaration. The code uses settingsManager directly without importing it or declaring it. This will cause a TypeScript error unless settingsManager is a global variable. Consider adding const settingsManager = SettingsManager.getInstance(); or importing it properly.
| // eslint-disable-next-line complexity | ||
| private static onSubmit_(e: SubmitEvent) { |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The eslint-disable comment for complexity is unnecessary and indicates the function may be too complex. Consider refactoring onSubmit_ into smaller helper functions (e.g., validateAndSetViewSettings(), validateAndSetZoomSettings(), validateAndSetFPSSettings()) to improve maintainability and remove the need for this eslint disable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| init: async () => { | ||
| const proPlugin = await import('../plugins-pro/graphics-menu/graphics-menu'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Load graphics menu from existing plugin
Plugin registration still imports ../plugins-pro/graphics-menu/graphics-menu, but this repo does not include a plugins-pro directory while the implemented menu now lives in src/plugins/graphics-menu/graphics-menu.ts. With GraphicsMenuPlugin enabled (it is enabled by default in default-plugins.ts), this dynamic import rejects at runtime, the graphics menu never initializes, and the new settings UI remains unavailable.
Useful? React with 👍 / 👎.
|


Major expansion of settings management to expose 80+ new settingsManager parameters to users, increasing from 15 (4.7%) to 95+ (30%) exposed settings.
Modified Plugins:
settings-menu: Added Advanced Orbital and Advanced UI Settings sections
graphics-menu: Replaced stub with fully functional Graphics Settings Menu
New Plugins:
performance-menu: Performance & Resource Management
data-menu: Data Sources & Catalogs
camera-menu: Camera Controls & Movement
All settings include:
Ref: #issue-number