vercel[bot]
commented
2 days ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| eternum | ❌ Failed (Inspect) | Jul 1, 2024 8:17am |
Open ponderingdemocritus opened 2 days ago
vercel[bot]
commented
2 days ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| eternum | ❌ Failed (Inspect) | Jul 1, 2024 8:17am |
github-actions[bot]
commented
2 days ago | ⏱️ Estimated effort to review [1-5] | 4 |
| 🧪 Relevant tests | No |
| 🔒 Security concerns | No |
| ⚡ Key issues to review |
Possible Bug: The Demo class in client/src/three/Demo.ts has methods like initScene, initStats, and initListeners that are commented out in the constructor but are called in client/src/main.tsx. This might lead to runtime errors if the methods are not properly initialized before being used. |
|
Redundancy: In client/src/three/Demo.ts, the initListeners method has multiple listeners for the 'keydown' event which could be combined into a single listener to streamline the code and improve performance. | |
|
Performance Concern: The animate method in client/src/three/Demo.ts is recursively calling itself without any clear condition to stop. This could lead to performance issues due to the stacking of animation frames. |
github-actions[bot]
commented
2 days ago | Category | Suggestion | Score |
| Possible bug |
Add null checks before cloning biome models to prevent runtime errors___ **To avoid potential runtime errors from null references, check ifthis.biomeModels.get(biome) returns a valid object before calling .clone() on it.**
[client/src/three/objects/HexagonMap.ts [215]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-21264d2475dc954c34dfcd823e1e221dabace64ba4177aad378dc10a74c0bd22R215-R215)
```diff
-const hexModel = this.biomeModels.get(biome)!.clone();
+const biomeModel = this.biomeModels.get(biome);
+if (biomeModel) {
+ const hexModel = biomeModel.clone();
+ hexModel.position.set(dummy.position.x, 0, dummy.position.z);
+ group.add(hexModel);
+}
```
Suggestion importance[1-10]: 10Why: Adding null checks before cloning biome models is crucial to prevent potential runtime errors, ensuring the robustness of the code. | 10 |
| Enhancement |
Update the
___
**Update the | 9 |
Prevent event bubbling in context menu handling___ **Consider usingevent.stopPropagation() in the showContextMenuForHexagon method to prevent the event from bubbling up and potentially triggering other event handlers that might not be intended to handle this right-click event.** [client/src/three/components/ContextMenuManager.ts [50]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-d0be667b9523a5406b23ae60bcc66e0f283bb12053955dd4eed15b3ab74bc2bfR50-R50) ```diff event.preventDefault(); +event.stopPropagation(); ``` Suggestion importance[1-10]: 8Why: Adding `event.stopPropagation()` is a good practice to prevent unintended side effects from event bubbling, which can improve the robustness of the context menu functionality. | 8 | |
Replace fragment shorthand with
___
**Replace the fragment shorthand ( | 5 | |
| Error handling |
Add error handling in the constructor to ensure valid dojo context is provided___ **Implement error handling for theDemo class constructor and initialization methods to gracefully handle failures in the setup process, such as when dojoContext is null or when the setup method fails.**
[client/src/three/Demo.ts [38-47]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-0387d2afa5d298b19c51c3730841a38ec6246acc5dbc4c0e8f8c096cb5205da8R38-R47)
```diff
constructor(dojoContext: SetupResult) {
+ if (!dojoContext) {
+ throw new Error('Invalid dojo context provided.');
+ }
this.dojo = dojoContext;
}
```
Suggestion importance[1-10]: 9Why: Adding error handling in the constructor is crucial for ensuring that the `dojoContext` is valid, which can prevent potential runtime errors and improve the robustness of the application. | 9 |
| Best practice |
Ensure proper cleanup of event listeners___ **To improve the performance and avoid potential memory leaks, consider removing eventlisteners when the ContextMenuManager is destroyed or no longer needed.**
[client/src/three/components/ContextMenuManager.ts [146-151]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-d0be667b9523a5406b23ae60bcc66e0f283bb12053955dd4eed15b3ab74bc2bfR146-R151)
```diff
-document.addEventListener('mousedown', this.onMouseDown.bind(this));
-document.addEventListener('mouseup', this.onMouseUp.bind(this));
-document.addEventListener('mousemove', this.onMouseMove.bind(this));
-document.addEventListener('click', this.hideContextMenu.bind(this));
-window.addEventListener('click', this.onMouseClick.bind(this));
+// Add these in a method called removeEventListeners
+document.removeEventListener('mousedown', this.onMouseDown.bind(this));
+document.removeEventListener('mouseup', this.onMouseUp.bind(this));
+document.removeEventListener('mousemove', this.onMouseMove.bind(this));
+document.removeEventListener('click', this.hideContextMenu.bind(this));
+window.removeEventListener('click', this.onMouseClick.bind(this));
```
Suggestion importance[1-10]: 9Why: Removing event listeners when they are no longer needed is crucial for preventing memory leaks and ensuring optimal performance. | 9 |
Ensure type safety and correctness for biome model paths___ **Consider using a more specific type for thebiomeModelPaths object to ensure that all biome types are covered and no typos exist in biome names. This can be achieved by using the BiomeType as the key type for the Record.**
[client/src/three/objects/HexagonMap.ts [72-81]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-21264d2475dc954c34dfcd823e1e221dabace64ba4177aad378dc10a74c0bd22R72-R81)
```diff
+const biomeModelPaths: RecordSuggestion importance[1-10]: 8Why: Using `BiomeType` as the key type for the `Record` ensures that all biome types are covered and prevents typos, improving type safety and correctness. | 8 | |
Standardize the version of
___
**Ensure that the version of | 8 | |
Use template literals for constructing keys___ **Use template literals for constructingchunkKey to enhance readability and reduce potential errors in string concatenation.** [client/src/three/objects/HexagonMap.ts [309]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-21264d2475dc954c34dfcd823e1e221dabace64ba4177aad378dc10a74c0bd22R309-R309) ```diff +const chunkKey = `${x},${z}`; - ``` Suggestion importance[1-10]: 6Why: Using template literals for constructing `chunkKey` enhances readability and reduces potential errors in string concatenation, but it is a minor improvement. | 6 | |
| Maintainability |
Update the
___
**Update the | 9 |
Improve code structure by separating raycasting logic___ **Refactor theshowContextMenuForHexagon method to separate concerns by moving the raycasting logic to a new method. This change improves code readability and maintainability.** [client/src/three/components/ContextMenuManager.ts [54-63]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-d0be667b9523a5406b23ae60bcc66e0f283bb12053955dd4eed15b3ab74bc2bfR54-R63) ```diff -this.raycaster.setFromCamera(this.mouse, this.camera); -const intersects: THREE.Intersection[] = []; -this.loadedChunks.forEach((chunk) => { - chunk.children.forEach((child) => { - if (child instanceof THREE.InstancedMesh) { - this.raycaster.intersectObject(child, false, intersects); - } +const intersects = this.getIntersects(); +// New method +private getIntersects(): THREE.Intersection[] { + this.raycaster.setFromCamera(this.mouse, this.camera); + const intersects: THREE.Intersection[] = []; + this.loadedChunks.forEach((chunk) => { + chunk.children.forEach((child) => { + if (child instanceof THREE.InstancedMesh) { + this.raycaster.intersectObject(child, false, intersects); + } + }); }); -}); + return intersects; +} ``` Suggestion importance[1-10]: 8Why: Refactoring the raycasting logic into a separate method enhances code readability and maintainability, making it easier to manage and understand. | 8 | |
Refactor biome determination into a separate method for cleaner code___ **Refactor the repeated biome determination logic into a separate method to improve codereadability and maintainability.** [client/src/three/objects/HexagonMap.ts [159-168]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-21264d2475dc954c34dfcd823e1e221dabace64ba4177aad378dc10a74c0bd22R159-R168) ```diff -let biome: BiomeType; -if (noise > 0.85) biome = "snow"; -else if (noise > 0.7) biome = "mountain"; -... -else biome = "sea"; +let biome: BiomeType = this.determineBiome(noise); +private determineBiome(noise: number): BiomeType { + if (noise > 0.85) return "snow"; + else if (noise > 0.7) return "mountain"; + ... + else return "sea"; +} + ``` Suggestion importance[1-10]: 7Why: Refactoring the biome determination logic into a separate method improves code readability and maintainability, although it is not critical for functionality. | 7 | |
| Possible issue |
Update the
___
**Consider updating the version of | 9 |
| Responsiveness |
Use container dimensions instead of window dimensions for setting renderer size to improve responsiveness___ **Replace the direct manipulation ofwindow.innerWidth and window.innerHeight with a more responsive approach using CSS for setting the renderer size, which can handle changes in viewport size more gracefully.** [client/src/three/Demo.ts [69]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-0387d2afa5d298b19c51c3730841a38ec6246acc5dbc4c0e8f8c096cb5205da8R69-R69) ```diff -this.renderer.setSize(window.innerWidth, window.innerHeight); +const container = document.getElementById('three-container') || document.body; +this.renderer.setSize(container.clientWidth, container.clientHeight); ``` Suggestion importance[1-10]: 8Why: This suggestion improves the responsiveness of the application by using container dimensions, which is a significant enhancement for handling viewport size changes gracefully. | 8 |
| Performance |
Optimize position updates by checking distance before interpolating___ **Optimize theupdate method by checking if the character is moving before calculating the position interpolation, to avoid unnecessary computations when the character is stationary.** [client/src/three/components/Character.ts [116-133]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-63711459a822a41c214c063e47bfc37cac9c4039feb6e244e5dd724ad95352a9R116-R133) ```diff if (this.isMoving && this.model) { - const t = Math.min(1, this.moveSpeed * deltaTime); - this.model.position.lerp(this.targetPosition, t); + if (this.model.position.distanceTo(this.targetPosition) > 0.01) { + const t = Math.min(1, this.moveSpeed * deltaTime); + this.model.position.lerp(this.targetPosition, t); + } else { + this.isMoving = false; + this.model.position.copy(this.targetPosition); + if (this.walkAction) { + this.walkAction.paused = true; // Stop walking animation + } + } } ``` Suggestion importance[1-10]: 7Why: This optimization reduces unnecessary computations when the character is stationary, improving performance. However, the performance gain might be minor depending on the frequency of updates. | 7 |
Optimize performance by binding methods in the constructor___ **Instead of usingbind(this) directly in the event listener, which creates a new function each time, consider binding these methods in the constructor and using the bound methods as event listeners. This change enhances performance by avoiding the creation of new functions on each render.** [client/src/three/components/ContextMenuManager.ts [146]](https://github.com/BibliothecaDAO/eternum/pull/1057/files#diff-d0be667b9523a5406b23ae60bcc66e0f283bb12053955dd4eed15b3ab74bc2bfR146-R146) ```diff -document.addEventListener('mousedown', this.onMouseDown.bind(this)); +// In the constructor: +this.boundOnMouseDown = this.onMouseDown.bind(this); +// Replace the existing line with: +document.addEventListener('mousedown', this.boundOnMouseDown); ``` Suggestion importance[1-10]: 7Why: Binding methods in the constructor can improve performance by avoiding the creation of new functions on each render, although the performance gain might be minor in this context. | 7 |
User description
PR Type
Enhancement, Dependencies
Description
Democlass for Three.js scene management.Character,ContextMenuManager,FogManager,Grass,Roads,SandDunes, andTrees.HexagonMapclass for managing hexagon grid and biomes.WorldandOnboardingcomponents for improved rendering and interaction.threepackage to version0.166.0and corresponding lock file.Changes walkthrough 📝
17 files
App.tsx
Simplified `App` component structureclient/src/App.tsx - Removed redundant `div` wrapper in `App` component.
main.tsx
Integrated `Demo` class into main initializationclient/src/main.tsx - Added initialization and setup for new `Demo` class.
Demo.ts
Added `Demo` class for Three.js scene managementclient/src/three/Demo.ts
Democlass with Three.js scene setup, stats, and eventlisteners.
Character.ts
Added `Character` class for character managementclient/src/three/components/Character.ts
Characterclass for managing character model and movement.ContextMenuManager.ts
Added `ContextMenuManager` for context menu handlingclient/src/three/components/ContextMenuManager.ts
ContextMenuManagerclass for handling context menuinteractions.
Fog.ts
Added `FogManager` for scene fog managementclient/src/three/components/Fog.ts - Created `FogManager` class for managing scene fog.
Grass.ts
Added `Grass` class for grass generationclient/src/three/components/Grass.ts - Created `Grass` class for generating grass instances.
HexagonGrid.ts
Added placeholder for `HexagonGrid` classclient/src/three/components/HexagonGrid.ts - Created placeholder `HexagonGrid` class.
InputManager.ts
Added placeholder for `InputManager` classclient/src/three/components/InputManager.ts - Created placeholder `InputManager` class.
Menu.ts
Added `Menu` class for UI menu managementclient/src/three/components/Menu.ts - Created `Menu` class for managing UI menu.
Roads.ts
Added `Roads` class for road generationclient/src/three/components/Roads.ts - Created `Roads` class for generating road instances.
SandDunes.ts
Added `SandDunes` class for sand dune generationclient/src/three/components/SandDunes.ts - Created `SandDunes` class for generating sand dune instances.
Trees.ts
Added `Trees` class for tree generationclient/src/three/components/Trees.ts - Created `Trees` class for generating tree instances.
HexagonMap.ts
Added `HexagonMap` class for hexagon grid managementclient/src/three/objects/HexagonMap.ts - Created `HexagonMap` class for managing hexagon grid and biomes.
Hexception.ts
Added `DetailedHexScene` class for detailed hexagon viewclient/src/three/objects/Hexception.ts
DetailedHexSceneclass for detailed hexagon scene management.Onboarding.tsx
Enabled pointer events for onboarding screenclient/src/ui/layouts/Onboarding.tsx - Enabled pointer events for onboarding screen.
World.tsx
Updated `World` component for conditional renderingclient/src/ui/layouts/World.tsx - Updated `World` component to conditionally render elements.
2 files
package.json
Updated `three` package versionclient/package.json - Updated `three` package version to `0.166.0`.
pnpm-lock.yaml
Updated lock file for `three` package versionpnpm-lock.yaml - Updated lock file to reflect new `three` package version.