[PATCH] Changing stance on garrisoning units causes instant garrison

[historic_bruno]

As reported on the forum, units will garrison instantly regardless of location when ordering them to garrison, then setting their stance to Passive, Defensive, or Stand. For some reason other stances don't cause this problem. This can be considered a fairly serious cheat.

[Kieran P]

Upgrading to release blocker, because if it doesn't cause OOS, then it's a cheat that no one will notice (unlike the dev overlay which relays the message).

[historic_bruno]

The problem is switching to the above stances sends a StopMoving order to the unit, which triggers a "MoveCompleted" event; if the unit was in the GARRISON.APPROACHING state, then from the "MoveCompleted" it's assumed the target was reached - there's no range check! This is a serious bug in garrisoning, you can instantly garrison to any point on the map, if the unit stops moving while approaching the target (it's unreachable, stances changed, etc.)

According to Michael, changing stance should not cause a unit to stop moving except for Stand. Then we still need some kind of range check for garrisoning, can we use the same range check for boats and land units? (both use GarrisonHolder)

Related minor bugs:

• If unit is tasked to repair and approaching the target, they will start the repair animation when receiving "MoveCompleted", even if out of target range
• If unit is tasked to attack and approaching the target, they will start the attack animation when receiving "MoveCompleted", even if out of target range

[leper]

I would recommend to only stop moving on stand if the current unit movement wasn't caused by the player (as in data.force == false). Force should be added to Garrison(target, qued):

replace

this.AddOrder("Garrison", { "target": target }, queued);

with

this.AddOrder("Garrison", { "target": target, "force": true }, queued);

This would fix this bug as garrison won't be caused by UnitAI (if it does we should add another parameter to Garrison that is only set (to true) if it is called by the Gui code (or AI)).

[historic_bruno]

Replying to leper:

This would fix this bug as garrison won't be caused by UnitAI (if it does we should add another parameter to Garrison that is only set (to true) if it is called by the Gui code (or AI)).

That would cover up the really serious bug, which is that garrisoning doesn't do range checks. There is a lesser bug concerning exactly what to do when the stance is changed to stand ground, but it doesn't have to be fixed now. In general, it does seem useful to know whether the current action was a player order or not.

I think garrisoning will always be "forced" in response to a player order.

[historic_bruno]

Attached a patch with possible fix.

It adds a range check to garrisoning, using a new optional <LoadingRange> element in GarrisonHolder, which defaults to 2 units from the garrison holder's obstruction. Exposing this range allows ships to have a farther loading distance, I guess you could say it simulates gangways or a small boat for loading purposes :) I tested and a max range of 10 seems reasonable for boats. The default value works for structures and siege units.

I had to add a new function in UnitAI to check the range, since the existing CheckTargetRange() assumes the range data comes from the source entity.

[leper]

The patch is looking fine (though I am not qualified to review this). But I would suggest adding a force parameter to SwitchToStance and modify the if to look like if (stance == "stand" && !force). This would allow the unit to execute the current task (started by the player) and stop afterwards or stop immediately if the unit is just chasing a enemy.

[historic_bruno]

New patch with Philip's suggestion of making <LoadingRange> a required element. That way bugs won't sneak in if someone creates a new boat or something :) Also I cleaned up some uses of <GarrisonHolder> that weren't using inheritance.

[historic_bruno]

The latest patch has a bug where if the garrison target is only temporarily unreachable, it will give up. Philip mentioned earlier in IRC (discussing trade) that the preferred behavior is to keep trying to reach the target and not worry about the pathfinding performance yet. I will update my patch with a fix.

[ben]

In 11292:

Fixes stance change during garrison causing instant garrison. Fixes #1184.
Adds range check to garrisoning, with <LoadingRange> element added to GarrisonHolder. Adds maximum range values to templates (2 for land units/structures, 10 for ships).
Adds retry to garrison failure due to unreachable target.
Cleans up a few templates.