Units attacking twice

[lmoureaux]

Describe the bug
It has been reported that sometimes units attack twice without being asked. For one user attack is automatic, for another the action choice popup is shown again after attacking. This does not always happen.

Will try to investigate for v3.1 but fixing this is not critical.

To Reproduce
Steps to reproduce the behavior:

1. Attack a city with a stack
2. Repeat until the same unit attacks twice

Expected behavior
Units don't attack without being asked.

Platform and version (please complete the following information):

• OS: Linux
• Freeciv21 version: assuming 3.1-beta.1
• Ruleset/Longturn game (if applicable): Sim06

Additional context
We have more action dialog issues: #768 #2039

[hugomflavio]

I'm still running the last version you asked me to test (the one with the fixed maps).

[hugomflavio]

logged back in just to see how the server recorded the attacks over time. 0 to 2 seconds difference, it seems:

[lmoureaux]

When the client gets a unit_actions packet with disturb_player set, it either performs the default action or pops up the action selection dialog depending on the settings. Need to investigate why the server thinks it should perform the same action again.

[lmoureaux]

The server only ever sends a unit_actions packet when the client requests it by sending unit_get_actions.

[lmoureaux]

...which is itself sent upon receiving a unit packet and the packet's action_decision_want is different from what the player knows about the unit (either because the unit is new or from what the client already knew). action_decision_want can be Nothing, Passive, or Active. Only Active always triggers the dialog.

[lmoureaux]

The GUI is supposed to call action_decision_clear_want() whenever it no longer wants to be bugged about a unit.

[lmoureaux]

I eliminated an entire civ with Combat_Rounds set to 2 (what a pain!) and couldn't see this. Maybe with occupychance...

[lmoureaux]

No difference after a few attempts with occupychance

[hugomflavio]

It is true that it seems to come and go. This morning I saw it happen several times over a single turn, but other times I can carry a whole turn's worth of attacks without noticing it happen...

…

On Sat, 2 Nov 2024, 22:50 Louis Moureaux, ***@***.***> wrote: I eliminated an entire civ with Combat_Rounds set to 2 (what a pain!) and couldn't see this. Maybe with occupychance... — Reply to this email directly, view it on GitHub <#2431 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJUETHPD3DNAUVV74JUSRULZ6V6OPAVCNFSM6AAAAABRCAFUJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJTGI2TMOBVGI> . You are receiving this because you commented.Message ID: ***@***.***>

[lmoureaux]

Maybe a network delay is involved then... :(

[hugomflavio]

Are you testing it on a local server? Could it have something to do with reaching the remote server and returning?

[lmoureaux]

Yes... maybe I need to introduce a delay in the server response...

[hugomflavio]

Well, there's always tomorrow's turn 😄

[lmoureaux]

Reproduced it! With questions disabled:

1. Add a sleep(10) in unit_versus_unit (unittools.cpp)
2. Attack a city (the unit needs plenty of MP)
3. While the unit is attacking, select another unit
4. The initial unit attacks twice

[lmoureaux]

It's not necessary to select another unit. Clicking on the attacker while the server is busy is enough.

[lmoureaux]

The issue is that while action_decision_want is active, selecting the unit causes the client to trigger the action selection process.

[lmoureaux]

There are two flags:

• A per-unit flag (action_decision_want) that the server sets when it wants input for an action. When a unit is selected and the flag is set, the client asks the server for actions to perform. The client can also ask the server to clear the flag. This is asynchronous.
• A global client-side flag (action_selection_in_progress_for) used by the client to prevent multiple action dialogs from popping up.

The fundamental issue is the client-side flag:

• To prevent double attacks, it must remain set until the per-unit flag is cleared by the server. This is currently not the case.
• When moving focus to another unit, we want to query the server immediately for the next unit's actions. This requires clearing the global flag earlier.

I think we need a second per-unit flag that the client can use to prevent duplicate requests for the same unit without constraining (or adding latency to) focus change.

[hugomflavio]

indeed, I was able to consistently reproduce by selecting another unit before the attack actually went through.

[lmoureaux]

On top of the above, managing state in the action dialog is a nightmare of static variables and functions that need to be called in a precise order from multiple places. I think a rewrite is needed...