Refactor Game Engine Efficiency - #149
Conversation
- Consolidated transient state updates (puddles, visual effects, held enemies) into a single pass in `MainViewModel.kt` to reduce `GameState` copies. - Optimized Line-of-Sight (LOS) firing logic by pre-filtering obstructions once per tick. - Centralized isometric geometry and LOS math into `GridUtils.kt`. - Updated `fixes.md` with refactoring details (REF-006). 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. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates transient/held-enemy state updates into updateTransientState, moves LOS geometry into GridUtils (ISOMETRIC_Y_FACTOR, lineIntersectsCircle, isLineOfSightBlocked), and updates handleStallFiring to precompute obstructions and use GridUtils for LOS checks. ChangesLine-of-Sight Geometry and Engine Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 (1)
app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
446-450: ⚡ Quick winAvoid repeated linear enemy lookup in the transient loop.
Line 449 does
indexOfFirstper held-stall check. In this per-tick path, a one-timeid -> indexmap removes repeated scans and keeps the refactor’s performance gains more consistent.Suggested refactor
var updatedHexes = state.hexes.toMutableMap() var updatedEnemies = state.enemies.toMutableList() + val enemyIndexById = updatedEnemies.withIndex().associate { it.value.id to it.index } var changed = false state.hexes.forEach { (coord, tile) -> val stall = tile.stall if (stall?.heldEnemyId != null) { - val enemyIndex = updatedEnemies.indexOfFirst { it.id == stall.heldEnemyId } + val enemyIndex = enemyIndexById[stall.heldEnemyId] ?: -1 if (enemyIndex != -1) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c2e31b5-147b-4eee-950e-254274144142
📒 Files selected for processing (3)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/utils/GridUtils.ktfixes.md
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Avoid repeated linear enemy lookup in the transient loop. Line 449 does indexOfFirst per held-stall check. In this per-tick path, a one-time id -> index map removes repeated scans and keeps the refactor’s performance gains more consistent. |
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
442-444: ⚡ Quick winDefer the mutable copies until the first actual mutation.
state.hexes.toMutableMap()andstate.enemies.toMutableList()happen on every tick, even when no held enemy changes and no transient expires. In a 32 ms loop, that keeps allocation pressure high and blunts the performance gain from this refactor.♻️ One way to make this copy-on-write
- var updatedHexes = state.hexes.toMutableMap() - var updatedEnemies = state.enemies.toMutableList() + var updatedHexes: MutableMap<AxialCoordinate, HexTile>? = null + var updatedEnemies: MutableList<Enemy>? = null var changed = false ... - val enemyIndex = updatedEnemies.indexOfFirst { it.id == stall.heldEnemyId } + val enemies = updatedEnemies ?: state.enemies.toMutableList().also { updatedEnemies = it } + val enemyIndex = enemies.indexOfFirst { it.id == stall.heldEnemyId } ... - updatedEnemies[enemyIndex] = releasedEnemy - updatedHexes[coord] = tile.copy(stall = stall.copy(heldEnemyId = null)) + enemies[enemyIndex] = releasedEnemy + val hexes = updatedHexes ?: state.hexes.toMutableMap().also { updatedHexes = it } + hexes[coord] = tile.copy(stall = stall.copy(heldEnemyId = null)) ... - hexes = updatedHexes, - enemies = updatedEnemies + hexes = updatedHexes ?: state.hexes, + enemies = updatedEnemies ?: state.enemiesAlso applies to: 495-500
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b9aff82-29d6-4297-8549-15dc192ccae2
📒 Files selected for processing (1)
app/src/main/java/com/messark/hawker/MainViewModel.kt
Good catch! I've optimized the transient update loop by pre-calculating an ID-to-index map for enemies. This ensures that lookups are O(1) inside the hex-map iteration, keeping the performance gains consistent even with many held enemies. |
- Replaced linear `indexOfFirst` with an ID-to-index map in `updateTransientState`. - Addressed PR feedback regarding performance consistency. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
|
Defer the mutable copies until the first actual mutation. state.hexes.toMutableMap() and state.enemies.toMutableList() happen on every tick, even when no held enemy changes and no transient expires. In a 32 ms loop, that keeps allocation pressure high and blunts the performance gain from this refactor. |
Excellent point. I've refactored |
…pressure - Refactored `updateTransientState` to only perform `toMutableList` and `toMutableMap` when changes are detected. - Added `any` checks for puddle and effect expiration to avoid redundant filtering. - Optimized `enemyIndexMap` creation to only occur when held enemies are present. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
1 participant
This refactoring improves the performance of the core game loop by reducing redundant state copies and optimizing spatial queries. Specifically:
updateTransientStatenow handles multiple transient types in a single iteration over the hex map.handleStallFiringuses a pre-calculated list of obstruction coordinates, avoiding repeated filtering of the entire map during LOS checks.GridUtils.ktfor better maintainability and reusability.Verified with existing unit tests (Pathfinding, StallStats, UncleDoubleGrab).
PR created automatically by Jules for task 12457731955428841459 started by @candour
Summary by CodeRabbit
Bug Fixes
Performance
Documentation