Refactor and Optimize Stall Targeting System - #156
Conversation
- Introduced pre-sorted enemy lists by TargetMode in MainViewModel to reduce targeting complexity. - Refactored StallBehavior interface to encapsulate targeting logic (selectTarget). - Optimized Line-of-Sight checks by evaluating candidates in priority order and short-circuiting. - Moved Tray Return Uncle's specific grabbing logic into its own behavior class. - Simplified handleStallFiring in MainViewModel by delegating to behaviors and FireResult. - Updated fixes.md with REF-008. 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 (2)
📝 WalkthroughWalkthroughStall firing architecture is refactored to delegate target selection and hold-release behavior to ChangesStall Firing Behavior Refactor
Possibly Related PRs
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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)
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: 3
🧹 Nitpick comments (2)
fixes.md (1)
38-38: ⚡ Quick winConsider documenting key architectural changes.
The entry focuses on performance optimization but omits significant architectural improvements introduced in this refactor:
FireResult.HoldEnemyvariant for hold-and-release outcomesStallBehavior.selectTarget(...)method delegationTrayReturnUncleBehaviorclass extractionWhile performance is the primary motivation, these structural changes improve encapsulation and maintainability. Documenting them provides a more complete picture of the refactor scope.
📝 Suggested expansion
-| REF-008 | 2025-06-12 | Refactored `handleStallFiring` and `StallBehavior` to optimize enemy targeting. Introduced pre-sorted enemy lists and encapsulated targeting logic in behaviors, reducing $O(S \times E)$ Line-of-Sight checks to $O(S \times \text{small constant})$. | Resolved | +| REF-008 | 2025-06-12 | Refactored `handleStallFiring` and `StallBehavior` to optimize enemy targeting. Introduced pre-sorted enemy lists, `StallBehavior.selectTarget(...)` delegation, `FireResult.HoldEnemy` variant, and `TrayReturnUncleBehavior` class, reducing Line-of-Sight checks via early termination on priority-ordered candidates. | Resolved |app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
788-792: ⚡ Quick winConsider defensive refactoring to future-proof
enemiesByModeagainst enum expansion.The
TargetModeenum currently has exactly four members:FIRST,CLOSEST,STRONGEST,WEAKEST. The hardcoded map seeds three of them (FIRST,STRONGEST,WEAKEST), withCLOSESTintentionally handled separately. No current bug exists. However, if a newTargetModevalue is added without updating the map,selectTarget()would silently return an empty list, causing stalls cycled to that mode to never fire.A defensive hardening: drive the map construction from
TargetMode.values()with an explicit switch anderror()for unhandled cases, ensuring any future enum member triggers a compile-time or runtime failure rather than silent failure.♻️ Suggested defensive construction
- val enemiesByMode = mapOf( - TargetMode.FIRST to state.enemies.sortedByDescending { it.currentPathIndex }, - TargetMode.STRONGEST to state.enemies.sortedByDescending { it.health }, - TargetMode.WEAKEST to state.enemies.sortedBy { it.health } - ) + val enemiesByMode: Map<TargetMode, List<Enemy>> = TargetMode.values() + .filter { it != TargetMode.CLOSEST } + .associateWith { mode -> + when (mode) { + TargetMode.FIRST -> state.enemies.sortedByDescending { it.currentPathIndex } + TargetMode.STRONGEST -> state.enemies.sortedByDescending { it.health } + TargetMode.WEAKEST -> state.enemies.sortedBy { it.health } + else -> error("Unhandled TargetMode for pre-sort: $mode") + } + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e366e83-fe87-4541-84b7-378d4587275c
📒 Files selected for processing (3)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/registry/Registry.ktfixes.md
|
The TargetMode enum currently has exactly four members: FIRST, CLOSEST, STRONGEST, WEAKEST. The hardcoded map seeds three of them (FIRST, STRONGEST, WEAKEST), with CLOSEST intentionally handled separately. No current bug exists. However, if a new TargetMode value is added without updating the map, selectTarget() would silently return an empty list, causing stalls cycled to that mode to never fire. |
|
When stall.targetMode != CLOSEST and the caller hasn't populated enemiesByMode[stall.targetMode], candidates collapses to emptyList() and the stall silently never fires (no log, no fallback). Today the caller in MainViewModel.handleStallFiring hardcodes only FIRST, STRONGEST, and WEAKEST (lines 788-792), so any additional TargetMode value (e.g., LAST) would be silently dead even though cycleTargetMode will happily route users to it. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Good point. I've updated |
- Updated `DefaultStallBehavior.selectTarget` to use a `when` block for `TargetMode`. This ensures that any future additions to the `TargetMode` enum will require an explicit update to the targeting logic, avoiding silent failures. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
1 similar comment
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
1 participant
Refactored the stall firing logic in
MainViewModel.ktandRegistry.ktfor better efficiency and encapsulation.Key improvements:
TargetMode(First, Strongest, Weakest) once per game tick, stalls now evaluate only the highest-priority candidates. LoS checks—which are computationally expensive—are now performed only until a valid target is found, significantly reducing the average workload per tick.StallBehaviorinterface. This allows for stall-specific targeting rules (likeTRAY_RETURN_UNCLE's unique proximity requirements) to be self-contained rather than cluttered in the ViewModel.handleStallFiringfunction inMainViewModel.kthas been significantly simplified. It now delegates the target selection and the firing action to the registry, handling different outcomes (projectiles, puddles, or capturing enemies) through a cleanFireResultsealed class.fixes.md(REF-008).Verified with existing unit tests for stall stats, Tray Return Uncle logic, and general game state management.
PR created automatically by Jules for task 10273960744059109657 started by @candour
Summary by CodeRabbit