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 Test_integration.exe --run_test=AttackSuite/TroopLimitKeepsOrderedRestrictedSoldier --report_level=short
  • Result: passed, 1 test case, 36 assertions
• Ran Test_integration.exe --run_test=AttackSuite --report_level=short
  • Result: passed, 22 test cases, 1225 assertions
• Ran git diff --check upstream/master...HEAD

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

[DevOpsOfChaos]

I’d keep this PR limited to the current RTTR behavior for now.

So this does not try to restore the full original S2 behavior where ordered soldiers always enter the military building first. It only keeps the existing RTTR immediate-cancel behavior safe by checking whether the soldier rank actually has an accepting return warehouse before cancelling the order.

If the preferred direction is to restore the original S2 behavior instead, I think that should be a separate decision/PR, because it changes the broader military-setting semantics beyond this specific no-valid-return-warehouse case.

[Flamefire]

I’d keep this PR limited to the current RTTR behavior for now.

I'm ok with this.

reuse the same rank-specific warehouse acceptance check for stationed soldiers in the troop-limit path

This isn't met though: The added check is different. But the existing check is incomplete and should be changed to the new one that checks if the warehouse accepts this figure

[DevOpsOfChaos]

I updated this so ordered and stationed soldiers now use the same rank/figure-specific return-home check.

The shared check uses the soldier’s actual job type with FW::AcceptsFigure(...), and the reachable accepting-warehouse result is cached by rank to avoid repeating the expensive lookup for multiple soldiers of the same rank.

Validation:

• built Test_integration
• ran AttackSuite/TroopLimitKeepsOrderedRestrictedSoldier: 1 test case, 36 assertions passed
• ran AttackSuite: 22 test cases, 1225 assertions passed
• git diff --check upstream/master...HEAD

[Flamefire]

LGTM.