beyond-all-reason / beyond-all-reason/RecoilEngine

Game defined commands of type `CMDTYPE_ICON_MODE` do not update cmddesc.params[0]

Open
#2,019 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
679
Forks
290
Avg merge
3d 2h
Merged PRs (30d)
40

Description

Notice if a command is of the type `CMDTYPE_ICON_MODE`, it is handled when sent through unsynced space [here](https://github.com/beyond-all-reason/spring/blob/f30aea20ae92a148009e573d844c20fa0bb31671/rts/Game/UI/GuiHandler.cpp#L1361-L1365). Also notice the setting of `param[0]` in the units commanddescription for holding current state, this is not the actual units command description but just a state holder for `GuiHandler`.

The same command, when given via Synced `Spring.GiveOrder` [goes straight to `CommandAI`](https://github.com/beyond-all-reason/spring/blob/f30aea20ae92a148009e573d844c20fa0bb31671/rts/Lua/LuaSyncedCtrl.cpp#L5435-L5435).

`CommandAI` actually does set param0, but [only for its hardcoded list of state commands](https://github.com/beyond-all-reason/spring/blob/f30aea20ae92a148009e573d844c20fa0bb31671/rts/Sim/Units/CommandAI/CommandAI.cpp#L862-L862).

There are at least 2 bugs here:

- GuiHandler updates the internal list of commands cmddescr param0 _before it sends the command through the network_, ignoring the fact the order might be disallowed by lua. Fortunately this is a less serious bug, this is only relevant for native engine UI display.
- Commands issued do not perform the state command change, as per the convention of using param0 to hold state.

For games wishing to retrieve the state of a unit for that state command, they are unable to, at points resorting to [editing the cmddesc manually](https://github.com/beyond-all-reason/Beyond-All-Reason/pull/4397) since this is the convention for engine defined commands and should maintain for game defined ones.

## Proposed fix

Inside `CCommandAI::ExecuteStateCommand`, right after the internal command handling but before the non-queing commands handling:

- Iterate on `possibleCommands`
- Match when `command.id == possibleCommand.id` and `possibleCommand.type == CMDTYPE_ICON_MODE`
- Call `SetCommandDescParam0(command)`

Now, as to the subject of returning true at this stage, that should be the case in my opinion. All other state commands do not care about queuing or similar handling. This "breaks" previous behavior but previous behavior was the broken case. Anyway I leave figuring this out as an exercise to the contributor.

Contributor guide

Open the contributing guide

Research direction

Start in rts/Sim/Units/CommandAI/CommandAI.cpp at CCommandAI::ExecuteStateCommand and compare its handling with rts/Game/UI/GuiHandler.cpp and rts/Lua/LuaSyncedCtrl.cpp. Trace possibleCommands and the command-description state holder before deciding how accepted game-defined icon-mode commands should behave. Done means the state is updated consistently for synced orders without the GUI recording a change before the order is accepted.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
game-dev
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.