Feat/graphics settings #2534
Feat/graphics settings #2534
Conversation
next into main
reduce queries
tile cache
donkey fix
Resouce bride out select fix
cache optimisations
fix race condition
update number
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a comprehensive overhaul of graphics settings management across multiple client-side components. The primary change is the replacement of a simple boolean Changes
Sequence DiagramsequenceDiagram
participant User
participant DeviceDetector
participant GraphicsSettings
participant Renderer
User->>DeviceDetector: Select/Detect Device
DeviceDetector->>GraphicsSettings: Determine Graphics Level
GraphicsSettings->>Renderer: Apply Rendering Strategy
alt Low Graphics
Renderer->>Renderer: Reduce Animations
Renderer->>Renderer: Limit Frame Rate
else Mid Graphics
Renderer->>Renderer: Partial Animations
Renderer->>Renderer: Moderate Performance
else High Graphics
Renderer->>Renderer: Full Animations
Renderer->>Renderer: Maximum Performance
end
Possibly related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Failed to generate code suggestions for PR |
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds a new graphics settings system with LOW/MID/HIGH tiers, replacing the previous binary low/high graphics flag. The implementation looks solid with good performance optimizations like frame rate limiting for lower settings. A few suggestions:
- Add documentation for what each graphics tier provides
- Extract some repeated code into utility functions
- Improve type safety around localStorage usage
- Consider adding performance metrics/telemetry
- Add visual indicators in settings UI for recommended graphics level based on device capabilities
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
| @@ -3,29 +3,47 @@ import { BuildingType, FELT_CENTER } from "@bibliothecadao/eternum"; | |||
|
|
|||
| export { FELT_CENTER }; | |||
|
|
|||
There was a problem hiding this comment.
Consider adding JSDoc comments to document what features/performance characteristics each graphics setting level provides
| /** | |
| * Graphics quality settings | |
| * LOW - Reduced shadows, animations, and effects. 30 FPS cap. Best for low-end devices | |
| * MID - Balanced settings with some effects enabled. 30 FPS cap | |
| * HIGH - Full graphics quality with all effects enabled. Uncapped FPS | |
| */ |
| const frameTime = 1000 / 30; // 33.33ms for 30 FPS | ||
| if (currentTime - this.lastTime < frameTime) { |
There was a problem hiding this comment.
Consider extracting the frame rate limiting logic into a reusable utility function since it's used in multiple places
| const frameTime = 1000 / 30; // 33.33ms for 30 FPS | |
| if (currentTime - this.lastTime < frameTime) { | |
| if (isFrameSkipped(currentTime, this.lastTime, 30)) { |
| const checkGraphicsSettings = async () => { | ||
| // Handle migration from old LOW_GRAPHICS_FLAG | ||
| const oldLowGraphicsFlag = localStorage.getItem("LOW_GRAPHICS_FLAG"); | ||
| if (oldLowGraphicsFlag !== null) { | ||
| // Migrate old setting to new format | ||
| const newSetting = oldLowGraphicsFlag === "true" ? GraphicsSettings.LOW : GraphicsSettings.HIGH; | ||
| localStorage.setItem("GRAPHICS_SETTING", newSetting); |
There was a problem hiding this comment.
Consider using TypeScript const assertions for the localStorage keys to prevent typos and improve type safety
| const checkGraphicsSettings = async () => { | |
| // Handle migration from old LOW_GRAPHICS_FLAG | |
| const oldLowGraphicsFlag = localStorage.getItem("LOW_GRAPHICS_FLAG"); | |
| if (oldLowGraphicsFlag !== null) { | |
| // Migrate old setting to new format | |
| const newSetting = oldLowGraphicsFlag === "true" ? GraphicsSettings.LOW : GraphicsSettings.HIGH; | |
| localStorage.setItem("GRAPHICS_SETTING", newSetting); | |
| const STORAGE_KEYS = { | |
| GRAPHICS_SETTING: 'GRAPHICS_SETTING', | |
| LOW_GRAPHICS_FLAG: 'LOW_GRAPHICS_FLAG', | |
| INITIAL_LAPTOP_CHECK: 'INITIAL_LAPTOP_CHECK' | |
| } as const; | |
| const checkGraphicsSettings = async () => { | |
| // Handle migration from old LOW_GRAPHICS_FLAG | |
| const oldLowGraphicsFlag = localStorage.getItem(STORAGE_KEYS.LOW_GRAPHICS_FLAG); | |
| if (oldLowGraphicsFlag !== null) { | |
| // Migrate old setting to new format | |
| const newSetting = oldLowGraphicsFlag === "true" ? GraphicsSettings.LOW : GraphicsSettings.HIGH; | |
| localStorage.setItem(STORAGE_KEYS.GRAPHICS_SETTING, newSetting); |
| @@ -168,6 +169,10 @@ export default class InstancedModel { | |||
| } | |||
There was a problem hiding this comment.
Remove console.log statement in removeInstance method
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores