Refactor Grid Logic and Optimize Pathfinding/Movement#125
Conversation
- Created GridUtils.kt to consolidate axial coordinate math and neighbor offsets. - Rewrote Pathfinding.kt with an idiomatic and efficient A* implementation. - Optimized handleEnemyMovement in MainViewModel to reduce redundant puddle iterations and distance calculations. - Updated MapGenerator and other call sites to use the new GridUtils. - Added record of changes to fixes.md. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCentralizes axial/grid math in GridUtils, rewrites A* Pathfinding to use the utilities, and updates MainViewModel to use GridUtils while adding Bak Kut Teh stat-boost aggregation, puddle slowdowns, and related enemy/stall/projectile logic changes. ChangesGrid Logic Centralization & Gameplay Update
Sequence Diagram(s)sequenceDiagram
participant VM as MainViewModel
participant Grid as GridUtils
participant Path as Pathfinding
participant GS as GameState
VM->>Grid: getNeighbors(coord)
Grid-->>VM: adjacent coords
VM->>Path: findPath(start, end)
Path->>Grid: getNeighbors / axialDistance
Grid-->>Path: neighbors / distances
Path-->>VM: reconstructed path
VM->>GS: apply movement updates (position, facing, status)
VM->>Grid: getNeighbors(coord) for boosts
Grid-->>VM: adjacent coords
VM->>GS: apply boosts/slowdowns to stalls/enemies/projectiles
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
![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 (3)
app/src/main/java/com/messark/hawker/utils/GridUtils.kt (1)
9-12: 💤 Low valueConsider exposing
NEIGHBOR_OFFSETSas an immutableList.
Arrayis mutable and reference-shared, so callers could in principle reassign elements (e.g.,GridUtils.NEIGHBOR_OFFSETS[0] = ...) and silently corrupt neighbor generation everywhere. AList(orprivatearray exposed viaList) gives you immutability for free.♻️ Suggested change
- val NEIGHBOR_OFFSETS = arrayOf( - AxialCoordinate(1, 0), AxialCoordinate(1, -1), AxialCoordinate(0, -1), - AxialCoordinate(-1, 0), AxialCoordinate(-1, 1), AxialCoordinate(0, 1) - ) + val NEIGHBOR_OFFSETS: List<AxialCoordinate> = listOf( + AxialCoordinate(1, 0), AxialCoordinate(1, -1), AxialCoordinate(0, -1), + AxialCoordinate(-1, 0), AxialCoordinate(-1, 1), AxialCoordinate(0, 1) + )app/src/main/java/com/messark/hawker/utils/Pathfinding.kt (1)
62-73: 💤 Low valueMinor:
reconstructPathwon't includestartifstart == endpath-relaxation never setsparents[start].Not a bug here because the
start == endcase is short-circuited on line 18, and any successful search to a differentendguarantees a chain ofparentsentries terminating atstart(whoseparents[start]is absent — so the loop correctly stops after addingstart). Just calling out that this invariant is what makes the helper correct; a small comment would help future readers.📝 Optional doc
+ /** + * Walks the parents map from `end` back to the start node (which is the only node + * without an entry in `parents`). Caller must guarantee `end` was reached. + */ private fun reconstructPath(app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
468-469: 💤 Low valuePrefer Kotlin's
maxOf/kotlin.math.maxoverjava.lang.Math.max.Idiomatic Kotlin and avoids the implicit
java.lang.Mathdependency. Applies to several other call sites in this file (e.g., 514, 804, 805) but only the changed lines are flagged here.♻️ Suggested change
- var freezeDuration = Math.max(0, enemy.freezeDurationMs - 32) - var speedBoostDuration = Math.max(0, enemy.speedBoostDurationMs - 32) + var freezeDuration = maxOf(0, enemy.freezeDurationMs - 32) + var speedBoostDuration = maxOf(0, enemy.speedBoostDurationMs - 32)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dba75ce-74bb-4227-8b08-d83a4b12af5a
📒 Files selected for processing (5)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/utils/GridUtils.ktapp/src/main/java/com/messark/hawker/utils/MapGenerator.ktapp/src/main/java/com/messark/hawker/utils/Pathfinding.ktfixes.md
💤 Files with no reviewable changes (1)
- app/src/main/java/com/messark/hawker/utils/MapGenerator.kt
|
@jules I think Neighbor_offsets should be an immutable List. Also Prefer Kotlin's maxOf / kotlin.math.max over java.lang.Math.max. |
|
The comparison for puddles being < 0.8 should use a float for consistency |
Thank you for the feedback, @jules! I've made the following changes:
|
- Changed NEIGHBOR_OFFSETS to a List in GridUtils.kt. - Replaced Math.max with Kotlin's maxOf in MainViewModel.kt. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
583-618:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn
finalEnemiesfromhandleEnemyMovement.Lines 584-602 apply Tiger Mom buff additions/removals to
finalEnemies, but Line 618 returnsupdatedEnemies. That drops those mutations every tick, so the armor buff state never actually sticks.Suggested fix
- return Pair(mutableState, updatedEnemies) + return Pair(mutableState, finalEnemies)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e4f0f60-9acf-4685-b661-341a75e6b6ed
📒 Files selected for processing (2)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/utils/GridUtils.kt
|
In MainViewModel.kt Return finalEnemies from handleEnemyMovement. Lines 584-602 apply Tiger Mom buff additions/removals to finalEnemies, but Line 618 returns updatedEnemies. That drops those mutations every tick, so the armor buff state never actually sticks. |
Sharp catch! I've updated |
Corrected a bug where Tiger Mom buff mutations were dropped by returning the intermediate updatedEnemies list instead of the final finalEnemies list in handleEnemyMovement. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
477-486:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a
Floatliteral for puddle proximity comparison.
GridUtils.axialDistance(PreciseAxialCoordinate, PreciseAxialCoordinate)returnsFloat, so< 0.8performs aFloat→Doublewidening on every enemy/puddle pair each tick. Use0.8fto keep the comparison purely inFloatand consistent with the rest of the precise-coord math.Proposed fix
- if (GridUtils.axialDistance(enemy.position, puddle.position) < 0.8) { + if (GridUtils.axialDistance(enemy.position, puddle.position) < 0.8f) {
🧹 Nitpick comments (1)
app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
524-528: 💤 Low valueOptional: extract the screen-x facing comparison.
The same
q + r / 2fscreen-x comparison is repeated at lines 431, 524–528, and 1096–1100 — three slightly different copies of the same axial→screen-x facing rule. A small helper (e.g.,GridUtils.screenX(q, r)orisFacingLeft(from, to, fallback)) would DRY these and keep the canonical transformation in one place alongsidetoScreenPrecise. Not blocking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c914104-b06f-4946-812b-a70524262c69
📒 Files selected for processing (5)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/utils/GridUtils.ktapp/src/main/java/com/messark/hawker/utils/MapGenerator.ktapp/src/main/java/com/messark/hawker/utils/Pathfinding.ktfixes.md
💤 Files with no reviewable changes (1)
- app/src/main/java/com/messark/hawker/utils/MapGenerator.kt
1 participant
I have picked the grid-related logic across several files for refactoring. The primary goal was to centralize redundant math and improve the efficiency of the core game loop.
Key changes include:
GridUtils.ktfile. This eliminates duplication inMainViewModel,MapGenerator, andPathfinding.Pathfinding.ktwas rewritten from scratch to be more idiomatic and efficient, utilizing the new centralized utility methods.MainViewModel.kt, thehandleEnemyMovementfunction was refactored to consolidate multiple iterations over puddles into a single pass per enemy, significantly reducing the number of distance calculations performed every tick.fixes.mdto track these improvements as requested.All unit tests (including Pathfinding and MainViewModel tests) pass successfully.
PR created automatically by Jules for task 774816735204400749 started by @candour
Summary by CodeRabbit
New Features
Improvements
Refactor