Opened 8 years ago
Last modified 6 years ago
#4291 closed defect
[PATCH] Doubleclicking on the hero button shows wrong building — at Version 8
Reported by: | elexis | Owned by: | |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 23 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
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.
Change History (11)
by , 8 years ago
Attachment: | commands.txt added |
---|
by , 8 years ago
Attachment: | unitAI_isGarrisoned_broken_v1.patch added |
---|
Saving the garrison holder entity ID in UnitAI.isGarrisoned
this way doesn't affect the outcome at all, it still saves the wrong ID.
comment:1 by , 8 years ago
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.
by , 8 years ago
Attachment: | unitAI_isGarrisoned_v2.patch added |
---|
Seems to fix the issue. Saves the correct garrison holder ID in GarrisonHolder.PerformGarrison
to UnitAI.isGarrisoned
.
comment:2 by , 8 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 22 |
Summary: | Doubleclicking on the hero button shows wrong building → [PATCH] Doubleclicking on the hero button shows wrong building |
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 theorderQueue
could be deleted inGarrisonHolder.PerformGarrison
. But wouldn't it be preferable to just have code that is agnostic of theorderQueue
and instead save the entity ID correctly? See the new attached patch. - The attached patch also works with
Upgrading
as the component callsChangeEntityTemplate
which callsTransferGarrisonedUnits
which callsGarrison
. - TODO: Also there is the fun edge-case where the hero is garrisoned in a ram that is garrisoned in a fortress or ship.
comment:3 by , 8 years ago
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; }
comment:4 by , 8 years ago
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.
comment:7 by , 7 years ago
comment:8 by , 7 years ago
Description: | modified (diff) |
---|---|
Keywords: | review removed |
Milestone: | Work In Progress → Backlog |
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.
r18860