feat: Implement alliance request and break functionality with new key…#3214
feat: Implement alliance request and break functionality with new key…#3214qxie11 wants to merge 2 commits intoopenfrontio:mainfrom
Conversation
…binds, events, and UI handling.
|
Yevgenii Ponomarov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughAdds F/G keybinds that emit AllianceRequestEvent and BreakAllianceEvent with pointer coordinates; MainRadialMenu handles these events to invoke player alliance actions; UnitLayer now accepts UIState and renders alliance/traitor cursor icons while tracking mouse and key state. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Input as InputHandler
participant Menu as MainRadialMenu
participant Layer as UnitLayer
participant Actions as PlayerActionHandler
participant World as World
User->>Input: Press/hold F or G + move mouse
Input->>Layer: MouseMoveEvent (position)
Layer->>Layer: Update mouse position, key state
User->>Input: Release pointer (pointerup)
Input->>Menu: Emit AllianceRequestEvent(x,y) / BreakAllianceEvent(x,y)
Input->>Layer: Emit same Event
Menu->>Menu: Screen -> World coords
Menu->>World: Query tile and owner
Menu->>Actions: handleAllianceRequest / handleBreakAlliance(targetPlayer)
Actions->>World: Update alliance/traitor state
Layer->>Layer: Transform mouse -> target tile
Layer->>Layer: Render alliance/traitor cursor icon at tile
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/InputHandler.ts (1)
228-243:⚠️ Potential issue | 🔴 CriticalKeyG is bound to both
groundAttackandbreakAlliance— they will conflict.
groundAttack(line 228) defaults to"KeyG", andbreakAlliance(line 243) also defaults to"KeyG". When the user holds G and clicks, the pointer-up handler emitsBreakAllianceEvent. Then, when G is released, the key-up handler at lines 392–395 also emitsDoGroundAttackEvent. Both actions fire for a single G interaction.Pick a different default key for
breakAlliance(e.g."KeyH"or"KeyV"), or add a guard so ground-attack doesn't fire when the break-alliance flow was used.#!/bin/bash # Verify keybind collision: check all default keybind values for duplicates rg -n 'keybinds\s*=' src/client/InputHandler.ts | head -80
🤖 Fix all issues with AI agents
In `@src/client/InputHandler.ts`:
- Around line 362-363: Replace the hardcoded key codes in the active-keys
tracking array with the user's configured keybinds: use this.keybinds.alliance
instead of "KeyF" and this.keybinds.breakAlliance instead of "KeyG" (locate the
array in InputHandler where these literals are listed, e.g., the active
keys/allowed keys setup inside the input handling method or constructor) so
remapped keys from saved settings are respected.
🧹 Nitpick comments (5)
src/client/graphics/layers/UnitLayer.ts (4)
139-147: Duplicate key listeners — UnitLayer tracks F/G independently of InputHandler.Both
InputHandlerandUnitLayeradd their ownwindowkeydown/keyuplisteners for the same keys. This works but creates two sources of truth. Also, the hardcoded"KeyF"/"KeyG"strings here won't respect user keybind remapping.Consider reading the key state from a shared source (e.g., expose an accessor on
InputHandleror pass key state throughUIState) instead of duplicating listeners.
87-97: No error handling for failed image loads.If the SVG asset path is wrong or the image fails to load,
img.onerroris never set, soallianceIcon/traitorIconsilently staynull. Consider logging a warning on error so broken assets are easier to diagnose.Proposed fix
private loadAndTintIcon( path: string, color: string, callback: (icon: HTMLCanvasElement) => void, ) { const img = new Image(); img.src = path; img.onload = () => { callback(this.tintIcon(img, color)); }; + img.onerror = () => { + console.warn(`Failed to load icon: ${path}`); + }; }
330-330: Remove commented-out debug log.- // console.log("DrawIcon", { f: this.fKeyHeld, g: this.gKeyHeld, x, y, drawSize, icon: !!iconToDraw });
66-76: Remove unuseduiStateparameter from constructor unless planned for immediate use.The
uiStateparameter is accepted and stored asprivate uiState: UIState, but it's not referenced anywhere in this file. If it's needed for future work, the addition can be deferred until the field is actually used.src/client/graphics/layers/MainRadialMenu.ts (1)
111-157: Consider adding alliance status checks before sending requests.The
AllianceRequestEventandBreakAllianceEventhandlers send requests without checking the current alliance state first. TheUnitLayer.drawCursorIconalready usesisAlliedWithto hint at alliance status — adding the same guard here would prevent sending pointless requests to the server.That said, if the server-side game logic validates this, this is optional.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/InputHandler.ts`:
- Around line 242-243: The default key mapping for breakAlliance conflicts with
groundAttack — change the default for the breakAlliance binding in the
InputHandler keymap (the object that currently sets alliance: "KeyF",
breakAlliance: "KeyG") to a non-conflicting key (e.g. "KeyH" or "KeyX"); update
the mapping where breakAlliance is defined and any related comments/tests, and
verify behavior in the onPointerUp handler that emits BreakAllianceEvent and the
keyup handler that emits DoGroundAttackEvent no longer collide.
|
I find the ally icon under the mouse a bit strange, it's not clear to the user what that means, or if there is a shortcut at all. Perhaps instead we could show the shortcuts underneath the player info panel (the panel at the top) so if we are on a pc, underneath the player info add another row that looks like: (F): And the hover over the player and pressing (f) will send an alliance request etc. and then as we add more shortcuts, users will know they are available. just like how the build menu shows the build number shortcut |
There was a problem hiding this comment.
i don't think this makes sense on the unit layer, but i think we can probably move this code altogether if we change the UX to show the keys in the player info panel
3 participants
Alliance & Betrayal Hotkeys Implementation (F/G)
Description
This PR introduces hotkeys for quick diplomatic actions, along with visual indicators. Players can now form or break alliances more efficiently by hovering over a target and pressing the corresponding keys.
Key Changes
1. New Hotkeys
2. Visual Feedback
When holding the keys and hovering over another player's territory, a context-aware icon appears at the cursor:
F): Displays if the target is not yet an ally.G): Displays if the target is currently an ally.3. Technical Details
UIStateto UnitLayer for access to UI state (where needed).Testing
screen-capture.mp4