Refactor MapGenerator for Efficiency and Determinism - #131
Conversation
- Transitions `generateRandomVerticalMap` from a brute-force retry loop to a single-pass "path-first" carving strategy. - Guarantees a valid path from START to GOAL_TABLE by pre-calculating it on an empty grid. - Centralizes offset-to-axial coordinate conversion in a private `offsetToAxial` helper. - Adds `Random` injection to all generation methods for improved determinism and testability. - Updates `MainViewModel` to pass its internal `random` instance to the generator. - Documents the changes in `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. |
📝 WalkthroughWalkthroughMapGenerator now accepts an injected Random and computes a guaranteed path first, then generates tiles while avoiding pillars on that path; generateMap uses the injected Random for floor variants. MainViewModel threads its random into map generation and uses it for enemy selection and shuffling. ChangesRandom Injection and Deterministic Map Generation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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/MapGenerator.kt (2)
38-38: ⚡ Quick winSilent
nullfallback could mask a degenerate case.
Pathfinding.findPath(...)?.toSet() ?: emptySet()quietly falls back to "no guaranteed path", at which point thecoord !in guaranteedPathcheck no longer protects anything and pillars can block the map. On an empty grid with valid endpoints this should never happen, but it is a load-bearing invariant of the new algorithm — worth either asserting or logging a warning so a future regression inPathfindingdoes not silently re-introduce unsolvable maps.🛡️ Suggested defensive guard
- // Guaranteed path carving: use A* on an empty grid to find a baseline path - val guaranteedPath = Pathfinding.findPath(startPos, endPos, emptySet(), allCoords)?.toSet() ?: emptySet() + // Guaranteed path carving: use A* on an empty grid to find a baseline path. + // On an empty grid with both endpoints in `allCoords`, A* must succeed. + val guaranteedPath = Pathfinding.findPath(startPos, endPos, emptySet(), allCoords)?.toSet() + ?: error("Pathfinding failed on an empty grid; check Pathfinding/offsetToAxial invariants")
54-56: ⚡ Quick winFloor-variant assignment is inconsistent with
generateMap.Here, every non-
PILLARtile (includingSTARTandGOAL_TABLE) gets a weighted floor variant viagetWeightedFloorVariant(random). IngenerateMapat Line 87, onlyTileType.FLOORtiles receive a variant;START/GOAL_TABLEget0. Two callers producing the same tile type with differentfloorVariantvalues will likely confuse downstream rendering or save/load logic. Pick one rule and apply it in both.♻️ Align with `generateMap` (FLOOR-only variants)
- val floorVariant = if (type != TileType.PILLAR) { - getWeightedFloorVariant(random) - } else 0 + val floorVariant = if (type == TileType.FLOOR) { + getWeightedFloorVariant(random) + } else 0app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
243-260: ⚡ Quick winStatic
Random.Defaulthere defeats the determinism the PR is establishing.Adjacent to your changes:
generateEnemyListstill callskotlin.random.Random.nextInt(allowedTiers.size)(Line 245), bypassing the injectedrandomfield. Seeding the view-model produces a deterministic map but a non-deterministic enemy roster, which makes deterministic replay/testing only half work. Trivial to fix while the file is open.♻️ Use the injected `random` field
- val type = allowedTiers[kotlin.random.Random.nextInt(allowedTiers.size)] + val type = allowedTiers[random.nextInt(allowedTiers.size)] @@ - return enemyList.shuffled() + return enemyList.shuffled(random)(Also worth considering for the
.shuffled()calls at Lines 211 and 266 if you want full reproducibility of the wave roster.)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af64eaf9-2fc3-461d-a61d-fdc218b70d3b
📒 Files selected for processing (3)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/utils/MapGenerator.ktfixes.md
|
In MapGenerator.kt: The comment is indeed out of sync. 1 + random.nextInt(6) returns values 1–6, but those index into FLOOR_RECTS at positions 1–6, yielding sprites floor02 through floor07. The comment "floor01 to floor07" is misleading because floor01 (at index 0) is only reachable from the if branch. The suggested fix in the original review is also incorrect: the comment should say "floor02 to floor07", not "floor01 to floor06". The value 6 maps to FLOOR_RECTS[6], which is floor07, and floor07 exists in the asset set and remains reachable. Pathfinding.findPath(...)?.toSet() ?: emptySet() quietly falls back to "no guaranteed path", at which point the coord !in guaranteedPath check no longer protects anything and pillars can block the map. On an empty grid with valid endpoints this should never happen, but it is a load-bearing invariant of the new algorithm — worth either asserting or logging a warning so a future regression in Pathfinding does not silently re-introduce unsolvable maps. In MainViewModel.kt: Adjacent to your changes: generateEnemyList still calls kotlin.random.Random.nextInt(allowedTiers.size) (Line 245), bypassing the injected random field. Seeding the view-model produces a deterministic map but a non-deterministic enemy roster, which makes deterministic replay/testing only half work. Trivial to fix while the file is open. |
Good catch. I've updated the comment in |
- Fixed misleading comment in `MapGenerator.getWeightedFloorVariant` regarding floor variants. - Added log warning in `MapGenerator.generateRandomVerticalMap` for `guaranteedPath` invariant protection. - Removed `Random.Default` fallback in `MainViewModel` generator calls. - Updated `generateEnemyList` and `.shuffled()` logic in `MainViewModel` to use the injected `random` instance, ensuring full game state determinism. - Updated `fixes.md` with REF-002 entry. 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.
🧹 Nitpick comments (1)
app/src/main/java/com/messark/hawker/utils/MapGenerator.kt (1)
38-43: 💤 Low valueGuaranteed-path invariant correctly guarded.
On an empty grid the A* call should always succeed; the
Log.ecovers the one real regression scenario (a bug inPathfinding.findPath). If the log is ever triggered in practice it signals a pathfinding regression, not a map-gen issue, and the map will silently be potentially unsolvable.One low-cost hardening option: retain the empty-set fallback but also re-randomise the endpoints and retry once before giving up.
🛡️ Optional retry sketch
- if (guaranteedPath.isEmpty()) { - android.util.Log.e("MapGenerator", "Failed to carve a guaranteed path from $startPos to $endPos") - } + if (guaranteedPath.isEmpty()) { + android.util.Log.e("MapGenerator", "Failed to carve a guaranteed path from $startPos to $endPos – retrying with fallback endpoints") + // Deterministic fallback: pin start/end to column 0 + val fallbackStart = offsetToAxial(0, startR) + val fallbackEnd = offsetToAxial(0, endR) + return generateRandomVerticalMap(width, height, random) + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d83c7a8d-a683-4a83-824c-4b1b3263ca1a
📒 Files selected for processing (2)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/utils/MapGenerator.kt
1 participant
I have refactored the map generation logic in
MapGenerator.ktto be significantly more efficient and deterministic. The previous implementation relied on awhile(true)loop that randomly placed pillars and hoped for a valid path, which could lead to performance issues or non-deterministic behavior.The new implementation first selects the start and end positions and uses the existing A* pathfinding to carve a guaranteed valid path through an empty grid. It then fills the remaining tiles, only placing obstacles (pillars) on coordinates that are NOT part of the guaranteed path. This ensures a valid, playable map in$O(\text{Area})$ time.
Additionally, I've centralized the coordinate conversion logic to reduce duplication and added
Randominjection support throughout the utility to support better unit testing and game state determinism. I've also updatedMainViewModel.ktto pass its ownrandominstance to the generator.All unit tests pass, and the project compiles successfully.
PR created automatically by Jules for task 3761664495891248248 started by @candour
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation