-
Notifications
You must be signed in to change notification settings - Fork 34
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
[client] Add Minimap Component to Display Worldmap Overview #1718
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Preparing PR description... |
Preparing review... |
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 implementation of the Minimap component is well-integrated into the existing codebase and follows the project's coding style. The use of performance optimizations like throttling and caching is commendable. However, there are a few areas where improvements could be made:
- The minimap configuration could be moved to a separate file for better maintainability.
- The fixed size of the minimap (200x112 pixels) might not be suitable for all screen sizes. Consider making this responsive or user-adjustable.
- The color scheme for minimap elements is hardcoded. It might be beneficial to tie this into the game's overall theming system if one exists.
- Error handling could be improved in some areas, particularly when parsing URL parameters.
- The addition of the minimap canvas element directly in the HTML file is fine, but creating it dynamically in JavaScript might offer more flexibility in the future.
Overall, this is a solid implementation that significantly enhances the user experience by providing an overview of the game world. The integration with existing systems like the camera, structure manager, and army manager is well done.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
import { Biome, BIOME_COLORS } from "./Biome"; | ||
import { StructureManager } from "./StructureManager"; | ||
|
||
const MINIMAP_CONFIG = { |
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.
Consider moving this configuration object to a separate file for better maintainability and easier adjustments.
this.biomeCache = new Map(); | ||
this.scaledCoords = new Map(); | ||
this.computeScaledCoords(); | ||
|
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 throttle value of 1000/30 (about 33ms) is a good choice for 30 FPS updates. However, you might want to make this configurable or tie it to the game's overall frame rate if that varies.
const col = parseInt(url.searchParams.get("col") || "0"); | ||
const row = parseInt(url.searchParams.get("row") || "0"); |
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.
Consider adding error handling here in case the URL parameters are not present or not valid numbers.
const col = parseInt(url.searchParams.get("col") || "0"); | |
const row = parseInt(url.searchParams.get("row") || "0"); | |
let col, row; | |
try { | |
col = parseInt(url.searchParams.get("col") || "0"); | |
row = parseInt(url.searchParams.get("row") || "0"); | |
if (isNaN(col) || isNaN(row)) { | |
throw new Error("Invalid URL parameters"); | |
} | |
} catch (error) { | |
console.error("Error parsing URL parameters:", error); | |
col = 0; | |
row = 0; | |
} |
@@ -81,6 +81,12 @@ | |||
</head> | |||
<body> | |||
<div id="root"></div> | |||
<canvas |
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.
While adding the canvas element directly in the HTML is fine, consider creating it dynamically in JavaScript for more flexibility, especially if you plan to make the minimap size adjustable in the future.
client/src/three/scenes/Worldmap.ts
Outdated
@@ -147,6 +149,8 @@ export default class WorldmapScene extends HexagonScene { | |||
}, | |||
); | |||
|
|||
this.minimap = new Minimap(this, this.exploredTiles, this.camera, this.structureManager, this.armyManager, this.biome); |
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.
This line is quite long and might be hard to read. Consider breaking it into multiple lines for better readability.
this.minimap = new Minimap(this, this.exploredTiles, this.camera, this.structureManager, this.armyManager, this.biome); | |
this.minimap = new Minimap( | |
this, | |
this.exploredTiles, | |
this.camera, | |
this.structureManager, | |
this.armyManager, | |
this.biome, | |
); |
Summary
This PR introduces a new
Minimap
component to the project, providing a visual overview of the world map. The minimap displays explored tiles, structures, armies, and the camera's current position, enhancing the user's ability to navigate and interact with the world map.Key Features
Implementation Details
MINIMAP_CONFIG
) for customizable colors, sizes, and other settings.Usage
Minimap
class is instantiated with the world map scene, explored tiles, camera, structure manager, army manager, and biome.Future Enhancements
This new component significantly improves the user's ability to navigate and understand the world map, providing a more intuitive and interactive experience.