new movement #2691
new movement #2691
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request refactors the travel path and biome management systems across client and core modules. The changes update interfaces and method signatures to use a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as UI/Update
participant AM as ArmyManager
participant FP as findShortestPath
participant EM as ArmyMovementManager
participant UI as UI Components
U->>AM: onUpdate(update, armyHexes, structureHexes, exploredTiles)
AM->>FP: findShortestPath(oldPosition, newPosition, exploredTiles, structureHexes, armyHexes, maxDistance)
FP-->>AM: Returns HexTileInfo[] path
AM->>EM: moveArmy(entityId, hexCoords, armyHexes, structureHexes, exploredTiles)
EM->>EM: Calculate troop type & stamina cost using biome info
UI->>UI: Render action info & stamina cost based on travel path
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
client/apps/game/src/three/managers/army-manager.tsOops! Something went wrong! :( ESLint: 9.20.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by client/apps/game/src/three/scenes/worldmap.tsxOops! Something went wrong! :( ESLint: 9.20.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by client/apps/game/src/three/systems/system-manager.tsOops! Something went wrong! :( ESLint: 9.20.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
✨ Finishing Touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
client/apps/game/src/hooks/store/_three-store.ts (1)
53-54:⚠️ Potential issueFix type inconsistency in
updateTravelPathsmethod.The
travelPathsproperty in theArmyActionsinterface has been updated to useHexTileInfo[], but theupdateTravelPathsmethod still usesHexPosition[]. This mismatch could lead to runtime errors.Apply this diff to fix the type inconsistency:
- updateTravelPaths: (travelPaths: Map<string, { path: HexPosition[]; isExplored: boolean }>) => + updateTravelPaths: (travelPaths: Map<string, { path: HexTileInfo[]; isExplored: boolean }>) =>
🧹 Nitpick comments (25)
client/apps/game/src/three/helpers/pathfinding.ts (1)
13-18: Add inline documentation for newly added parameters.While the updated function signature looks good, it might be helpful to briefly document what
structureHexes,armyHexes, andmaxDistancerepresent and how they're intended to be used. This will improve code readability and maintainability.packages/core/src/utils/biome/biome.ts (1)
1-91: Consider replacing the static-only class with pure functions.The entire class contains only static methods. A collection of free-standing functions or a single object with named exports might be simpler and align better with functional design principles. Lint tools typically flag static-only classes as unnecessary overhead.
🧰 Tools
🪛 Biome (1.9.4)
[error] 24-90: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/core/src/utils/biome/simplex-noise.ts (1)
34-126: Consider breaking down the largenoisefunction.This function is quite extensive and contains multiple steps (corner calculations, permutations, gradients, normalization, etc.). Splitting it into smaller helper functions (e.g., for gradient computation, corner permutations) could improve readability and ease future maintenance.
packages/core/src/utils/biome/fixed-point.ts (3)
66-382: Avoid classes containing only static members
FixedTraitincludes only static methods. The static analysis hints recommend using simpler standalone functions or a namespace in TypeScript instead of a static-only class to improve maintainability and clarity.🧰 Tools
🪛 Biome (1.9.4)
[error] 66-382: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
92-94: Consider returning a plain number or bigint fromtoIntCurrently,
toIntreturns aFixedobject, which may be misleading since the method name suggests obtaining an integer value. This can cause confusion in client code expecting a numeric return type.
304-309: Validate edge case forMIN_64x64inabsThrowing an error for
MIN_64x64inabs()mirrors the original library’s approach, but be sure it’s tested and properly handled to prevent unexpected runtime errors if such extreme values appear in normal usage.packages/core/src/managers/army-movement-manager.ts (1)
157-249: Improve priority queue efficiencySorting
priorityQueueeach iteration is less efficient than using a proper min-heap. Consider a more optimal priority queue structure to avoid performance bottlenecks in pathfinding with large data sets.client/apps/game/src/three/managers/army-manager.ts (2)
302-308: ExpandedmoveArmysignatureAdding
armyHexes,structureHexes, andexploredTilestomoveArmysupports more advanced pathfinding. Ensure upstream code passing these arguments is thoroughly updated to avoid runtime issues.
317-318: Externalize the “20” path limitThe
findShortestPathcall uses a hardcoded maximum distance of 20. Converting it into a constant or configuration value clarifies its purpose and simplifies future adjustments.client/apps/game/src/three/scenes/worldmap.tsx (3)
57-59: New data structures for map-based state
DeclaringexploredTiles,armyHexes,structureHexes, andarmiesPositionsclarifies the separation of tile vs. entity states. If these maps grow large, consider a more memory-friendly approach (e.g., partial caching or chunked retrieval) to maintain performance at scale.Also applies to: 61-61
457-471:updateStructureHexesmethod
The logic for mapping updated structure positions is straightforward. Consider an optional removal step if structures can move or be destroyed.
585-585: EmptyremoveCachedMatricesAroundColRow
This stub remains unimplemented. If it's not needed, consider removing it. Otherwise, add a TODO comment or implement the intended cleanup logic to keep code consistent.packages/core/src/utils/travel-path.ts (1)
38-40: Normalize position consistently with posKey method.The
isHighlightedmethod addsFELT_CENTERto the input coordinates, butposKeyhas anormalizedparameter. Consider usingposKeyconsistently.isHighlighted(row: number, col: number): boolean { - return this.paths.has(TravelPaths.posKey({ col: col + FELT_CENTER, row: row + FELT_CENTER })); + return this.paths.has(TravelPaths.posKey({ col, row }, true));packages/core/src/utils/biome/vec3.ts (2)
17-22: Optimize dot product calculation.The
dotmethod creates unnecessary intermediate variables. Consider chaining the operations directly.dot(other: Vec3): Fixed { - let x = this.x.mul(other.x); - let y = this.y.mul(other.y); - let z = this.z.mul(other.z); - return x.add(y).add(z); + return this.x.mul(other.x).add(this.y.mul(other.y)).add(this.z.mul(other.z));
48-50: Fix method name typo:remScaler→remScalar.The method name has a typo and is inconsistent with other scalar operation methods.
-remScaler(scalar: Fixed): Vec3 { +remScalar(scalar: Fixed): Vec3 {client/apps/game/src/ui/elements/stamina-resource-cost.tsx (1)
21-37: Optimize memoization by separating total cost calculation.The
totalCostcalculation can be memoized separately to avoid recalculating when onlystaminaorisExploredchanges.+const useTotalCost = (path: HexTileInfo[]) => + useMemo(() => path.reduce((acc, tile) => acc + tile.staminaCost, 0), [path]); const pathInfo = useMemo(() => { if (!stamina) return; - - // Calculate total cost and collect biome info - const totalCost = path.reduce((acc, tile) => { - return acc + tile.staminaCost; - }, 0); + const totalCost = useTotalCost(path); const balanceColor = stamina.amount < totalCost ? "text-order-giants" : "text-order-brilliance"; return { isExplored, totalCost, balanceColor, balance: stamina.amount, }; -}, [stamina, path, isExplored]); +}, [stamina, isExplored, useTotalCost(path)]);packages/core/src/utils/biome/vec4.ts (2)
19-25: Optimize dot product calculation.The
dotmethod creates unnecessary intermediate variables. Consider chaining the operations directly.dot(other: Vec4): Fixed { - let x = this.x.mul(other.x); - let y = this.y.mul(other.y); - let z = this.z.mul(other.z); - let w = this.w.mul(other.w); - return x.add(y).add(z).add(w); + return this.x.mul(other.x) + .add(this.y.mul(other.y)) + .add(this.z.mul(other.z)) + .add(this.w.mul(other.w));
51-53: Fix method name typo:remScaler→remScalar.The method name has a typo and is inconsistent with other scalar operation methods.
-remScaler(scalar: Fixed): Vec4 { +remScalar(scalar: Fixed): Vec4 {client/apps/game/src/ui/components/worldmap/armies/action-info.tsx (1)
135-135: LGTM! Enhanced null checking.The additional null check for
travelPathimproves robustness by preventing potential runtime errors.Consider extracting the condition into a descriptive variable for better readability:
- if (!showTooltip || !selectedEntityId || !travelPath) return null; + const canShowTooltip = showTooltip && selectedEntityId && travelPath; + if (!canShowTooltip) return null;client/apps/game/src/three/systems/system-manager.ts (2)
284-285: Remove debugging console.log statement.The console.log statement appears to be a debugging artifact and should be removed.
- console.log("newState", newState); -
290-290: Simplify biome fallback logic.The biome fallback logic can be simplified using the nullish coalescing operator.
- biome: newState?.biome === "None" ? BiomeType.Grassland : newState?.biome || BiomeType.Grassland, + biome: newState?.biome === "None" || !newState?.biome ? BiomeType.Grassland : newState.biome,client/apps/game/src/three/managers/minimap.ts (1)
223-223: Add type safety to biome color access.Add type assertion or validation to ensure the biome exists in
BIOME_COLORS.- biomeColor = BIOME_COLORS[biome].getStyle(); + biomeColor = BIOME_COLORS[biome]?.getStyle() ?? BIOME_COLORS[BiomeType.Grassland].getStyle();packages/core/src/types/common.ts (2)
166-171: LGTM! Well-structured type definition.The
HexTileInfotype appropriately extendsHexPositionwith stamina cost and biome information, which is useful for pathfinding and movement mechanics.Consider adding JSDoc comments to document the purpose and usage of this type:
+/** + * Represents a hex tile with additional information for pathfinding and movement. + * @property col - The column coordinate of the hex + * @property row - The row coordinate of the hex + * @property staminaCost - The stamina cost to move through this hex + * @property biomeType - The type of biome in this hex, if known + */ export type HexTileInfo = { col: number; row: number; staminaCost: number; biomeType: BiomeType | undefined; };
218-222: LGTM! Clear and concise enum definition.The
TroopTypeenum follows TypeScript best practices with clear, descriptive values.Consider adding JSDoc comments to document the purpose and usage of this enum:
+/** + * Represents the different types of troops available in the game. + * Used for army composition and combat mechanics. + */ export enum TroopType { Knight = "Knight", Crossbowman = "Crossbowman", Paladin = "Paladin", }client/apps/game/src/three/scenes/hexception.tsx (1)
619-621: Consider defining the buildable area biome as a constant.The hardcoded string "Grassland" should be defined as a constant for better maintainability and type safety.
+const BUILDABLE_AREA_BIOME: BiomeType = "Grassland"; const biome = Biome.getBiome(targetHex.col, targetHex.row); -const buildableAreaBiome = "Grassland"; +const buildableAreaBiome = BUILDABLE_AREA_BIOME; const isFlat = biome === "Ocean" || biome === "DeepOcean" || isMainHex;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
client/apps/game/src/hooks/store/_three-store.ts(2 hunks)client/apps/game/src/three/helpers/pathfinding.ts(4 hunks)client/apps/game/src/three/managers/army-manager.ts(8 hunks)client/apps/game/src/three/managers/biome-colors.ts(1 hunks)client/apps/game/src/three/managers/biome.ts(0 hunks)client/apps/game/src/three/managers/minimap.ts(5 hunks)client/apps/game/src/three/scenes/constants.ts(1 hunks)client/apps/game/src/three/scenes/hexagon-scene.ts(1 hunks)client/apps/game/src/three/scenes/hexception.tsx(3 hunks)client/apps/game/src/three/scenes/worldmap.tsx(10 hunks)client/apps/game/src/three/systems/system-manager.ts(2 hunks)client/apps/game/src/three/types/systems.ts(2 hunks)client/apps/game/src/ui/components/worldmap/armies/action-info.tsx(4 hunks)client/apps/game/src/ui/elements/stamina-resource-cost.tsx(1 hunks)packages/core/src/managers/army-movement-manager.ts(6 hunks)packages/core/src/managers/config-manager.ts(1 hunks)packages/core/src/types/common.ts(3 hunks)packages/core/src/utils/biome/biome.ts(1 hunks)packages/core/src/utils/biome/fixed-point.ts(1 hunks)packages/core/src/utils/biome/index.ts(1 hunks)packages/core/src/utils/biome/simplex-noise.ts(1 hunks)packages/core/src/utils/biome/vec3.ts(1 hunks)packages/core/src/utils/biome/vec4.ts(1 hunks)packages/core/src/utils/index.ts(2 hunks)packages/core/src/utils/travel-path.ts(1 hunks)
💤 Files with no reviewable changes (1)
- client/apps/game/src/three/managers/biome.ts
✅ Files skipped from review due to trivial changes (3)
- packages/core/src/utils/biome/index.ts
- packages/core/src/utils/index.ts
- client/apps/game/src/three/scenes/constants.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/utils/biome/biome.ts
[error] 24-90: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/core/src/utils/biome/fixed-point.ts
[error] 66-382: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (26)
client/apps/game/src/three/helpers/pathfinding.ts (4)
20-26: Good approach: early return when target is too far.Ensuring we bail out immediately if the distance exceeds
maxDistancehelps mitigate unnecessary pathfinding computations. This is a solid performance safeguard.
58-61: Verify path length check.Skipping further neighbors when
current.g + 1 > maxDistanceensures we don't exceed the cap, but please confirm thatghere aligns exactly with your intended “current path distance” measure. Ifgis updated in other places (e.g., terrain cost), it might deviate from a simple one-step increment.
69-71: Avoiding structure hexes.It’s correct to disallow movement onto structure hexes. Ensure external calls (e.g., from UI or AI logic) gracefully handle the situation where no valid path exists.
73-76: Check for blocking army hex.Skipping army hexes unless it’s the target location prevents collisions between units, which is beneficial. Ensure you have test cases covering forced collisions, if any are allowed in special conditions.
packages/core/src/utils/biome/fixed-point.ts (1)
205-216: Clarify “overflow behavior” usageThe comments reference relying on “overflow behavior,” but in TypeScript with
BigInt, integer overflow does not occur in the same manner as in lower-level or Solidity-based code. This can lead to confusion; consider revising or documenting the intended behavior before finalizing.Do you want a script to locate and examine all “overflow behavior” references in the codebase?
packages/core/src/managers/army-movement-manager.ts (3)
44-57: Confirm tie-breaking in_getTroopTypeThe method selects the most numerous troop type, defaulting to Paladin when counts are equal. Ensure this tie-breaking strategy matches the game design requirements.
86-98: Cross-check “fix max hex cost” objectiveThe logic calculates a maximum number of steps based on
configManager.getMinTravelStaminaCost(). Confirm this truly implements the intended “fix” for maximum travel distance in line with the PR’s purpose.
115-155: Biome-specific stamina drain appears correct
staminaDrainintroduces well-structured cost modifiers for each biome, considering troop types. The approach is clear, and changes look consistent.client/apps/game/src/three/managers/army-manager.ts (1)
118-123: Verify usage of new parameters inonUpdateThe expanded method signature uses
armyHexes,structureHexes, andexploredTiles. Confirm that these parameters are consistently handled or forwarded to all relevant logic to avoid mismatches.client/apps/game/src/three/scenes/worldmap.tsx (9)
22-22: Confirm necessity ofBiomeimport
The import is only used once in this file to fetch biome data (line 727). If this usage remains minimal, consider directly referencing that static method where needed to avoid unnecessary overhead.
28-28: ValidateStaminaManagerusage
Ensure robust error handling if no stamina data is found or ifgetStaminareturns undefined.
38-38: Imports from new system types
The import ofArmySystemUpdate,StructureSystemUpdate, andTileSystemUpdateappropriately centralizes system update definitions. Good job keeping them in one place.
119-119: Updated ArmyManager instantiation
The manager no longer needsexploredTilesor other references in the constructor, aligning with the new approach of passing them throughonUpdate. This is consistent with a more flexible design.
125-125: Passing tile/army/structure data to ArmyManager
TheonUpdatecall now includesarmyHexes,structureHexes, andexploredTiles, making the manager's logic more self-contained. Verify that all three maps are always in sync with the latest data to prevent desynchronization.
134-134: Double subscription to Army events
You already subscribe to Army updates at line 124 viathis.armySubscription. Subscribing again at line 134 might trigger duplicate or conflicting updates. Confirm if both calls are intentional or consolidate them. Likewise for structures.Also applies to: 136-136
379-389: Check for edge cases with stamina-based pathfinding
Retrievingstamina(line 380) and then callingarmyMovementManager.findPathsis good, but confirm how zero or undefined stamina is handled. Also verify thatselectedEntityIdis never null here, to avoid exceptions.
474-476: Include biome data in tile updates
The new parameterbiomeallows for a more precise representation of tile state. Just ensure your server logic will always provide a valid biome, or handle fallbacks if not.
492-492: Initializing nested Map before setting biome
Usingthis.exploredTiles.set(col, new Map());ensures no “undefined” errors when setting the biome. This defensive approach is clear and safe.Also applies to: 495-495
client/apps/game/src/three/managers/biome-colors.ts (1)
1-21: Centralized biome color mapping
Providing a single source of truth for biome colors promotes consistency across renders. For large-scale usage, consider referencing a palette from config or environment files if the colors need to be user-configurable.client/apps/game/src/three/types/systems.ts (2)
2-2: Added biome import
ImportingBiomeTypehelps unify type usage across the codebase.
28-28: Newbiomefield inTileSystemUpdate
Includingbiomein the tile update event aligns with the new data flow for rendering or logic based on biome type. Good addition.client/apps/game/src/ui/components/worldmap/armies/action-info.tsx (2)
32-32: LGTM! Enhanced type safety.The type change from
anyto a specific type improves type safety and makes the code more maintainable.
70-70: LGTM! Improved path handling.The change from passing
travelLengthto passing the actual path array allows for more accurate stamina cost calculations based on the full path information.client/apps/game/src/three/managers/minimap.ts (1)
64-64: LGTM! Enhanced data structure.The type change from
Set<number>toMap<number, BiomeType>improves the data structure by including biome information.client/apps/game/src/three/scenes/hexagon-scene.ts (1)
14-14: LGTM! Import statement updated correctly.The import of
BiomeTypefrom@bibliothecadao/eternumaligns with the centralized biome management approach.client/apps/game/src/three/scenes/hexception.tsx (1)
5-5: LGTM! Import statement updated correctly.The import of
BIOME_COLORSfrom the new location aligns with the centralized biome management approach.
| getHighlightedHexes(): Array<{ col: number; row: number }> { | ||
| return Array.from(this.paths.values()).map(({ path }) => ({ | ||
| col: path[path.length - 1].col - FELT_CENTER, | ||
| row: path[path.length - 1].row - FELT_CENTER, | ||
| })); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking for path array.
The getHighlightedHexes method assumes that each path array has at least one element. Add a check to handle empty paths.
getHighlightedHexes(): Array<{ col: number; row: number }> {
- return Array.from(this.paths.values()).map(({ path }) => ({
- col: path[path.length - 1].col - FELT_CENTER,
- row: path[path.length - 1].row - FELT_CENTER,
- }));
+ return Array.from(this.paths.values())
+ .filter(({ path }) => path.length > 0)
+ .map(({ path }) => ({
+ col: path[path.length - 1].col - FELT_CENTER,
+ row: path[path.length - 1].row - FELT_CENTER,
+ }));📝 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.
| getHighlightedHexes(): Array<{ col: number; row: number }> { | |
| return Array.from(this.paths.values()).map(({ path }) => ({ | |
| col: path[path.length - 1].col - FELT_CENTER, | |
| row: path[path.length - 1].row - FELT_CENTER, | |
| })); | |
| } | |
| getHighlightedHexes(): Array<{ col: number; row: number }> { | |
| return Array.from(this.paths.values()) | |
| .filter(({ path }) => path.length > 0) | |
| .map(({ path }) => ({ | |
| col: path[path.length - 1].col - FELT_CENTER, | |
| row: path[path.length - 1].row - FELT_CENTER, | |
| })); | |
| } |
| // todo: need to get this from config | ||
| getMinTravelStaminaCost() { | ||
| return 10; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider retrieving the minimum stamina cost from configuration.
The hardcoded value of 10 should be moved to configuration for better maintainability and flexibility.
Consider implementing one of these approaches:
- Add to the existing config interface:
export interface Config {
stamina: {
travelCost: number;
exploreCost: number;
refillPerTick: number;
startBoostTickCount: number;
+ minTravelCost: number;
};
// ...
}- Add a new component in the contract:
export interface MinTravelStaminaCostConfig {
cost: number;
}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/core/src/managers/army-movement-manager.ts (1)
392-392:⚠️ Potential issueVerify stamina cost calculation.
The stamina cost calculation in
_optimisticTravelHexuses a fixed cost per step, which contradicts the new biome-specific costs.The stamina cost should be calculated using the sum of
staminaDrainvalues for each hex in the path:- this._optimisticStaminaUpdate(overrideId, configManager.getTravelStaminaCost() * pathLength, currentArmiesTick); + const totalStaminaCost = path.reduce((cost, hex, i) => { + if (i === 0) return cost; + const biome = exploredHexes.get(hex.col - FELT_CENTER)?.get(hex.row - FELT_CENTER); + return cost + ArmyMovementManager.staminaDrain(biome!, troopType); + }, 0); + this._optimisticStaminaUpdate(overrideId, totalStaminaCost, currentArmiesTick);
🧹 Nitpick comments (3)
client/apps/game/src/three/scenes/worldmap.tsx (1)
425-454: Remove debug logging and consider adding error handling.The implementation is good, but has a few areas for improvement:
- Remove the debug console.log statement
- Consider adding error handling for edge cases
Apply this diff to improve the implementation:
public updateArmyHexes(update: ArmySystemUpdate) { const { hexCoords: { col, row }, } = update; const normalized = new Position({ x: col, y: row }).getNormalized(); const newCol = normalized.x; const newRow = normalized.y; const oldHexCoords = this.armiesPositions.get(update.entityId); const oldCol = oldHexCoords?.col; const oldRow = oldHexCoords?.row; - console.log({ oldCol, oldRow, newCol, newRow }); // update the position of the army this.armiesPositions.set(update.entityId, { col: newCol, row: newRow }); + try { if (oldCol !== undefined && oldRow !== undefined) { if (oldCol !== newCol || oldRow !== newRow) { this.armyHexes.get(oldCol)?.delete(oldRow); if (!this.armyHexes.has(newCol)) { this.armyHexes.set(newCol, new Set()); } this.armyHexes.get(newCol)?.add(newRow); } } else { if (!this.armyHexes.has(newCol)) { this.armyHexes.set(newCol, new Set()); } this.armyHexes.get(newCol)?.add(newRow); } + } catch (error) { + console.error('Failed to update army hexes:', error); + // Consider adding error recovery logic here + } }packages/core/src/managers/army-movement-manager.ts (2)
158-206: Optimize initial neighbor processing.The initial neighbor processing could be simplified and made more efficient:
- Consider extracting the hex validation logic into a helper method
- The stamina check could be moved earlier to avoid unnecessary calculations
+ private isValidHex(col: number, row: number, hasArmy: boolean, hasStructure: boolean, isExplored: boolean, canExplore: boolean): boolean { + return !hasArmy && !hasStructure && (isExplored || canExplore); + } public findPaths(...) { // ... existing code ... for (const { col, row } of neighbors) { + if (!this.isValidHex(col, row, hasArmy, hasStructure, isExplored, canExplore)) continue; - if (hasArmy || hasStructure) continue; - if (!isExplored && !canExplore) continue;
208-249: Optimize path exploration logic.The current implementation might not be optimal for large maps:
- Sorting the entire queue on each iteration is inefficient
- Consider using a priority queue data structure
Consider using a binary heap implementation for the priority queue to improve performance from O(n log n) to O(log n) per operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/apps/game/src/three/managers/army-manager.ts(8 hunks)client/apps/game/src/three/scenes/worldmap.tsx(9 hunks)client/apps/game/src/three/systems/system-manager.ts(2 hunks)client/apps/game/src/ui/elements/stamina-resource-cost.tsx(1 hunks)packages/core/src/managers/army-movement-manager.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/apps/game/src/three/systems/system-manager.ts
- client/apps/game/src/three/managers/army-manager.ts
🔇 Additional comments (9)
client/apps/game/src/ui/elements/stamina-resource-cost.tsx (5)
2-2: LGTM!The import of
HexTileInfotype is correctly added to support the new path prop type.
10-14: LGTM!The prop type changes correctly replace the simple
travelLengthnumber with a more detailedpatharray ofHexTileInfoobjects, enabling richer path information.
21-37: LGTM!The path info calculation is well-structured with:
- Correct memoization dependencies
- Proper total cost calculation by summing individual tile costs
- Appropriate balance color determination based on available stamina
45-47: LGTM!The stamina cost display correctly shows either the total path cost or exploration cost based on the exploration status, with appropriate color-coding for the balance.
48-58: LGTM!The biome info display is well-implemented with:
- Clear separation between explored and unexplored states
- Detailed per-tile biome and stamina cost information
- Proper use of tile coordinates as unique keys
client/apps/game/src/three/scenes/worldmap.tsx (4)
56-62: LGTM! Well-structured data management.The new properties provide better type safety and efficient tracking of game entities:
exploredTilesnow includes biome type informationarmyHexesandstructureHexestrack normalized positionsarmiesPositionsmaintains army position history for movement updates
456-470: LGTM! Clean structure hex tracking implementation.The method correctly normalizes positions and maintains the structure hex map.
472-495: LGTM! Well-structured explored hex management.The changes maintain consistency with other position handling and properly integrate biome information.
735-735: LGTM! Improved biome determination.The static method call to
Biome.getBiome()is a cleaner approach than using an instance method.
| private _getTroopType(): TroopType { | ||
| const entityArmy = getComponentValue(this.components.Army, this.entity); | ||
| const knightCount = entityArmy?.troops?.knight_count ?? 0; | ||
| const crossbowmanCount = entityArmy?.troops?.crossbowman_count ?? 0; | ||
| const paladinCount = entityArmy?.troops?.paladin_count ?? 0; | ||
|
|
||
| if (knightCount >= crossbowmanCount && knightCount >= paladinCount) { | ||
| return TroopType.Knight; | ||
| } | ||
| if (crossbowmanCount >= knightCount && crossbowmanCount >= paladinCount) { | ||
| return TroopType.Crossbowman; | ||
| } | ||
| return TroopType.Paladin; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider edge case handling in troop type determination.
The logic assumes at least one troop type exists. Consider handling the case where all counts are 0.
🏁 Script executed:
#!/bin/bash
# Search for army creation/initialization to verify default troop counts
ast-grep --pattern 'troops = { knight_count: $_, crossbowman_count: $_, paladin_count: $_ }'Length of output: 94
Additional Verification Required: Check Default Troop Initialization
The previous command did not produce any output, so we're not yet sure whether troop counts are safely defaulted or if an all‑zero case might occur. To clarify this, please run the following script to search for default initialization of the troops values across the repository:
This will help confirm whether additional edge case handling is necessary in the _getTroopType method.
🏁 Script executed:
#!/bin/bash
# Search for default initialization of troop counts in the codebase
rg --ignore-case 'troops\s*[=:]\s*{' .Length of output: 138
Additional Verification Required: Retrying Default Troop Initialization Search
The previous regex encountered an error. Please run the following corrected script to search for default initialization of troop counts across the repository:
This will help determine if there's any default initialization that might trigger an all‑zero troop count, informing whether additional edge-case handling is necessary in the _getTroopType method.
🏁 Script executed:
#!/bin/bash
# Search for default initialization of troop counts using PCRE flag for a literal '{'
rg --ignore-case -P 'troops\s*[:=]\s*\{' .Length of output: 6090
Action: Add Explicit Handling for All-Zero Troop Counts
In the _getTroopType method (lines 44–57), the current logic compares troop counts and defaults to TroopType.Knight when all counts are 0. This behavior may not be appropriate if an army with no troops should be treated differently. Please review the intended design and consider one of the following:
- If an all-zero state is expected, add a guard clause that either returns a distinct value (such as a
TroopType.Noneor similar) or logs a warning/error. - If the initialization is supposed to guarantee at least one nonzero troop count, update the initialization routines/documentation to reflect that assumption.
| // todo : refactor to use stamina config | ||
| public static staminaDrain(biome: BiomeType, troopType: TroopType) { | ||
| const baseStaminaCost = 20; // Base cost to move to adjacent hex | ||
|
|
||
| // Biome-specific modifiers per troop type | ||
| switch (biome) { | ||
| case BiomeType.Ocean: | ||
| return baseStaminaCost - 10; // -10 for all troops | ||
| case BiomeType.DeepOcean: | ||
| return baseStaminaCost - 10; // -10 for all troops | ||
| case BiomeType.Beach: | ||
| return baseStaminaCost; // No modifier | ||
| case BiomeType.Grassland: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | ||
| case BiomeType.Shrubland: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | ||
| case BiomeType.SubtropicalDesert: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | ||
| case BiomeType.TemperateDesert: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | ||
| case BiomeType.TropicalRainForest: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | ||
| case BiomeType.TropicalSeasonalForest: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | ||
| case BiomeType.TemperateRainForest: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | ||
| case BiomeType.TemperateDeciduousForest: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | ||
| case BiomeType.Tundra: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | ||
| case BiomeType.Taiga: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | ||
| case BiomeType.Snow: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? 0 : 10); | ||
| case BiomeType.Bare: | ||
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | ||
| case BiomeType.Scorched: | ||
| return baseStaminaCost + 10; // +10 for all troops | ||
| default: | ||
| return baseStaminaCost; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor stamina drain calculation for maintainability.
The current implementation has several issues:
- Magic numbers (10, 20) should be constants
- Repetitive logic for Paladin modifiers
- Missing documentation for biome effects
Consider this refactor:
+ const STAMINA_BASE_COST = 20;
+ const STAMINA_MODIFIER = 10;
+
+ type BiomeEffect = {
+ baseModifier: number;
+ paladinModifier?: number;
+ };
+
+ const BIOME_EFFECTS: Record<BiomeType, BiomeEffect> = {
+ [BiomeType.Ocean]: { baseModifier: -10 },
+ [BiomeType.Grassland]: { baseModifier: 0, paladinModifier: -10 },
+ // ... other biomes
+ };
+
public static staminaDrain(biome: BiomeType, troopType: TroopType) {
- const baseStaminaCost = 20;
- switch (biome) {
- case BiomeType.Ocean:
- return baseStaminaCost - 10;
- // ... other cases
- }
+ const effect = BIOME_EFFECTS[biome];
+ const paladinBonus = (troopType === TroopType.Paladin && effect.paladinModifier) || 0;
+ return STAMINA_BASE_COST + effect.baseModifier + paladinBonus;
}📝 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.
| // todo : refactor to use stamina config | |
| public static staminaDrain(biome: BiomeType, troopType: TroopType) { | |
| const baseStaminaCost = 20; // Base cost to move to adjacent hex | |
| // Biome-specific modifiers per troop type | |
| switch (biome) { | |
| case BiomeType.Ocean: | |
| return baseStaminaCost - 10; // -10 for all troops | |
| case BiomeType.DeepOcean: | |
| return baseStaminaCost - 10; // -10 for all troops | |
| case BiomeType.Beach: | |
| return baseStaminaCost; // No modifier | |
| case BiomeType.Grassland: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | |
| case BiomeType.Shrubland: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | |
| case BiomeType.SubtropicalDesert: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | |
| case BiomeType.TemperateDesert: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | |
| case BiomeType.TropicalRainForest: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | |
| case BiomeType.TropicalSeasonalForest: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | |
| case BiomeType.TemperateRainForest: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | |
| case BiomeType.TemperateDeciduousForest: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | |
| case BiomeType.Tundra: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | |
| case BiomeType.Taiga: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? 10 : 0); | |
| case BiomeType.Snow: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? 0 : 10); | |
| case BiomeType.Bare: | |
| return baseStaminaCost + (troopType === TroopType.Paladin ? -10 : 0); | |
| case BiomeType.Scorched: | |
| return baseStaminaCost + 10; // +10 for all troops | |
| default: | |
| return baseStaminaCost; | |
| } | |
| } | |
| // todo : refactor to use stamina config | |
| const STAMINA_BASE_COST = 20; | |
| const STAMINA_MODIFIER = 10; | |
| type BiomeEffect = { | |
| baseModifier: number; | |
| paladinModifier?: number; | |
| }; | |
| const BIOME_EFFECTS: Record<BiomeType, BiomeEffect> = { | |
| [BiomeType.Ocean]: { baseModifier: -10 }, | |
| [BiomeType.DeepOcean]: { baseModifier: -10 }, | |
| [BiomeType.Beach]: { baseModifier: 0 }, | |
| [BiomeType.Grassland]: { baseModifier: 0, paladinModifier: -10 }, | |
| [BiomeType.Shrubland]: { baseModifier: 0, paladinModifier: -10 }, | |
| [BiomeType.SubtropicalDesert]: { baseModifier: 0, paladinModifier: -10 }, | |
| [BiomeType.TemperateDesert]: { baseModifier: 0, paladinModifier: -10 }, | |
| [BiomeType.TropicalRainForest]: { baseModifier: 0, paladinModifier: 10 }, | |
| [BiomeType.TropicalSeasonalForest]: { baseModifier: 0, paladinModifier: 10 }, | |
| [BiomeType.TemperateRainForest]: { baseModifier: 0, paladinModifier: 10 }, | |
| [BiomeType.TemperateDeciduousForest]: { baseModifier: 0, paladinModifier: 10 }, | |
| [BiomeType.Tundra]: { baseModifier: 0, paladinModifier: -10 }, | |
| [BiomeType.Taiga]: { baseModifier: 0, paladinModifier: 10 }, | |
| [BiomeType.Snow]: { baseModifier: 0, paladinModifier: 10 }, | |
| [BiomeType.Bare]: { baseModifier: 0, paladinModifier: -10 }, | |
| [BiomeType.Scorched]: { baseModifier: 10 } | |
| }; | |
| public static staminaDrain(biome: BiomeType, troopType: TroopType) { | |
| const effect = BIOME_EFFECTS[biome]; | |
| const paladinBonus = (troopType === TroopType.Paladin && effect.paladinModifier) || 0; | |
| return STAMINA_BASE_COST + effect.baseModifier + paladinBonus; | |
| } |
Summary by CodeRabbit
New Features
Refactor