Check warehouse acceptance before cancelling ordered troops

[DevOpsOfChaos]

Summary

• check whether a warehouse accepts a soldier rank before cancelling excess ordered troops
• reuse the same rank-specific warehouse acceptance check for stationed soldiers in the troop-limit path
• add a regression test for ordered soldiers whose return warehouse rejects their rank

Motivation

The troop-limit branch in nobMilitary::RegulateTroops() cancelled ordered_troops before checking whether any warehouse accepted the soldier's concrete rank. This could send restricted ordered soldiers home into the same no-accepting-warehouse path that was handled for stationed soldiers separately.

This keeps ordered soldiers assigned to the military building unless a rank-compatible warehouse exists.

Validation

• Built Test_integration locally with Visual Studio 2022 Debug
• Ran AttackSuite/TroopLimitKeepsOrderedRestrictedSoldier locally
• Ran AttackSuite locally

[Flamefire]

@Spikeone What was the original behavior in S2? I guess this can be triggered by building a fortress, disable soldiers accept in HQ, wait for soldiers to leave HQ, then change military setting.

@DevOpsOfChaos What does happen in RTTR without this change that this fixes/changes exactly?

[DevOpsOfChaos]

Without this change RTTR cancels the already ordered soldier in the troop-limit branch before checking whether any connected warehouse accepts that soldier rank.

Concrete sequence:

1. A soldier is ordered from the HQ to the military building.
2. The soldier has already left the HQ, so he is in ordered_troops for the military building.
3. The HQ is then changed to reject that soldier rank.
4. The troop limit for that rank is reduced to 0.
5. RegulateTroops() sees excess for that rank.
6. Current RTTR removes the matching entry from ordered_troops and calls NotNeeded() on that soldier.
7. Only after that, the stationed-soldier path checks whether a rank-compatible return warehouse exists.

So the ordered soldier is cancelled even though there is no valid return warehouse for his concrete rank. This differs from the stationed troop path, which already only sends soldiers home when FindWarehouse(... FW::AcceptsFigure(SOLDIER_JOBS[rank]) ...) succeeds.

This PR makes the ordered-troop path use the same rank-compatible warehouse check before cancelling the order. If no accepting warehouse exists, the soldier remains assigned to the military building and can still arrive there.

[Spikeone]

@Flamefire

The orignal is storing out regardless:

Although I sometimes hate this behavior, this is part of the game and I think pretty cool - because that way you may destroy the only storehouse that accepts generals, the enemy forgets about it and loses because his generals just die (happend to me quite often). Also since most people use the collect feature nowadays, I don't see a high risk for this "problem" anymore. I don't think we should change this, since this allows you to transfer soldiers from two disconnected areas (although that won't happen that often, it is a nice detail).

[Flamefire]

@Spikeone

The orignal is storing out regardless:

That's not exactly what is happening here: Here It is blocking to accept soldiers and then cancel some on their way to a building.
To me it would be consistent that they wander around

So the ordered soldier is cancelled even though there is no valid return warehouse for his concrete rank. This differs from the stationed troop path, which already only sends soldiers home when FindWarehouse(... FW::AcceptsFigure(SOLDIER_JOBS[rank]) ...) succeeds.

@Spikeone Similar here: What happens when you change soldier occupancy when no warehouse accepts them?

I agree that those 2 behaviors should be consistent at least, unless the original is different and that is not "a bug"

[Spikeone]

Okay tested:
Are soldiers recruited if storing is is disallowed: Yes

Changing the settings while the building is initially occupied seems to not affect the soldiers that have been called already

Soldiers are not stored out until they are connected to a warehouse that accepts them.

This does make sense to me.

Store in stop is not tied to the store out option.

Therefore it does make sense, that you are able to store out your soldiers and they do die since they are not allowed back in. In the original you can even store them out but if you do not prevent them from storing in, they find home - and are ejected again.

The issue on our side is, that you are able to cancel soldiers that are on their way to a building - this is wrong:

So soldiers always have to first enter the building to be called back. If then no warehouse is connected, soldiers do not go back at all but stay (regardless of settings).

[Flamefire]

Therefore it does make sense, that you are able to store out your soldiers and they do die since they are not allowed back in.

"Store out" refers to the warehouse setting, not the military setting?

Ok so correct behavior would be:

• Never cancel ordered soldiers
• Only send troops back that are there already and if they have a warehouse to go to

So it is possible that you change the setting causing soldiers to got to the building and return to wh after arrival.
I just checked: That's what the original does.

Do we consider this a bug we fixed here by cancelling ordered troops or as a QoL enhancement or shall we keep the (strange) original behavior?

I would go with the fix and hence what is done here: Try sending soldiers back ASAP but only ever if possible.

@Spikeone @Flow86 What do you say?

[Spikeone]

It's really hard to tell.

I always referred to the original where the gold stop on a building was not immediate, so if you forgot it you had to cut the road for a short moment (as far as I remember even a ware that was currently carried would not change it's destination until the next carrier would pick the ware up, thus it would not cancel it's target).

I never knew, that soldiers walking back is affected in the same way and I think it is quite a change compared to the original. In RttR it's easy to react to a misstake you made, while in the original you'd have to change the settings and possibly cut a road.

That said, I think the RttR way is a QoL way most people got used to. But I also see the use case in multiplayer where this requires more planning than currently. So I'd really appreciate the original implementation and would like to have it in RttR as well, but would keep the "immediate effect" as an addon maybe?

[Flamefire]

I never knew, that soldiers walking back is affected in the same way and I think it is quite a change compared to the original. In RttR it's easy to react to a misstake you made, while in the original you'd have to change the settings and possibly cut a road.

That said, I think the RttR way is a QoL way most people got used to. But I also see the use case in multiplayer where this requires more planning than currently. So I'd really appreciate the original implementation and would like to have it in RttR as well, but would keep the "immediate effect" as an addon maybe?

Is it really such a deviation? The "mistake" here is moving the slider a bit too much, so you already changed the setting.
I doubt many people would notice that at all, especially no one seems to have noticed it until now.

But then again, given this is such a small detail we could "fix" the RTTR behavior to match the original one and likely no one would notice that, would they?

[Spikeone]

But then again, given this is such a small detail we could "fix" the RTTR behavior to match the original one and likely no one would notice that, would they?

For resources like coins: You would notice this immediatly if you do not play with the "disable coins by default" addon.

For soldiers: I think you'd also notice this. Sometimes you may have 5+ Fortresses you accidently occupy - so moving 40 soldiers is quite much. Also sometimes at least I missclick and occupy inland buildings which results in lots of soldiers moving which I then can stop by simply changing the setting again. Also sometimes I use our RttR way to check if I forgot to disconnect a building by maxing the setting, having a look if a soldier starts moving, if yes, watch which direction and then lower the setting to disconnect the buildings (so in that case it wouldn't change much, since I want to disconnect them anyways).

Conclusion: I think it does have an impact. Mostly for multiplayer, and you'd have to manage slightly more. I think we should always try to match the original - but also allow QoL settings.

[Flamefire]

Conclusion: I think it does have an impact. Mostly for multiplayer, and you'd have to manage slightly more. I think we should always try to match the original - but also allow QoL settings.

To me this wouldn't be worth an addon: I'd keep the RTTR QoL improvement.
I doubt people would ever want to disable it, would they?
If an addon I'd make it so it enables S2 behavior, so RTTR is the default here. That would deviate from our other defaults though.