Improve blitz validation & add property for attacking from contested territories.

[asvitkine]

Change Summary & Additional Notes

Improve blitz validation & add property for attacking from contested territories.

This change makes several improvements to combat move validation, including handling combinations of blitzing and air/land transported units correctly. It also adds a property to allow maps to permit attacking from contested territories unrelated to blitzing.

More specifically, the following improvements are done:

1. When moving with a combination of blitzing, air transports, land transport units and non-blitzing units, the engine will now correctly determine which units are allowed to move. Previously, it would permit any land transportable units with enough movement to tag along in the presence of just one blitzing land transport and similar for air transports. Now this is validated explicitly. The following saved game on Pacific Challenge with infantry modified to have 2 movement can be used to test. Prior to this change, including on 2.5, you can take a land transport and 6 infantry units and blitz with them, for example, which is not correct (and same for air transport).
   blitz_test_inf2.tsvg.zip
2. Additionally, makes MoveValidator use the passed in mapping of units to air transports for validation, instead of calculating a different mapping. The passed in mapping is also validated. Note: Air transport moves always come with this mapping provided, so the code path of creating a mapping automatically in MoveValidator is removed.
3. A new property ("All Units Can Attack From Contested Territories") is added to allow attacks out of contested territories (regardless of blitz ability or territories being blitzable), defaulting to false, so maps that may want that behavior can set that. If not set, the restrictions are as before (which this PR adds tests for). Maps like Imperialism 1974 and AA 1914 will want to set this, although AA 1914 may require further refinement with additional properties since the rules about leaving contested territories are more nuanced.
4. Expands test suite to cover the different cases, including a ton of tests for existing behavior that was previously untested, and how the behavior is affected by the added property. Additionally, a number of tests for paratrooper and mech info tests are added, verifying the fixes of this PR.

Tested:

• Various blitz scenarios, including using land and air transports, including using an edited game state on Pacific Challenge map with land and air transports and editing infantry to have 2 movement (see save file above).
• Moving out of contested territory on Imperialism 1974 without the property set:
  imperialism_contested.tsvg.zip
  • The move is still not allowed, but has the new error message that doesn't mention blitzing
• Setting the new property on Imperialism 1974 and verifying that units can move out of contested territories. Save that includes the new property:
  imperialism_contested_new_property2.tsvg.zip

Release Note

FIX|Improved validation of blitzes in the presence of land and air transports and better move error messages. NEW|"All Units Can Attack From Contested Territories" game property to allow all units to attack from contested territories.

[asvitkine]

@Cernelius @panther2

I'd like to get your thoughts as rule experts if this is the right thing to do. One option is to make it a map property - but that would still result in the wrong error message for current maps.

[codecov]

Codecov Report

Merging #10617 (f7647f5) into master (2afed3f) will increase coverage by 0.05%.
The diff coverage is 69.86%.

@@             Coverage Diff              @@
##             master   #10617      +/-   ##
============================================
+ Coverage     28.10%   28.16%   +0.05%     
- Complexity     7998     8018      +20     
============================================
  Files          1197     1197              
  Lines         76982    76985       +3     
  Branches      10526    10522       -4     
============================================
+ Hits          21633    21680      +47     
+ Misses        53396    53361      -35     
+ Partials       1953     1944       -9     

Impacted Files	Coverage Δ
...rc/main/java/games/strategy/triplea/Constants.java	100.00% <ø> (ø)
...games/strategy/triplea/ai/pro/ProCombatMoveAi.java	0.39% <0.00%> (+<0.01%)	⬆️
...mes/strategy/triplea/ai/pro/data/ProTerritory.java	0.00% <0.00%> (ø)
...ames/strategy/triplea/ai/pro/logging/ProLogUi.java	21.05% <0.00%> (ø)
...trategy/triplea/ai/pro/util/ProTransportUtils.java	0.00% <0.00%> (ø)
...riplea/delegate/move/validation/MoveValidator.java	55.43% <86.20%> (+1.57%)	⬆️
...c/main/java/games/strategy/triplea/Properties.java	72.05% <100.00%> (+0.94%)	⬆️
...rc/main/java/games/strategy/engine/data/Route.java	68.04% <0.00%> (-0.60%)	⬇️
.../java/games/strategy/triplea/delegate/Matches.java	51.92% <0.00%> (+0.22%)	⬆️
...trategy/triplea/delegate/battle/BattleTracker.java	55.11% <0.00%> (+0.43%)	⬆️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 259f015...f7647f5. Read the comment docs.

[asvitkine]

Hmm, I took a look at the A&A 1914 rules, which also use 1-round combat.

Land units that begin the turn in contested territories can only be moved to territories that are controlled by your power, or to territories that are also contested and already contain units belonging to your power. If they are moved by transport, they may also remain at sea.

So, that's not the same logic as this. That requires checking if the destination territory is contested. Though, implementing A&A 1914 rules is its own topic I suppose, but if we want to implement them, some additional properties would need to be added to control this logic.

[Cernelius]

Hmm, I took a look at the A&A 1914 rules, which also use 1-round combat.

Land units that begin the turn in contested territories can only be moved to territories that are controlled by your power, or to territories that are also contested and already contain units belonging to your power. If they are moved by transport, they may also remain at sea.

So, that's not the same logic as this. That requires checking if the destination territory is contested. Though, implementing A&A 1914 rules is its own topic I suppose, but if we want to implement them, some additional properties would need to be added to control this logic.

Are you saying that, under these rules, you can move directly from a contested territory to an enemy owned territory with enemy units in it as long as you have one or more units in it too, yet you cannot move from a contested territory to an empty enemy owned territory? If so, I would say this makes no sense: it should be easier to move into an empty enemy owned territory than to move into an enemy onwed territory crowded by a million enemy units and one of your units...

How about land units which begin the turn in enemy territories devoid of enemy units (in case there is no taking control of such territories immediately)? Are they treated exactly like those starting in contested territories?

Are there any differences between starting in a contested territory which you own and starting in a contested territory which your enemy owns, or ownership just doesn't matter so long as the territory is contested?

My suggestion here is having a few properties, to define what you can do and keeping the default behaviour as simple and generic as possible (documenting it in "PoS2"). This should allow map-makers to choose what they want and would have the benefit of telling everyone the behaviour of the game, to some extent.
For example, having a property like "Units can move between contested zones". If this property is false, a unit starting in a contested territory can never move into an enemy owned territory, whereas it can if this property is true and the enemy territory is contested.

---

In 2017, I've tested the program and written the following explanations as how it works:

If you start your turn into a hostile land territory, you are allowed to go out and then into the same (to create retreat ways for your land units) (this is possible only if the land territory you go into and then out is friendly) (only multiple movement units can do it).

If you start your turn into a hostile sea zone, you are allowed to go out and then into the same (to create retreat ways for your sea units) (this is possible only if the sea zone you go into and then out has no enemy sea units in it (it can be an enemy owned convoy zone you capture)), with no need of loading units.

If you start your turn into a hostile land territory, you are allowed to go out of it, of 1 movement only, to an adjacent friendly land territory or sea zone (in this second case, you have to load onto sea units), ending your Combat Move for that land unit at this point (escaping combat), unless you want to perform a normal Combat Move movement. Then, you can fully move or unload from sea units, at any point during Non Combat Move, with any units that made such single movements (to escape Combat), during Combat Move, just like any land units that didn't move during Combat Move, except only that, if a land unit moved into an adjacent friendly land territory, during the Combat Move phase beforehand, it will have 1 movement less (meaning that it will be, either, unable to move, if it is a movement 1 unit, or able to move of 1 land territory only, if it is a movement 2 unit, since it already moved of 1 land territory), and will be unable to load onto sea units, and that if a land unit loaded onto sea units of other players, it will be unable to unload, on that same turn (practically, as long as the Non Combat Move phase is concerned, that single movement made during Combat Move is just like it was made during Non Combat Move; and indeed it is to be regarded as a Non Combat Move movement irreversibily (no edit granted, unless agreed) anticipated during Combat Move).

If you start your turn into a hostile sea zone, you are allowed to go out of it, of 1 movement only, to an adjacent friendly sea zone (and eventually load any land units from any hostile territories), ending your Combat Move for that land unit at this point (escaping combat), unless you want to perform a normal Combat Move movement. Then, you can fully move, as well as load and unload units, at any point during Non Combat Move, with any units that made such single movements (to escape Combat), during Combat Move, just like any sea units that didn't move during Combat Move, except only that, if a sea unit moved into an adjacent friendly sea zone, during the Combat Move phase beforehand, it will have only 1 movement left (practically, as long as the Non Combat Move phase is concerned, that single movement made during Combat Move is just like it was made during Non Combat Move; and indeed it is to be regarded as a Non Combat Move movement irreversibily (no edit granted, unless agreed) anticipated during Combat Move).

This is how the program worked back in 2017, according to my experience. I do know that something changed since then.

Something similar should now be written to detail how the program currently works.

[Cernelius]

@Cernelius @panther2

I'd like to get your thoughts as rule experts if this is the right thing to do. One option is to make it a map property - but that would still result in the wrong error message for current maps.

Yes, I suggest having contested-to-friendly only as the basis and addining any further movement allowances (like being able to go from contested to already contested and whatever this PR is about) as well-worded properties (you need to have true).

Since now also land units can be unable to block or able to move past other units, if either all enemy units that are contesting the territory against you are unable to block other units or all units you are trying to move are able to move past them, then the case should be handled as if the starting territory was non contested (so that you can move directly even into an uncontested enemy territory with enemy units able to block your units).

Moreover, the matter should be the same as land for convoy zones (sea territories) if you cannot enter enemy convoy zones during Non Combat Move (there is a property for this too).

[panther2]

@asvitkine

Allow moving land units out of contested territories they start in.

Only from a core-boardgame-rules perspective this feature would be restricted to 1914 and its ruleset only.

Any variation maybe a welcome option for 1914 or any custom mapmaker game.

Otherwise it of course reminds me of the "Sea Units starting in hostile Seazones" - implementation - brought to land territories.

[Cernelius]

@panther2 Is it true that, in 1914, my sea units starting in a contested sea zone can move to an adjacent sea zone which is occupied by enemy units and no other units, whereas my land units starting in a contested land zone cannot move to an adjacent land zone which is occupied by enemy units unless the adjacent zone is also already occupied by one or more units each of which is owned by me or one of my allies?

If so, we have a different handlying of the case in 1914, possibly depending on whether or not the zone is land or sea.

[panther2]

@Cernelius With "otherwise" I meant the wwII-related games. And the "reminds me of" was just a hint towards a code implementation that meets the "Sea unit starting in hostile Sea Zones"-rules of the WWII games, that maybe could somehow be adapted/altered for use for territories in user made games, too.

Concerning 1914 I am not familiar with the boardgame ruleset, as I have never been interested in this WW I scenario.
Of course - if really needed - I would take a look into the rulebook, but in case you, @Cernelius , are more familiar with it, I would prefer to focus on other things, right now.

[asvitkine]

I guess my problem with the current behavior is the error is talking about blitz which is completely unrelated.

Otherwise, the map that's affected by this is Imperialism 1974 where how it should work is that you should be able to move troops through any territories and can choose or not to attack (either other players or neutrals). The land battles being optional is another triplea property that needs to be added (it exists for sea battles but not land battles).

[asvitkine]

@Cernelius Thanks. I've switched to split the blitz and no-blitz cases into separate checks such that the "no blitz" case will now have an error message that doesn't mention blitzing ("Cannot attack out of a contested territory"). I've added a property to allow such moves (in the second, non-blitz category) so existing maps are not affected.

I've revised the description of this change to mention the new logic. Can you read over that and let me know if you agree with that logic?

[asvitkine]

EDIT: Nevermind deleting this post content because I was accidentally in edit mode!

[asvitkine]

While testing the edge cases I found some things the current engine doesn't handle well. Please hold off on looking more for now as I'd like to fix them at the same time.

[Cernelius]

@Cernelius Thanks. I've switched to split the blitz and no-blitz cases into separate checks such that the "no blitz" case will now have an error message that doesn't mention blitzing ("Cannot attack out of a contested territory"). I've added a property to allow such moves (in the second, non-blitz category) so existing maps are not affected.

I've revised the description of this change to mention the new logic. Can you read over that and let me know if you agree with that logic?

I need some time to get around checking out the intended 1914 rules. Wouldn't be advisable also supporting the 1914 rules (with properties to be set true) as part of this PR?

Can we please have an exact and exhaustive description of how the program currently handles these matters when all properties are false? I think that is very important in order to build upon it.

[Cernelius]

I believe it is important to clarify how the program behaves in all such situations (by "Neutral" I mean the relationship archetype neutral basic so not the "Neutral" player (which is not neutral but at war with anything)), assuming we are moving 1 land unit over land of 1 space only:

Starting territory ownership:

1. Own/Allied
2. Neutral
3. Enemy

Starting zone units (beside infrastructures):

1. Non-contested (the unit we are trying to move does not share the zone with any enemy units) (if the unit we are trying to move is in an enemy owned territory with no enemy units (beside infrastructures), the zone counts as non-contested)
2. Contested

Ending zone ownership:

1. Own/Allied
2. Neutral
3. Enemy

Ending zone units (beside infrastructures):

1. Empty
2. The only units present are one or more own/allied units
3. The only units present are one or more enemy units
4. Both one or more own/allied units and one or more enemy units are present

Outcome
Cannot move.
Can move.

@asvitkine If it is easier for you to know the results than for me to do so by testing every single case in a game, may you please compile a list of program behaviours without the property set true? This is the list (other than the ones where the ending zone is Neutral, I filled the obvious ones, where the starting territory is friendly owned and has no enemy units in it, and where the ending zone is friendly owned and has no enemy units in it):

1111=Can move
1112=Can move
1113=Can move
1114=Can move
1121=Cannot move
1122=Cannot move
1123=Cannot move
1124=Cannot move
1131=Can move
1132=Can move
1133=Can move
1134=Can move
1211=Can move
1212=Can move
1213=
1214=
1221=Cannot move
1222=Cannot move
1223=Cannot move
1224=Cannot move
1231=
1232=
1233=
1234=
2111=Can move
2112=Can move
2113=
2114=
2121=Cannot move
2122=Cannot move
2123=Cannot move
2124=Cannot move
2131=
2132=
2133=
2134=
2211=Can move
2212=Can move
2213=
2214=
2221=Cannot move
2222=Cannot move
2223=Cannot move
2224=Cannot move
2231=
2232=
2233=
2234=
3111=Can move
3112=Can move
3113=
3114=
3121=Cannot move
3122=Cannot move
3123=Cannot move
3124=Cannot move
3131=
3132=
3133=
3134=
3211=Can move
3212=Can move
3213=
3214=
3221=Cannot move
3222=Cannot move
3223=Cannot move
3224=Cannot move
3231=
3232=
3233=
3234=

Then list all the cases for which the property changes the behaviour.

Thanks.

Once this is done, I think it would be important to add such documentation within PoS2.

[asvitkine]

@Cernelius

Thanks for thinking about this diligently. I'm afraid I don't have a way to verify the above easier than manually testing.

I also think there's unfortunately a lot of extra conditions that the above wouldn't tell the full story of, such as whether the unit has moved before, whether that move was an attack, the presence of air and land transports, how much movement units have, whether a territory was conquered this turn, etc. Having said that, I don't think enumerating all the possible combinations of these options is feasible - there's just too many factors. So maybe just doing it for your suggested list is OK and the rest can be covered with some more targeted testing.

[asvitkine]

After some back and forth with @Cernelius, I'm abandoning changing the blitzing logic changes this PR was originally proposing and keeping movement out of contested territories working exactly as before when the new property isn't set.

I've updated the tests and the PR description to reflect the changes. @DanVanAtta, can you review?

[DanVanAtta]

Overall looks good, no major callouts or issues that I can see. I left a few comments for you to review @asvitkine to consider places where we might be able to improve user messaging.

[DanVanAtta]

Is there any way to make this more specific regarding which units cannot blitz? Can we make this dynamic perhaps and list the unique unit types that cannot?

[asvitkine]

I'm not going to change anything with this error message since it's existing behavior (I just moved it to a variable), but actually the result object already contains the units that were invalid, since this message is added thus:

        result.addDisallowedUnit(NOT_ALL_UNITS_CAN_BLITZ_OUT_OF_EMPTY_ENEMY_TERRITORY, u);

With u being the unit that's not OK. So it's up to the code that's using the result (e.g. the UI code) to display that better, which would be a separate improvement.

[DanVanAtta]

Is there a typo in the name of this method? Reading it, it sounds like we have 'dependents' -> units' -> 'transports'

Should it be: 'dependentUnitsToTransportMap'?

[asvitkine]

It's actually "dependents" to "unitsToTransportMap", but I agree that it's hard to read.

How about I rename it to convertTransportKeyedMapToLoadedUnitKeyedMap()?

[DanVanAtta]

I wonder if we can make this error message more specific and helpful to the user. For example, could we say something like "not all units could be air transported, X out of Y transport capacity required"

Or, if we could say which units could not be transported, eg: "Not enough transport capacity for: X, Y, Z"

[asvitkine]

Let me try using result.addDisallowedUnit() here instead, like it's done for the blitz case.

[asvitkine]

Actually, addDisallowedUnit() is not an error, just a warning, so it's not the same semantics.

Thinking about this more, I think this is actually OK to leave as-is, because this situation won't happen in practice due to validation done in MovePanel - it doesn't let you load more units than allowed on an air transport. So this is just guarding against coding mistakes (e.g. if move panel changes or if AI submits invalid moves) or malicious players that are sending invalid moves with a custom client. As such, I'll just keep it as-is but add a comment to this extent.

[DanVanAtta]

Consider adding javadocs to '@test' to describe the test setup and what we are testing. Those comments describing the test I've personally found to be useful pretty soon after to go back and understand more easily what is being tested.

[asvitkine]

Done!

[Cernelius]

"Not all units can blitz out of empty enemy territory" is not an universally good definition for blitzable territories. For example, if in Revised you can blitz-out an empty enemy territory, in Classic you should also be able to blitz out an enemy territory with a factory and a aa gun in it as the only enemy units (capturing or liberating these units).

In V2 and all following rules-set, you can indeed blitz only "empty" territories, but in V1 (which should be the default for the program) you can also blitz territories in which the only enemy units in them are all infrastructures.

So, the messages should be more something like:
V1: "Not all units can blitz out of an enemy territory which is empty of enemy units beside infrastructures."
V2,V3,V4,V5,1940: "Not all units can blitz out of an enemy territory which is empty of enemy units."

By the way, I'm with Classic here: I never liked the "factories stop tanks" nonsense that is the rule since Revised.

Side note, here I've used the term "infrastructures" even though I believe it is a bad term to define the units (Nobody would consider an AA Gun of WW2 an infrastructure, let alone a Napoleonic general.). I've raised the matter here:
https://forums.triplea-game.org/topic/1644/a-better-word-than-infrastructure
My proposal to rename "infrastructure":
https://en.wikipedia.org/wiki/Appurtenance

[asvitkine]

"Not all units can blitz out of empty enemy territory" is not an universally good definition for blitzable territories. For example, if in Revised you can blitz-out an empty enemy territory, in Classic you should also be able to blitz out an enemy territory with a factory and a aa gun in it as the only enemy units (capturing or liberating these units).

In V2 and all following rules-set, you can indeed blitz only "empty" territories, but in V1 (which should be the default for the program) you can also blitz territories in which the only enemy units in them are all infrastructures.

So, the messages should be more something like: V1: "Not all units can blitz out of an enemy territory which is empty of enemy units beside infrastructures." V2,V3,V4,V5,1940: "Not all units can blitz out of an enemy territory which is empty of enemy units."

Since I'm not changing that logic here in the end, outside of the new property disabling that, that can be done in a separate change. Feel free to file an issue to track it.

By the way, I'm with Classic here: I never liked the "factories stop tanks" nonsense that is the rule since Revised.

Side note, here I've used the term "infrastructures" even though I believe it is a bad term to define the units (Nobody would consider an AA Gun of WW2 an infrastructure, let alone a Napoleonic general.). I've raised the matter here: https://forums.triplea-game.org/topic/1644/a-better-word-than-infrastructure My proposal to rename "infrastructure": https://en.wikipedia.org/wiki/Appurtenance