[PATCH] Doubleclicking on the hero button shows wrong building

[elexis]

If a hero receives a garrison order on the same turn when it garrisons another building, doubleclicking on the hero button will show the building that isn't garrisoned by the hero.

It is easily reproducible by starting a multiplayergame, pressing F9, typing Engine.SetTurnLength(1000), sending a garrison command to one building and then sending another command one second before the unit garrisons.

The reason is the GUI function getEntityOrHolder only checking for the most recent garrison command.

[elexis]

r18860

[elexis]

Saving the garrison holder entity ID in UnitAI.isGarrisoned this way doesn't affect the outcome at all, it still saves the wrong ID.

[mimo]

I don't think this patch is the best way to fix it, as it only hides an inconsistent state which may lead to various problems. We should enforce the target of the first order to be equal to the holder whenever a unit is garrisoned, either by overwriting the orderQueue[0].target in the UnitAI SetGarrisoned fonction, or by returning false in the GarrisonHolder PerformGarrison function when the holder is different from the target order.

In addition, if sticking to this patch, it should take a possible holder upgrade into account. It seems you only have unit promotion implemented.

[elexis]

Seems to fix the issue. Saves the correct garrison holder ID in GarrisonHolder.PerformGarrison to UnitAI.isGarrisoned.

[elexis]

Replying to mimo:

• Yes, as stated the previously attached patch doesn't work at all
• No, we can't change the target of the order because the order might have been replaced with a non-garrison command too. SetGarrisoned is only called when other components set it to garrisoned, not when UnitAI itself decides to garrison.
• No, we can't add an early return to PerformGarrison because the order wasn't replaced at that moment yet, thus shows just a legitimate garrison order. The new order replacing the old one arrives after the unit was garrisoned.
• Not convinced of using the orderQueue as a way to save the garrisoned location. If the last command can't be useful anymore (because it will be replaced with the ungarrison command), then the orderQueue could be deleted in GarrisonHolder.PerformGarrison. But wouldn't it be preferable to just have code that is agnostic of the orderQueue and instead save the entity ID correctly? See the new attached patch.
• The attached patch also works with Upgrading as the component calls ChangeEntityTemplate which calls TransferGarrisonedUnits which calls Garrison.
• TODO: Also there is the fun edge-case where the hero is garrisoned in a ram that is garrisoned in a fortress or ship.

[mimo]

The problem is that some part of the code already use this target order (see for example line 1162 of UnitAI but there are a lot of other uses, and also grep "aiorder" in the petra directory). So we must keep it consistent or change a lot of UnitAI and AI logic. Then having the holder also cached in IsGarrisoned may be convenient for some cases, but i'm not sure it is worth the trouble (duplicating the same information in two places, and having to make sure both are updated when needed i.e. promotions and upgrades).

Concerning the original problem, as I said, it is only hidden by the proposed patch. The real fix is that we should prevent a garrisoned unit to be garrisoned a second time to avoid such weird state: this can simply be done with the patch

Index: binaries/data/mods/public/simulation/components/UnitAI.js
===================================================================
--- binaries/data/mods/public/simulation/components/UnitAI.js	(révision 18864)
+++ binaries/data/mods/public/simulation/components/UnitAI.js	(copie de travail)
@@ -5133,7 +5133,7 @@
  */
 UnitAI.prototype.Garrison = function(target, queued)
 {
-	if (target == this.entity)
+	if (this.IsGarrisoned() || target == this.entity)
 		return;
 	if (!this.CanGarrison(target))
 	{

Futhermore, in your case, you had a garrison order arriving just at the turn the first garrison was performed, but the second order could have been anything I suppose. So maybe we should also add the same protection to other orders, taking care that some orders should nonetheless be available to turrets (like the attack order) and for them the test should be

	if (this.IsGarrisoned() && !this.IsTurret())
		return;

Finally, for the TODO, it should be enough to use

Index: binaries/data/mods/public/gui/session/utility_functions.js
===================================================================
--- binaries/data/mods/public/gui/session/utility_functions.js	(révision 18864)
+++ binaries/data/mods/public/gui/session/utility_functions.js	(copie de travail)
@@ -114,7 +114,7 @@
 	var entState = GetEntityState(ent);
 	if (entState && !entState.position && entState.unitAI && entState.unitAI.orders.length &&
 			(entState.unitAI.orders[0].type == "Garrison" || entState.unitAI.orders[0].type == "Autogarrison"))
-		return entState.unitAI.orders[0].data.target;
+		return getEntityOrHolder(entState.unitAI.orders[0].data.target);
 
 	return ent;
 }

[elexis]

Indeed I didn't search other places in UnitAI that might assume the most recent order to be equal to the order that ordered to garrison the building. I still don't like that command being used as a state (since we might want to send other commands to garrisoned entities for any reason. But until we have such a case, your patches should fix the two issues discovered.

[elexis]

Moving to the new WIP milestone.

[mimo]

In 19118:

Fix double-click on hero button when the hero is garrisoned in a ram garrisoned somewhere, refs #4291
Reviewed By: Imarok
Differential Revision: ​https://code.wildfiregames.com/D46

[elexis]

If the player issues a command while the GUI is at turn N, the command will be executed on turn N+2, which should explain why this garrison command arrives late (also explains bartering after losing the market #3645 and perhaps even a notification of being attacked by an allied captured ram #3484)

[elexis]

Bug: On the danubius map, gaia heroes are garrisoned without sending an order, just with the cmpGarrisonHolder.Garrison command, therefore doubleclicking on the hero portrait doesn't work, as that looks up the garrison holder by analyzing the topmost command.