[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.

[mimo]

Should be enough to replace line 730 of GarrisonHolder.js by cmpUnitAI.Autogarrison(this.entity); In fact, the previous line was not correct as the garrisoned entity was not in the right state.

[elexis]

Bug description:

On danubius, the gaia hero is spawned about half an hour after the game started and garrisoned into a ship (so the GarrisonHolder.prototype.OnGlobalInitGame change probably is probably correct, but probably doesn't fix this bug).

It is garrisoned using the cmpGarrisonHolder.Garrison(ent) line in danubius_triggers.js and the effect is that it is garrisoned in the same turn, the icons of garrisoned units are displayed when clicking the ship in observermode, but the hero icon in the top panel doesn't react when doubleclicking (since the unit didn't receive an order).

When changing this line to Engine.QueryInterface(ent, IID_UnitAI).Autogarrison(ship); doubleclicking works, but for any reason the icons are not visible anymore when clicking on the ship in observermode.

When changing it to Engine.QueryInterface(ent, IID_UnitAI).Garrison(ship, false);, the doubleclicking doesn't work and the icon isn't shown in the panel either.

When changing it to ProcessCommand(0, { "type": "garrison", "entities": [ent], "target": ship, "queued": false }); it also doesn't show the portrait when clicking on the ship and the doubleclicking doesn't work either.

(For easier testing, set heroProbability to return 1 and attackersPerShip to return 10)

[mimo]

Sorry, i was not clear but the change in initGarrison was just to show how it should be done in general, and not fix the danube case.

The cmpGarrisonHolder.Garrison() must be kept so that the component is correctly set, but the cmpUnitAI.Autogarrison() must also be called to correctly set the UnitAI state (this also sets the UnitAI garrisoned flag and thus the cmpUnitAI.SetGarrisoned is no more needed). I attach a (untested) patch which should hopefully fix it, if you have time to test it that would be nice, otherwise i'll try to test it later.

To be complete, there is still a SetGarrisoned in Promotion.js which could also be changed, but as Promotion.js copies the full order array, the Autogarrison order is not needed.

[elexis]

Patch works as advertized, great! I'm just dubious about future repetition of this mistake.

If replacing the SetGarrisoned call in Promotion.js with a different call is correct, then I'd agree to have that done and remove that SetGarrisoned function altogether.

The danubius change seems to indicate that each time we call cmpGarrisonHolder.Garrison and that entity having UnitAI we will have to call cmpUnitAI.Autogarrison. So perhaps it would be better to do that inside of cmpGarrisonHolder.Garrison directly?

Also since regular garrisoning via commands isn't apparently broken, I wonder why sending a regular garrison command for gaia didn't work.

Regicide.js also has a cmpUnitAI.Garrison call but somehow that doesn't bug (tested on Sicilia Nomad)

[mimo]

Replying to elexis:

Patch works as advertized, great! I'm just dubious about future repetition of this mistake.

good :)

If replacing the SetGarrisoned call in Promotion.js with a different call is correct, then I'd agree to have that done and remove that SetGarrisoned function altogether.

not sure as we don't do a cmpGarrisonHolder.Garrison(), we just copy everything from the original unit to the promoted one.

The danubius change seems to indicate that each time we call cmpGarrisonHolder.Garrison and that entity having UnitAI we will have to call cmpUnitAI.Autogarrison. So perhaps it would be better to do that inside of cmpGarrisonHolder.Garrison directly?

Not inside cmpGarrisonHolder.Garrison, but rather that cmpUnitAI.Autogarrison() calls cmpGarrisonHolder.Garrison (would be more logical). But in fact, looking at the code, i'm not sure anymore if it should call cmpGarrisonHolder.Garrison or cmpGarrisonHolder.PerformGarrison as is done in ProductionQueue and why we have this distinction: ProductionQueue was at the origin of this Autogarrison stuff when we wanted to produce entities already garrisoned without having to spawn them outside the structure and reenter immediately into it (which would anyway not work for units in ships).

Also since regular garrisoning via commands isn't apparently broken, I wonder why sending a regular garrison command for gaia didn't work.

No idea. We should add some warn to see which test prevents it.

Regicide.js also has a cmpUnitAI.Garrison call but somehow that doesn't bug (tested on Sicilia Nomad)

Expected as if it is cmpUnitAI.Garrison and not cmpGarrisonHolder.Garrison

[elexis]

r20659 / ​Phab:D1146 introduced a TriggerHelper function doing that and using that on Danubius solves the problem.

[elexis]

In r21445:

Use mimos garrison function from rP20659 / D1146 to support doubleclicking on garrisoned gaia heroes on Danubius, fixing the bug described in comment:10 of #4291.

mimo do you recall if the bug originally described in the ticket description was fixed? I don't see the commit fixing it but I can't reproduce it and getEntityOrHolder in session.js still looks for the first order.

[mimo]

Looking at the code, getEntityOrHolder seems to have been fixed for the recursive stuff (last patch of comment #3), but i think the first patch of that comment #3 (not answering a garrison order when already garrisoned) is still missing.

I think that was the root of the bug, the hero had a garrison order, move to the structure and on the turn it was going to be garrisoned the player issued another garrison order in a different structure from which it was also in garrisonRange. It is certainly very rare but could on theory happen again, so i'd rather have the patch.

see ​phab:1357

[mimo]

Nope, in fact already fixed by r19900 as noticed by elexis. So i guess the ticket can be closed.

[elexis]

Indeed! Thanks for writing the fixes to all three reported bugs, r19900, r19118, r20659+r21445.

(I think those three were all reported bugs here. Some other cases like Upgrading were considered already. I guess it's still possible to write code that doesn't call the Autogarrison function when needed, but meh.)