Opened 8 years ago
Closed 6 years ago
#4291 closed defect (fixed)
[PATCH] Doubleclicking on the hero button shows wrong building
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.
Attachments (4)
Change History (24)
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.
- 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.
comment:9 by , 7 years ago
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.
comment:10 by , 7 years ago
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)
comment:11 by , 7 years ago
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.
by , 7 years ago
Attachment: | autogarrison.patch added |
---|
follow-up: 13 comment:12 by , 7 years ago
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)
comment:13 by , 7 years ago
Replying to elexis:
Patch works as advertized, great! I'm just dubious about future repetition of this mistake.
good :)
If replacing the
SetGarrisoned
call inPromotion.js
with a different call is correct, then I'd agree to have that done and remove thatSetGarrisoned
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 havingUnitAI
we will have to callcmpUnitAI.Autogarrison
. So perhaps it would be better to do that inside ofcmpGarrisonHolder.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 acmpUnitAI.Garrison
call but somehow that doesn't bug (tested on Sicilia Nomad)
Expected as if it is cmpUnitAI.Garrison and not cmpGarrisonHolder.Garrison
comment:14 by , 7 years ago
Milestone: | Backlog → Alpha 22 |
---|
comment:15 by , 7 years ago
Milestone: | Alpha 22 → Work In Progress |
---|
comment:16 by , 6 years ago
Milestone: | Work In Progress → Alpha 23 |
---|---|
Priority: | Must Have → Should Have |
r20659 / Phab:D1146 introduced a TriggerHelper function doing that and using that on Danubius solves the problem.
comment:17 by , 6 years ago
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.
comment:18 by , 6 years ago
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
comment:19 by , 6 years ago
Nope, in fact already fixed by r19900 as noticed by elexis. So i guess the ticket can be closed.
comment:20 by , 6 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.)
r18860