adding "Reverse move" hotkey #7003
Conversation
📝 WalkthroughWalkthroughAdds reverse-move command mode: new callback to force-enable/disable reverse movement on units, new key bindings and command-mode starter, cursor support, and command-mode state/handlers that enable or disable reverse movement when commands are issued or the mode ends. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InputSystem as Input System
participant CmdMode as Command Mode
participant WorldView
participant Callbacks
participant Unit
User->>InputSystem: press reverse_move key
InputSystem->>CmdMode: StartReverseMoveCommandMode()
CmdMode->>CmdMode: StartCommandMode(isReverseMove=true)
CmdMode->>WorldView: set cursor = RULEUCC_ReverseMove
User->>CmdMode: click/map issue
CmdMode->>CmdMode: OnCommandIssued(command)
CmdMode->>CmdMode: EnableReverseMove(command)
CmdMode->>Callbacks: ForceReverseMove({Enable:true}, selection)
Callbacks->>Unit: unit:ForceReverseMove(true)
User->>CmdMode: issue other command or exit
CmdMode->>CmdMode: DisableReverseMove(command) / EndCommandMode()
CmdMode->>Callbacks: ForceReverseMove({Enable:false}, trackedUnits)
Callbacks->>Unit: unit:ForceReverseMove(false)
CmdMode->>WorldView: restore cursor
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
| if not data.Units then | ||
| selection = SecureUnits(selection) | ||
| else | ||
| selection = SecureUnits(data.Units) |
There was a problem hiding this comment.
Would it not be better to always use the current selection? That way you don't even need to use the SecureUnits function as far as I am aware.
There was a problem hiding this comment.
There are cases when we don't need all selected units but only part of them (exlcude units without MaxSpeedReverse or with MaxSpeedReverse = 0). I do this check on UI and sim side just in case. Theoretically we can do this check only in sim and just use current selection, idk what is better
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@lua/keymap/keydescriptions.lua`:
- Around line 562-563: Add the missing locale keys referenced by
keydescriptions.lua: define key_desc_reverse_move and
key_desc_shift_reverse_move in every localization strings_db.lua under ./loc/*/
(e.g., US, FR, etc.), ensuring each file maps those keys to appropriate
translated strings (English can mirror "Reverse move."), so the <LOC> tokens no
longer appear; update the entries consistently across all locale files where
other key descriptions live to match existing formatting and placement.
In `@lua/SimCallbacks.lua`:
- Around line 831-856: In Callbacks.ForceReverseMove, the status message
currently prints table.getn(selection) even for units that lack a reverse speed;
update the function to track a counter (e.g., affectedCount) and increment it
only when you actually call unit:ForceReverseMove (i.e., when
unit.Blueprint.Physics.MaxSpeedReverse and > 0); then use that affectedCount in
the data.ShowMsg print statements (respecting data.Enable) so the log reflects
only the units that were modified rather than the full selection.
In `@lua/skins/skins.lua`:
- Line 204: Add the missing cursor asset file at
/textures/ui/common/game/cursors/reverse_move-.dds (matching the naming used in
RULEUCC_ReverseMove) and commit it so the skin entry RULEUCC_ReverseMove has a
real texture to load; then update the CursorType alias in lua/skins/skins.lua to
include "RULEUCC_ReverseMove" alongside the other cursor names to keep the type
definition consistent with the RULEUCC_* entries.
In `@lua/ui/controls/worldview.lua`:
- Around line 537-549: OnCursorReverseMove currently updates the cursor but
doesn't toggle the ignore-mode like OnCursorMove does, causing inconsistent
click handling; update OnCursorReverseMove to mirror OnCursorMove's enter/exit
behavior by calling the same EnableIgnoreMode toggle (or method/property used in
OnCursorMove) when enabled changes, so when the reverse-move cursor is applied
you set EnableIgnoreMode on entry and clear it on exit, then continue to update
the cursor and call self:ApplyCursor(); reference OnCursorReverseMove,
OnCursorMove, and the EnableIgnoreMode toggle used in the existing code.
🧹 Nitpick comments (4)
lua/ui/game/commandmode.lua (4)
187-190: Type definition missingisReverseMovefield.The
data.isReverseMovefield is accessed here but not defined in theCommandModeDataBaseor related type definitions (lines 102-123). Consider adding the field to the appropriate type for better documentation and IDE support.📝 Suggested type update
Add to
CommandModeDataBaseor create a new type:---@class CommandModeDataBase ---@field cursor? CommandCap # Similar to the field 'name' ---@field altCursor? string # Allows for an alternative cursor ---@field isReverseMove? boolean # Indicates reverse-move command mode
656-684: Naming inconsistency:EnableReverseMoveForvsdisableReverseMoveFor.
EnableReverseMoveForuses PascalCase whiledisableReverseMoveFor(line 697) uses camelCase. Consider using consistent naming for local variables throughout.♻️ Suggested fix
local function EnableReverseMove(command) - local EnableReverseMoveFor = {} + local enableReverseMoveFor = {} for _,unit in command.Units do -- ... - TableInsert(EnableReverseMoveFor, entityID) + TableInsert(enableReverseMoveFor, entityID) end end - local cb = { Func = 'ForceReverseMove', Args = { Enable = true, ShowMsg = false, Units = EnableReverseMoveFor } } + local cb = { Func = 'ForceReverseMove', Args = { Enable = true, ShowMsg = false, Units = enableReverseMoveFor } } SimCallback(cb, false) end
669-675: Queue deletion iterates from the beginning, butqueueLengthindex may shift.When deleting commands by ID, if
DeleteCommandremoves the command immediately from the underlying queue, subsequent iterations might reference stale indices. However, since you're using command IDs (not indices) for deletion and the Lua tablecommandQueueis a snapshot, this should be safe.Consider adding a brief comment clarifying that
commandQueueis a snapshot and deletions by ID don't affect iteration.
682-684: SimCallback issued even when no units are eligible for reverse move.If
EnableReverseMoveForis empty (no units hadMaxSpeedReverse > 0), the SimCallback is still issued with an empty units list. Consider adding an early return to avoid unnecessary sim callbacks.♻️ Suggested optimization
end end + if TableEmpty(enableReverseMoveFor) then + return + end + local cb = { Func = 'ForceReverseMove', Args = { Enable = true, ShowMsg = false, Units = EnableReverseMoveFor } } SimCallback(cb, false) end
lua/keymap/keydescriptions.lua
Outdated
| ['reverse_move'] = '<LOC key_desc_reverse_move>Reverse move.', | ||
| ['shift_reverse_move'] = '<LOC key_desc_shift_reverse_move>Reverse move.', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "key_desc_(shift_)?reverse_move" -S .Repository: FAForever/fa
Length of output: 275
🏁 Script executed:
rg -n "key_desc_" --type lua | head -20Repository: FAForever/fa
Length of output: 1894
🏁 Script executed:
find . -type d -name "*locale*" 2>/dev/null | head -10Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
find . -type f -name "*loc*" 2>/dev/null | grep -E "\.(lua|txt)" | head -20Repository: FAForever/fa
Length of output: 84
🏁 Script executed:
rg -n "key_desc_0000" -S .Repository: FAForever/fa
Length of output: 1118
🏁 Script executed:
rg -n "key_desc_reverse_move" -S ./loc/Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
rg -n "key_desc_shift_reverse_move" -S ./loc/Repository: FAForever/fa
Length of output: 38
Add missing locale entries for reverse_move and shift_reverse_move hotkeys.
The locale keys key_desc_reverse_move and key_desc_shift_reverse_move referenced in keydescriptions.lua (lines 562–563) are not defined in any of the language files under ./loc/. Add these entries to all localization files (e.g., ./loc/US/strings_db.lua, ./loc/FR/strings_db.lua, etc.) to prevent raw <LOC> tokens from displaying in the UI.
🤖 Prompt for AI Agents
In `@lua/keymap/keydescriptions.lua` around lines 562 - 563, Add the missing
locale keys referenced by keydescriptions.lua: define key_desc_reverse_move and
key_desc_shift_reverse_move in every localization strings_db.lua under ./loc/*/
(e.g., US, FR, etc.), ensuring each file maps those keys to appropriate
translated strings (English can mirror "Reverse move."), so the <LOC> tokens no
longer appear; update the entries consistently across all locale files where
other key descriptions live to match existing formatting and placement.
| ---@param data { Enable: boolean, ShowMsg: boolean } | ||
| ---@param selection Unit[] | ||
| Callbacks.ForceReverseMove = function(data, selection) | ||
| -- verify selection | ||
| if not data.Units then | ||
| selection = SecureUnits(selection) | ||
| else | ||
| selection = SecureUnits(data.Units) | ||
| end | ||
|
|
||
| if (not selection) or TableEmpty(selection) then | ||
| return | ||
| end | ||
|
|
||
| for k, unit in selection do | ||
| if unit.Blueprint.Physics.MaxSpeedReverse and unit.Blueprint.Physics.MaxSpeedReverse > 0 then | ||
| unit:ForceReverseMove(data.Enable) | ||
| end | ||
| end | ||
|
|
||
| if data.ShowMsg then | ||
| if data.Enable == true then | ||
| print(string.format("Force reverse move ENABLED for %d units", table.getn(selection))) | ||
| else | ||
| print(string.format("Force reverse move DISABLED for %d units", table.getn(selection))) | ||
| end |
There was a problem hiding this comment.
Count only reverse-capable units in the status message.
The log currently reports the full selection size even when some units lack MaxSpeedReverse, which can be misleading. Track the affected count instead.
🐛 Proposed fix
- for k, unit in selection do
+ local affected = 0
+ for k, unit in selection do
if unit.Blueprint.Physics.MaxSpeedReverse and unit.Blueprint.Physics.MaxSpeedReverse > 0 then
unit:ForceReverseMove(data.Enable)
+ affected = affected + 1
end
end
if data.ShowMsg then
if data.Enable == true then
- print(string.format("Force reverse move ENABLED for %d units", table.getn(selection)))
+ print(string.format("Force reverse move ENABLED for %d units", affected))
else
- print(string.format("Force reverse move DISABLED for %d units", table.getn(selection)))
+ print(string.format("Force reverse move DISABLED for %d units", affected))
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ---@param data { Enable: boolean, ShowMsg: boolean } | |
| ---@param selection Unit[] | |
| Callbacks.ForceReverseMove = function(data, selection) | |
| -- verify selection | |
| if not data.Units then | |
| selection = SecureUnits(selection) | |
| else | |
| selection = SecureUnits(data.Units) | |
| end | |
| if (not selection) or TableEmpty(selection) then | |
| return | |
| end | |
| for k, unit in selection do | |
| if unit.Blueprint.Physics.MaxSpeedReverse and unit.Blueprint.Physics.MaxSpeedReverse > 0 then | |
| unit:ForceReverseMove(data.Enable) | |
| end | |
| end | |
| if data.ShowMsg then | |
| if data.Enable == true then | |
| print(string.format("Force reverse move ENABLED for %d units", table.getn(selection))) | |
| else | |
| print(string.format("Force reverse move DISABLED for %d units", table.getn(selection))) | |
| end | |
| ---@param data { Enable: boolean, ShowMsg: boolean } | |
| ---@param selection Unit[] | |
| Callbacks.ForceReverseMove = function(data, selection) | |
| -- verify selection | |
| if not data.Units then | |
| selection = SecureUnits(selection) | |
| else | |
| selection = SecureUnits(data.Units) | |
| end | |
| if (not selection) or TableEmpty(selection) then | |
| return | |
| end | |
| local affected = 0 | |
| for k, unit in selection do | |
| if unit.Blueprint.Physics.MaxSpeedReverse and unit.Blueprint.Physics.MaxSpeedReverse > 0 then | |
| unit:ForceReverseMove(data.Enable) | |
| affected = affected + 1 | |
| end | |
| end | |
| if data.ShowMsg then | |
| if data.Enable == true then | |
| print(string.format("Force reverse move ENABLED for %d units", affected)) | |
| else | |
| print(string.format("Force reverse move DISABLED for %d units", affected)) | |
| end |
🤖 Prompt for AI Agents
In `@lua/SimCallbacks.lua` around lines 831 - 856, In Callbacks.ForceReverseMove,
the status message currently prints table.getn(selection) even for units that
lack a reverse speed; update the function to track a counter (e.g.,
affectedCount) and increment it only when you actually call
unit:ForceReverseMove (i.e., when unit.Blueprint.Physics.MaxSpeedReverse and >
0); then use that affectedCount in the data.ShowMsg print statements (respecting
data.Enable) so the log reflects only the units that were modified rather than
the full selection.
| RULEUCC_Teleport = {'/textures/ui/common/game/cursors/transport.dds', 15, 15}, | ||
| RULEUCC_Script = {'/textures/ui/common/game/cursors/attack.dds', 15, 15}, | ||
| RULEUCC_Invalid = {'/textures/ui/common/game/cursors/attack-invalid.dds', 15, 15}, | ||
| RULEUCC_ReverseMove = {'/textures/ui/common/game/cursors/reverse_move-.dds', 15, 15, 12, 12}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify the reverse-move cursor DDS exists in the repo
fd -a 'reverse_move-.dds' .Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
# Search for CursorType definition to understand the typing context
rg -i "CursorType" --type luaRepository: FAForever/fa
Length of output: 138
🏁 Script executed:
# Get context around line 204 in the target file
sed -n '190,220p' lua/skins/skins.luaRepository: FAForever/fa
Length of output: 2821
🏁 Script executed:
# Find the CursorType alias definition in skins.lua
rg -A 50 '---@alias CursorType' lua/skins/skins.luaRepository: FAForever/fa
Length of output: 477
🏁 Script executed:
# Find the CursorType alias definition in skins.lua with proper escaping
rg -A 50 '@alias CursorType' lua/skins/skins.luaRepository: FAForever/fa
Length of output: 1306
Add the missing reverse_move-.dds asset and update the CursorType alias.
The cursor asset /textures/ui/common/game/cursors/reverse_move-.dds does not exist in the repository; without it, the cursor will fail to render. Additionally, add "RULEUCC_ReverseMove" to the CursorType alias definition in lua/skins/skins.lua for type consistency with other cursor entries.
🤖 Prompt for AI Agents
In `@lua/skins/skins.lua` at line 204, Add the missing cursor asset file at
/textures/ui/common/game/cursors/reverse_move-.dds (matching the naming used in
RULEUCC_ReverseMove) and commit it so the skin entry RULEUCC_ReverseMove has a
real texture to load; then update the CursorType alias in lua/skins/skins.lua to
include "RULEUCC_ReverseMove" alongside the other cursor names to keep the type
definition consistent with the RULEUCC_* entries.
| --- Called when the order `RULEUCC_ReverseMove` is being applied | ||
| ---@param self WorldView | ||
| ---@param identifier 'RULEUCC_ReverseMove' | ||
| ---@param enabled boolean | ||
| ---@param changed boolean | ||
| OnCursorReverseMove = function(self, identifier, enabled, changed) | ||
| if enabled then | ||
| if changed then | ||
| local cursor = self.Cursor | ||
| cursor[1], cursor[2], cursor[3], cursor[4], cursor[5] = UIUtil.GetCursor(identifier) | ||
| self:ApplyCursor() | ||
| end | ||
| end |
There was a problem hiding this comment.
Align reverse-move cursor with Move ignore-mode behavior.
OnCursorMove toggles EnableIgnoreMode on enter/exit; reverse move should mirror this to avoid inconsistent click handling.
🐛 Proposed fix
OnCursorReverseMove = function(self, identifier, enabled, changed)
if enabled then
if changed then
local cursor = self.Cursor
cursor[1], cursor[2], cursor[3], cursor[4], cursor[5] = UIUtil.GetCursor(identifier)
self:ApplyCursor()
+ self:EnableIgnoreMode(true)
end
else
+ self:EnableIgnoreMode(false)
end
end,🤖 Prompt for AI Agents
In `@lua/ui/controls/worldview.lua` around lines 537 - 549, OnCursorReverseMove
currently updates the cursor but doesn't toggle the ignore-mode like
OnCursorMove does, causing inconsistent click handling; update
OnCursorReverseMove to mirror OnCursorMove's enter/exit behavior by calling the
same EnableIgnoreMode toggle (or method/property used in OnCursorMove) when
enabled changes, so when the reverse-move cursor is applied you set
EnableIgnoreMode on entry and clear it on exit, then continue to update the
cursor and call self:ApplyCursor(); reference OnCursorReverseMove, OnCursorMove,
and the EnableIgnoreMode toggle used in the existing code.
lua/keymap/keydescriptions.lua
Outdated
| ['reverse_move'] = '<LOC key_desc_reverse_move>Reverse move.', | ||
| ['shift_reverse_move'] = '<LOC key_desc_shift_reverse_move>Reverse move.', |
There was a problem hiding this comment.
These should not end with a full stop, none of the other hotkeys (in the GitHub preview) do this.
There was a problem hiding this comment.
yea, idk where it came from :) I think I just copy-pasted something above and changed the text. Will fix it
2 participants
.exe for testing will be in Zulip -> engine-development -> reverse move
Additional context
requires FAForever/FA-Binary-Patches#116
1.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.