feat: flat mode view and optimized chunks loading #2572
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
|
You are out of MentatBot reviews. Your usage will refresh December 23 at 08:00 AM. |
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
client/src/three/scenes/Worldmap.ts (4)
372-372: **Consider dynamic scaling of maxDistance **
Currently, maxDistance is a fixed value dependent on IS_FLAT_MODE (40 vs. 20). For added flexibility, you could dynamically scale maxDistance based on screen size or device capabilities to improve user experience, especially for edge cases like ultra-wide screens or WebGL performance constraints.
490-505: **Use consistent chunk key parsing and error handling **
The approach of extracting row and column from the chunk key via split is fine, but it might be safer to add basic validation (e.g., ensure that chunkKey is in the expected format). If an unexpected chunkKey value were passed in, parseInt might yield NaN.
You could also factor out a helper function like parseChunkKey(chunkKey) to keep the code standardized across all references to chunk keys.
571-571: **Remove console logs or guard them in production **
Consider removing or conditionally wrapping console.log("cache applied") in a development/debug flag to avoid polluting production logs.
602-606: **Potential concurrency improvement for chunk fetching **
You are populating tile entities for the current chunk and the surrounding chunks, which is good for smooth user experience. However, you might consider running these in parallel using Promise.all to reduce total wait time, if the server and client can handle multiple requests at once.client/src/three/helpers/GUIManager.ts (1)
6-6: **Use triple equals to avoid unintended type coercion **
“env.VITE_PUBLIC_GRAPHICS_DEV == true” can be changed to “env.VITE_PUBLIC_GRAPHICS_DEV === true” for stricter comparison.- autoPlace: env.VITE_PUBLIC_GRAPHICS_DEV == true && !IS_MOBILE, + autoPlace: env.VITE_PUBLIC_GRAPHICS_DEV === true && !IS_MOBILE,client/src/ui/config.tsx (1)
43-44: **Consider a more explicit fallback behavior **
Currently, missing localStorage yields GraphicsSettings.HIGH by default. If this is intended, no problems; otherwise, consider prompting for user preference or defaulting to a more conservative setting like LOW on unknown devices.client/src/three/scenes/constants.ts (1)
80-82: **Flexible path handling for flat vs. base mode **
Directly setting BASE_PATH or FLAT_PATH is concise. For multi-theme or multi-mode scenarios, consider an object keyed by mode type, so new mode expansions remain readable:- const MODELS_PATH = IS_FLAT_MODE ? FLAT_PATH : BASE_PATH; + const PATHS = { + flat: FLAT_PATH, + base: BASE_PATH, + }; + const MODELS_PATH = IS_FLAT_MODE ? PATHS.flat : PATHS.base;client/src/ui/modules/settings/Settings.tsx (1)
89-98: Consider optimizing the flat mode toggle implementation.The implementation forces a full page reload when toggling flat mode. While this ensures all components reflect the change, it might impact user experience.
Consider implementing a more granular update mechanism:
- Use React context or state management to propagate the change
- Update only the affected components
- Avoid full page reload
const toggleFlatMode = () => { setIsFlatMode((prev) => { const newFlatMode = !prev; localStorage.setItem("FLAT_MODE", newFlatMode.toString()); - window.location.reload(); // Reload the page to apply changes + // Dispatch an event or update state to notify affected components + window.dispatchEvent(new CustomEvent('flatModeChanged', { detail: newFlatMode })); return newFlatMode; }); };client/src/three/scenes/Hexception.tsx (1)
275-275: Consider extracting magic numbers into named constantsThe maxDistance values should be defined as named constants for better maintainability and clarity.
+const FLAT_MODE_MAX_DISTANCE = 36; +const DEFAULT_MAX_DISTANCE = 18; - this.controls.maxDistance = IS_FLAT_MODE ? 36 : 18; + this.controls.maxDistance = IS_FLAT_MODE ? FLAT_MODE_MAX_DISTANCE : DEFAULT_MAX_DISTANCE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/src/three/GameRenderer.ts(2 hunks)client/src/three/helpers/GUIManager.ts(1 hunks)client/src/three/scenes/HexagonScene.ts(4 hunks)client/src/three/scenes/Hexception.tsx(4 hunks)client/src/three/scenes/Worldmap.ts(8 hunks)client/src/three/scenes/constants.ts(2 hunks)client/src/ui/config.tsx(2 hunks)client/src/ui/modules/settings/Settings.tsx(4 hunks)
🔇 Additional comments (14)
client/src/three/scenes/Worldmap.ts (3)
9-9: **Good addition of config imports **
Importing IS_FLAT_MODE allows for conditional scene logic for flat mode rendering. This is a clear and organized use of the configuration constants.
431-438: **Conditional rotation logic is concise **
The code neatly toggles random rotations for non-flat mode while resetting rotation in flat mode. This cleanly separates the two modes.
Consider adding a small comment to explain that the random rotation highlights biome variety or terrain variation. This can help future maintainers understand the purpose of the rotation logic.
689-693: **Validate chunk key parsing to avoid NaN errors **
Similar to the earlier comment about chunk keys, make sure chunkKey is always in a valid format ("row,col"). If it isn’t, parseInt might return NaN and cause range computations to malfunction.
Would you like a script to confirm all chunkKey references match the "number,number" pattern across the codebase?
client/src/ui/config.tsx (3)
9-9: **Enum trailing comma is acceptable **
No errors or concerns here. The trailing comma in the enum can improve diffs and is standard in many code styles.
46-48: **Function getFlatMode is straightforward **
This boolean retrieval approach is clear and easy to maintain. Just ensure you store "true"/"false" consistently in localStorage for Flat Mode.
52-52: **Exposing IS_FLAT_MODE is helpful for global usage **
client/src/three/scenes/constants.ts (3)
2-2: **Clear addition of IS_FLAT_MODE import **
Bringing in IS_FLAT_MODE for conditional resource path logic is well structured for code readability.
83-101: **BiomeFilenames enum is well-structured **
Having a single source of truth for biome file names improves maintainability and reduces potential typos in string paths.
104-120: **All references default to Grassland or TemperateDeciduousForest **
You are routing multiple biome types to the same file. If this is intentional (placeholder) it’s fine. Otherwise, confirm with a designer or map artist that reusing the same models is correct, as you might lose biome-specific variations.
client/src/three/GameRenderer.ts (1)
90-90: Verify the impact of increased far clipping plane.
Increasing the far clipping plane from 30 to 50 in flat mode could affect:
- Rendering performance due to larger view frustum
- Z-fighting artifacts for distant objects
✅ Verification successful
Let me gather more information about the rendering setup and camera usage.
The increased far clipping plane in flat mode is safe to use.
The codebase analysis reveals that the change is well-considered within the rendering architecture:
- The renderer uses
PCFSoftShadowMapand has proper depth buffer handling - No z-fighting issues are reported in the codebase
- Flat mode already has specific optimizations:
- Disabled fog effects in flat mode
- Adjusted camera controls (maxDistance: 40 vs 20)
- Simplified rotation handling
- The far plane increase (30 to 50) aligns with flat mode's extended viewing range (maxDistance: 40)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential z-fighting issues in flat mode
# Look for depth-related configurations or comments in the codebase
# Search for depth buffer configurations
rg -A 2 'depthTest|depthWrite|renderOrder'
# Search for z-fighting related comments
rg -i 'z-fighting|z fighting|depth buffer'
Length of output: 1382
Script:
#!/bin/bash
# Search for camera-related configurations and render settings
rg -A 3 'PerspectiveCamera|OrthographicCamera'
# Search for any flat mode related configurations
rg -A 3 'IS_FLAT_MODE'
# Look for renderer configurations
rg -A 3 'renderer\.set|antialias|logarithmicDepthBuffer'
Length of output: 12723
client/src/three/scenes/HexagonScene.ts (1)
70-72: LGTM: Fog configuration is well-considered.
The conditional fog application makes sense:
- Disabled in flat mode for clearer visibility
- Only enabled in high graphics mode for performance
client/src/three/scenes/Hexception.tsx (3)
8-8: LGTM: Import of flat mode configuration
The import follows proper naming conventions for configuration constants.
544-544: LGTM: Z-fighting prevention
Setting pillars slightly below the surface (-0.01) helps prevent z-fighting artifacts.
675-684: LGTM: Flat mode implementation
The implementation correctly handles both position and rotation adjustments for flat mode:
- Forces y-position to 0 in flat mode
- Removes rotation in flat mode for consistent appearance
Let's verify that flat mode is consistently implemented across other scene components:
✅ Verification successful
Flat mode implementation is consistently handled across scenes
The verification confirms that flat mode is consistently implemented:
- In Hexception.tsx and Worldmap.ts, both position.y and rotation.y are properly handled for flat mode
- Other scenes (HexagonScene.ts) correctly handle flat mode positioning
- Global constants and camera settings are properly adjusted for flat mode
- No inconsistencies found in the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential places where flat mode should be considered
rg -l "position\.y|rotation\.y" | while read -r file; do
echo "=== $file ==="
rg "position\.y|rotation\.y" "$file"
done
# Verify IS_FLAT_MODE usage consistency
ast-grep --pattern 'IS_FLAT_MODE ? $_ : $_'
Length of output: 12590
|
|
||
| const newPosition = IS_FLAT_MODE | ||
| ? new THREE.Vector3(newTarget.x, pos.y, newTarget.z) | ||
| : new THREE.Vector3(pos.x + deltaX, pos.y, pos.z + deltaZ); | ||
|
|
||
| if (duration) { | ||
| this.cameraAnimate(new THREE.Vector3(pos.x + deltaX, pos.y, pos.z + deltaZ), newTarget, duration); | ||
| this.cameraAnimate(newPosition, newTarget, duration); | ||
| } else { | ||
| target.set(newTarget.x, newTarget.y, newTarget.z); | ||
| pos.set(pos.x + deltaX, pos.y, pos.z + deltaZ); | ||
| target.copy(newTarget); | ||
| pos.copy(newPosition); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate camera movement logic.
The camera position calculation logic is duplicated between moveCameraToXYZ and moveCameraToColRow.
Extract the common logic into a shared method:
+private calculateNewCameraPosition(newTarget: THREE.Vector3, currentPosition: THREE.Vector3, currentTarget: THREE.Vector3): THREE.Vector3 {
+ if (IS_FLAT_MODE) {
+ return new THREE.Vector3(newTarget.x, currentPosition.y, newTarget.z);
+ }
+ const deltaX = newTarget.x - currentTarget.x;
+ const deltaZ = newTarget.z - currentTarget.z;
+ return new THREE.Vector3(currentPosition.x + deltaX, currentPosition.y, currentPosition.z + deltaZ);
+}
public moveCameraToXYZ(x: number, y: number, z: number, duration: number = 2) {
const newTarget = new THREE.Vector3(x, y, z);
const target = this.controls.target;
const pos = this.controls.object.position;
- const deltaX = newTarget.x - target.x;
- const deltaZ = newTarget.z - target.z;
- const newPosition = IS_FLAT_MODE
- ? new THREE.Vector3(newTarget.x, pos.y, newTarget.z)
- : new THREE.Vector3(pos.x + deltaX, pos.y, pos.z + deltaZ);
+ const newPosition = this.calculateNewCameraPosition(newTarget, pos, target);
// ... rest of the method
}
public moveCameraToColRow(col: number, row: number, duration: number = 2) {
const { x, y, z } = getWorldPositionForHex({ col, row });
const newTarget = new THREE.Vector3(x, y, z);
const target = this.controls.target;
const pos = this.controls.object.position;
- const deltaX = newTarget.x - target.x;
- const deltaZ = newTarget.z - target.z;
- const newPosition = IS_FLAT_MODE
- ? new THREE.Vector3(newTarget.x, pos.y, newTarget.z)
- : new THREE.Vector3(pos.x + deltaX, pos.y, pos.z + deltaZ);
+ const newPosition = this.calculateNewCameraPosition(newTarget, pos, target);
// ... rest of the method
}Also applies to: 347-357
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/three/scenes/constants.ts (1)
83-101: Add JSDoc documentation for the BiomeFilenames enumWhile the enum is well-structured, adding documentation would improve maintainability and help other developers understand the purpose and usage of these filenames.
Add documentation like this:
+/** + * Enum containing the filenames for different biome 3D models. + * These files are expected to exist in both the base and flat model directories. + */ export enum BiomeFilenames {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/three/scenes/constants.ts(2 hunks)
🔇 Additional comments (4)
client/src/three/scenes/constants.ts (4)
81-81: LGTM! Clean implementation of path selection logic
The ternary operator provides a clear and concise way to switch between flat and base paths based on the mode.
2-2: Verify the existence of flat models directory
The code references a new directory for flat mode models. Let's ensure this directory exists and contains the required models.
Also applies to: 80-80
✅ Verification successful
✓ Flat mode models directory and files are properly set up
The directory client/public/models/biomes-flat-opt/ exists and contains all the necessary biome model files (.glb). The models follow the same naming convention as the regular biome models, ensuring consistency across different view modes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the flat models directory exists and contains the expected models
fd . "public/models/biomes-flat-opt" --type f -e glb
Length of output: 169
Script:
#!/bin/bash
# Let's check if the directory exists in any location within the repository
fd "biomes-flat-opt" --type d
# Also check for any .glb files in the repository to understand model organization
fd . --type f -e glb
Length of output: 6478
103-120: Verify path construction consistency
Let's ensure the path construction pattern is consistent across the codebase.
✅ Verification successful
Let me gather more information about the path constants to understand the full context.
Based on the gathered information, I can now provide the final response:
Path construction is consistent with codebase patterns
The path construction in the biome models follows the established pattern in the codebase. The MODELS_PATH constant is properly defined and conditionally set based on IS_FLAT_MODE, choosing between FLAT_PATH and BASE_PATH. This is consistent with other model path constructions in the file, where paths are either concatenated using the same pattern or directly specified with string literals.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances of path construction for models
rg "models/.+\.glb" --type ts
# Check for potential path joining issues
rg "MODELS_PATH.*\+" --type ts
Length of output: 5676
Script:
#!/bin/bash
# Check the definition of MODELS_PATH and other path constants
ast-grep --pattern 'const $_ = $_' client/src/three/scenes/constants.ts
# Check if there are any path utilities being used
rg "path\." --type ts
Length of output: 3397
103-120: Document why Outline uses BASE_PATH instead of MODELS_PATH
The Outline biome specifically uses BASE_PATH while all other biomes use MODELS_PATH. This special case should be documented to explain the reasoning.
Add a comment explaining the special case:
export const biomeModelPaths: Record<BiomeType | "Outline", string> = {
// ... other biomes ...
+ // Outline uses BASE_PATH as it's shared between flat and regular modes
Outline: BASE_PATH + BiomeFilenames.Outline,
// ... remaining biomes ...
};✅ Verification successful
Let me check the usage of Outline in these files to understand why it uses BASE_PATH.
Now I can generate the final response based on the evidence found. The code shows that:
MODELS_PATHswitches betweenFLAT_PATHandBASE_PATHbased onIS_FLAT_MODE- Outline is used as a special visual element with unique properties (transparent, different render order)
- Outline is consistently treated differently from other biomes (e.g., shadow handling)
Add a comment explaining why Outline uses BASE_PATH
The Outline model should always use BASE_PATH instead of MODELS_PATH because it's a shared visual element that remains the same regardless of flat/regular mode, as evidenced by its special rendering properties in the codebase.
export const biomeModelPaths: Record<BiomeType | "Outline", string> = {
// ... other biomes ...
+ // Outline uses BASE_PATH directly as it's a shared visual element that doesn't change between flat/regular modes
Outline: BASE_PATH + BiomeFilenames.Outline,
// ... remaining biomes ...
};🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other special cases of outline handling
rg -l "Outline" --type ts
Length of output: 224
Script:
#!/bin/bash
# Check how Outline is used in the codebase
rg "Outline" --type ts -C 3
# Also check what BASE_PATH and MODELS_PATH are defined as
rg "BASE_PATH|MODELS_PATH" --type ts -C 2
Length of output: 7277
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/src/three/scenes/Worldmap.ts (3)
490-505: Consider optimizing chunk calculation logicThe chunk calculation could be improved for better performance and reliability:
- Add chunk size validation
- Use Set instead of Array to avoid duplicate checks
- Optimize the nested loops
getChunksAround(chunkKey: string) { const startRow = parseInt(chunkKey.split(",")[0]); const startCol = parseInt(chunkKey.split(",")[1]); - const chunks: string[] = []; + const chunks = new Set<string>(); + if (this.chunkSize <= 0) { + throw new Error("Invalid chunk size"); + } for (let i = -this.renderChunkSize.width / 2; i <= this.renderChunkSize.width / 2; i += this.chunkSize) { for (let j = -this.renderChunkSize.width / 2; j <= this.renderChunkSize.height / 2; j += this.chunkSize) { const { x, y, z } = getWorldPositionForHex({ row: startRow + i, col: startCol + j }); const { chunkX, chunkZ } = this.worldToChunkCoordinates(x, z); const _chunkKey = `${chunkZ * this.chunkSize},${chunkX * this.chunkSize}`; - if (!chunks.includes(_chunkKey)) { - chunks.push(_chunkKey); - } + chunks.add(_chunkKey); } } - return chunks; + return Array.from(chunks); }
649-654: Consider extracting magic number to a constantThe y-position adjustment of 0.1 in flat mode should be defined as a named constant for better maintainability.
+const FLAT_MODE_Y_OFFSET = 0.1; if (!IS_FLAT_MODE) { dummy.rotation.y = randomRotation; } else { - dummy.position.y += 0.1; + dummy.position.y += FLAT_MODE_Y_OFFSET; dummy.rotation.y = 0; }
571-571: Remove or replace debug console.logsConsider removing the debug console.log statements or replacing them with proper logging using a logging framework.
-console.log("cache applied"); +// TODO: Replace with proper logging +logger.debug("Cache applied for chunk computation"); -console.log("chunkKey", chunkKey); +logger.debug("Computing tile entities for chunk", { chunkKey });Also applies to: 604-605
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/three/scenes/Worldmap.ts(8 hunks)
🔇 Additional comments (2)
client/src/three/scenes/Worldmap.ts (2)
372-372: LGTM: Camera distance adjustment for flat mode
The conditional adjustment of maxDistance based on the view mode is appropriate, allowing for better visibility in flat mode.
431-438: LGTM: Proper rotation handling for flat mode
The conditional rotation logic appropriately handles both modes:
- Flat mode: Consistent 0-degree rotation for a clean, map-like appearance
- Normal mode: Randomized rotation for visual variety
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/three/scenes/Worldmap.ts(8 hunks)
🔇 Additional comments (3)
client/src/three/scenes/Worldmap.ts (3)
9-9: LGTM: Appropriate camera distance adjustment for flat mode
The doubled maxDistance (40 vs 20) in flat mode is a good adjustment to maintain proper visibility of the map from a top-down perspective.
Also applies to: 372-372
431-438: LGTM: Proper visual adjustments for flat mode
The changes appropriately handle hex visualization in flat mode:
- Zero rotation ensures consistent orientation
- Small Y-axis offset (+0.1) prevents Z-fighting issues
Also applies to: 651-656
603-608: Remove or implement commented chunk loading code
The commented code for loading surrounding chunks suggests an incomplete implementation. Either implement the surrounding chunk loading or remove the commented code if it's not needed.
Would you like me to help implement the surrounding chunk loading feature?
| private async computeTileEntities(chunkKey: string) { | ||
| const startCol = parseInt(chunkKey.split(",")[1]) + FELT_CENTER; | ||
| const startRow = parseInt(chunkKey.split(",")[0]) + FELT_CENTER; | ||
|
|
||
| const adjustedX = cameraPosition.x + (this.chunkSize * HEX_SIZE * Math.sqrt(3)) / 2; | ||
| const adjustedZ = cameraPosition.z + (this.chunkSize * HEX_SIZE * 1.5) / 3; | ||
|
|
||
| // Parse current chunk coordinates | ||
| const { chunkX, chunkZ } = this.worldToChunkCoordinates(adjustedX, adjustedZ); | ||
|
|
||
| const startCol = chunkX * this.chunkSize + FELT_CENTER; | ||
| const startRow = chunkZ * this.chunkSize + FELT_CENTER; | ||
| //const range = this.chunkSize / 2; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up debug code and optimize range calculation
- Remove debug console.log statements before production deployment
- The commented range calculation (
const range = this.chunkSize / 2) suggests a potential optimization opportunity compared to the current implementation usingrenderChunkSize.width / 2
Apply these changes:
- //const range = this.chunkSize / 2;
-
- const { width } = this.renderChunkSize;
- const range = width / 2;
-
- // Skip if we've already fetched this chunk
- if (this.fetchedChunks.has(chunkKey)) {
- console.log("Already fetched");
- return;
- }
-
- // Add to fetched chunks before the query to prevent concurrent duplicate requests
- this.fetchedChunks.add(chunkKey);
- console.log(startCol, startRow, range);
+ const range = this.chunkSize / 2;
+
+ if (this.fetchedChunks.has(chunkKey)) {
+ return;
+ }
+
+ this.fetchedChunks.add(chunkKey);Committable suggestion skipped: line range outside the PR's diff.
| getChunksAround(chunkKey: string) { | ||
| const startRow = parseInt(chunkKey.split(",")[0]); | ||
| const startCol = parseInt(chunkKey.split(",")[1]); | ||
| const chunks: string[] = []; | ||
| for (let i = -this.renderChunkSize.width / 2; i <= this.renderChunkSize.width / 2; i += this.chunkSize) { | ||
| for (let j = -this.renderChunkSize.width / 2; j <= this.renderChunkSize.height / 2; j += this.chunkSize) { | ||
| const { x, y, z } = getWorldPositionForHex({ row: startRow + i, col: startCol + j }); | ||
| const { chunkX, chunkZ } = this.worldToChunkCoordinates(x, z); | ||
| const _chunkKey = `${chunkZ * this.chunkSize},${chunkX * this.chunkSize}`; | ||
| if (!chunks.includes(_chunkKey)) { | ||
| chunks.push(_chunkKey); | ||
| } | ||
| } | ||
| } | ||
| return chunks; | ||
| } |
There was a problem hiding this comment.
Fix incorrect dimension in chunk calculation loop
There's a bug in the inner loop condition where it uses width instead of height, which could lead to incorrect chunk calculations.
Apply this fix:
- for (let j = -this.renderChunkSize.width / 2; j <= this.renderChunkSize.height / 2; j += this.chunkSize) {
+ for (let j = -this.renderChunkSize.height / 2; j <= this.renderChunkSize.height / 2; j += this.chunkSize) {📝 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.
| getChunksAround(chunkKey: string) { | |
| const startRow = parseInt(chunkKey.split(",")[0]); | |
| const startCol = parseInt(chunkKey.split(",")[1]); | |
| const chunks: string[] = []; | |
| for (let i = -this.renderChunkSize.width / 2; i <= this.renderChunkSize.width / 2; i += this.chunkSize) { | |
| for (let j = -this.renderChunkSize.width / 2; j <= this.renderChunkSize.height / 2; j += this.chunkSize) { | |
| const { x, y, z } = getWorldPositionForHex({ row: startRow + i, col: startCol + j }); | |
| const { chunkX, chunkZ } = this.worldToChunkCoordinates(x, z); | |
| const _chunkKey = `${chunkZ * this.chunkSize},${chunkX * this.chunkSize}`; | |
| if (!chunks.includes(_chunkKey)) { | |
| chunks.push(_chunkKey); | |
| } | |
| } | |
| } | |
| return chunks; | |
| } | |
| getChunksAround(chunkKey: string) { | |
| const startRow = parseInt(chunkKey.split(",")[0]); | |
| const startCol = parseInt(chunkKey.split(",")[1]); | |
| const chunks: string[] = []; | |
| for (let i = -this.renderChunkSize.width / 2; i <= this.renderChunkSize.width / 2; i += this.chunkSize) { | |
| for (let j = -this.renderChunkSize.height / 2; j <= this.renderChunkSize.height / 2; j += this.chunkSize) { | |
| const { x, y, z } = getWorldPositionForHex({ row: startRow + i, col: startCol + j }); | |
| const { chunkX, chunkZ } = this.worldToChunkCoordinates(x, z); | |
| const _chunkKey = `${chunkZ * this.chunkSize},${chunkX * this.chunkSize}`; | |
| if (!chunks.includes(_chunkKey)) { | |
| chunks.push(_chunkKey); | |
| } | |
| } | |
| } | |
| return chunks; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/three/scenes/Worldmap.ts (1)
572-572: Remove debug loggingDebug console.log statements should be removed before merging to production.
- console.log("cache applied");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/three/scenes/Worldmap.ts(8 hunks)
🔇 Additional comments (4)
client/src/three/scenes/Worldmap.ts (4)
372-372: LGTM: Camera distance adjustment for flat mode
The different maximum distances for flat mode (40) and regular mode (20) are appropriate for their respective viewing perspectives.
431-439: LGTM: Appropriate rotation handling for flat mode
The code correctly handles rotations:
- Regular mode: Uses seeded random rotations for visual variety
- Flat mode: Zeroes rotation and slightly elevates position for better visibility
496-496: Fix incorrect dimension in chunk calculation loop
The inner loop condition incorrectly uses width instead of height.
604-609: Clarify the commented chunk computation implementation
The commented code for computing surrounding chunks suggests an unfinished implementation. This could be important for the chunk loading optimization mentioned in the PR objectives.
Please clarify if this implementation should be:
- Uncommented and completed
- Removed if no longer needed
✅ Verification successful
Let me gather more information about the chunk computation implementation and its usage in the codebase.
Based on the search results, I can now generate the final response as I have enough context about the chunk computation implementation.
Keep the chunk computation code commented out
The commented code for computing surrounding chunks should remain commented out because:
- The current implementation already handles chunk computation efficiently through
updateVisibleChunks()which calculates and loads chunks based on camera position - The
getChunksAround()method's implementation shows it would process a much larger area (renderChunkSize.width x renderChunkSize.height) which could impact performance - The existing chunk loading system includes proper chunk caching (
fetchedChunks), matrix caching, and optimized updates for armies and structures
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for chunk-related computations and methods
ast-grep --pattern 'computeTileEntities($_) {
$$$
}'
# Search for getChunksAround implementation
ast-grep --pattern 'getChunksAround($_) {
$$$
}'
# Search for other chunk-related methods
rg "chunk" client/src/three/scenes/Worldmap.ts -A 5
Length of output: 6706
r0man1337
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/src/ui/modules/settings/Settings.tsx (3)
76-96: Add error handling for localStorage operationsWhile the guild whitelist implementation is clean, consider adding try-catch blocks around localStorage operations to handle potential errors (e.g., when storage is full or disabled).
const [selectedGuilds, setSelectedGuilds] = useState<string[]>(() => { - const savedGuilds = localStorage.getItem("WHITELIST"); - return savedGuilds ? savedGuilds.split(",") : []; + try { + const savedGuilds = localStorage.getItem("WHITELIST"); + return savedGuilds ? savedGuilds.split(",") : []; + } catch (error) { + console.error("Failed to load guild whitelist:", error); + return []; + } }); const handleGuildSelect = (guildId: string) => { setSelectedGuilds((prev) => { const newGuilds = prev.includes(guildId) ? prev.filter((id) => id !== guildId) : [...prev, guildId]; - localStorage.setItem("WHITELIST", newGuilds.join(",")); + try { + localStorage.setItem("WHITELIST", newGuilds.join(",")); + toast(prev.includes(guildId) ? "Guild removed from whitelist!" : "Guild added to whitelist!"); + } catch (error) { + console.error("Failed to save guild whitelist:", error); + toast.error("Failed to save guild selection"); + } return newGuilds; }); };
98-107: Enhance UX during flat mode toggleThe page reload approach works but might be jarring for users. Consider these improvements:
- Show a loading overlay during the reload
- Save any unsaved user state before reload
- Add a confirmation dialog for better user experience
const toggleFlatMode = () => { + if (!confirm("Changing view mode will reload the page. Continue?")) { + return; + } setIsFlatMode((prev) => { const newFlatMode = !prev; - localStorage.setItem("FLAT_MODE", newFlatMode.toString()); - window.location.reload(); + try { + localStorage.setItem("FLAT_MODE", newFlatMode.toString()); + setBlankOverlay(true); // Show loading overlay + window.location.reload(); + } catch (error) { + console.error("Failed to save flat mode setting:", error); + toast.error("Failed to change view mode"); + } return newFlatMode; }); };
170-194: Consider pagination for guild listThe guild whitelist UI implementation looks clean, but consider adding pagination or virtualization if the list of guilds grows large. This would improve performance and prevent potential UI issues with a large number of guilds.
Consider using a virtualized list component like
react-windoworreact-virtualizedfor better performance with large lists:import { FixedSizeList } from 'react-window'; const GuildList = ({ guilds, selectedGuilds, onSelect }) => { const Row = ({ index, style }) => ( <Button style={style} size="xs" key={guilds[index].entityId} variant={selectedGuilds.includes(guilds[index].entityId.toString()) ? "success" : "outline"} onClick={() => onSelect(guilds[index].entityId.toString())} > {guilds[index].name} </Button> ); return ( <FixedSizeList height={200} itemCount={guilds.length} itemSize={35} width="100%" > {Row} </FixedSizeList> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/ui/modules/settings/Settings.tsx(3 hunks)
🔇 Additional comments (1)
client/src/ui/modules/settings/Settings.tsx (1)
15-15: LGTM: Clean import addition
The import of IS_FLAT_MODE is properly organized alongside existing graphics settings imports.
The client now manages chunk loading more efficiently. Previously, loading was not optimal because each camera movement would reload many entities. Now, entities are only loaded from chunks that were not previously on the screen. A flat mode view has also been added.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation