Sub combat rules wrong

[simon33-2]

One additional comment from #1638 @ssoloff , the first issue with the sub combat logic is the #1 remaining issue for users of world_war_ii_global at least. The fix is very likely to break serialisation. Be good if we could get enough of these changes in one go. Have a look in triplea/delegate/MustFightBattle.java

The important point here is that only subs without a countering destroyer can force the person receiving the fire to have to select casualties without knowing how many hits are caused in the remaining round.

In detail, when the defending side has a destroyer and the attacker doesn't, the order is:

• defender subs submerge prompt
• defender's subs submerge vs air only
• defending subs fire
• attacker chooses sub casualties
• sub casualties removed
• attacking subs fire
• defender chooses sub casualties
• Attacking air units fire
• defender chooses air casualties
• Remaining attacking units fire
• defender chooses remaining casualties
• remaining defenders fire
• attacker chooses casualties
• casualties removed

Order should be:

• defender's subs submerge vs air only
• defender subs submerge prompt
• defending subs fire
• attacker chooses sub casualties
• sub casualties removed
• attacking subs fire
• attacking air fires
• remaining attackers fire
• defender chooses sub casualties
• defender chooses air casualties
• defender chooses remaining casualties
• remaining defenders fire
• attacker chooses casualties
• remaining casualties removed

And vice versa. I'll spell out the correct order for all four cases to eliminate any confusion. If the attacker has a destroyer and the defender doesn't, then the correct order is:

• attacker's subs submerge vs air only
• attacker subs submerge prompt
• attacking subs fire
• defender selects sub casualties
• sub casualties removed
• remaining attackers fire
• defender selects casualties
• attacking air fires
• remaining attackers fire
• defender chooses air casualties
• defender chooses remaining casualties
• defending subs fire
• defending air fires
• remaining defenders fire
• attacker selects sub casualties
• attacker selects air casualties
• attacker selects remaining casualties
• casualties removed

If both sides have a destroyer, desired order is relatively simple:

• attacking subs fire
• remaining attackers fire
• defender selects sub casualties
• defender selects remaining casualties
• defending subs fire
• remaining defenders fire
• attacker selects sub casualties
• attacker selects remaining casualties
• casualties removed

If neither side has a destroyer, desired order is:

• attacker's and defender's subs submerge vs air only
• attacker's subs submerge prompt
• defender's subs submerge prompt
• attacking subs fire
• defender selects sub casualties
• defending subs fire
• attacker selects sub casualties
• sub casualties removed
• attacking air fires
• remaining attackers fire
• defender chooses air casualties
• defender chooses remaining casualties
• defending air fires
• remaining defenders fire
• attacker chooses air casualties
• attacker chooses remaining casualties

It's slightly different in a few maps in that subs can be hit by air without a destroyer, so you might need to be mindful of that but you should be able to see it from the existing code.

In case you want to know why there should ever be consecutive boxes for choosing casualties, that is because not all units can hit other units. Subs can never hit air. Air can't hit subs without a destroyer, and allied (different power, same alliance) destroyers only count when defending.

[ssoloff]

@simon33-2 While I have some free cycles, I'd like to at least start putting together acceptance tests for the scenarios you described above. I'm not quite sure where to begin, so I thought I'd look for some existing tests that verify battle step order. I found some tests in WW2V3_41_Test (e.g. testAttackDestroyerAndSubsAgainstSub(), testAttackSubsOnDestroyer(), etc.) that look promising.

Is any one of these tests a good place to start writing the acceptance tests for this issue?

[simon33-2]

I would guess so. If you commit & push, I can download it and have a look so long as you tell me the branch name.

You should also be aware that there are maps like classic which don't have destroyers and handle air vs subs differently. Normally, the sub can retreat after the first shot even though it's defending. AFAIA, these rules work fine at present. However, 1940 maps, v5, probably v3 & v4 all don't work. Not sure about revised or Triple-A only games like Big World. Hopefully we won't need a property.

[ssoloff]

@simon33-2 I have a few more questions. To make things simpler, I'm going to focus on your first use case (attacker without destroyer; defender with destroyer).

I'm trying to match these steps up to those documented in BattleStepStrings. Here's what I have so far:

• defending subs fire (defender SUBS_FIRE)
• attacker chooses sub casualties (attacker SELECT_SUB_CASUALTIES)
• sub casualties removed (REMOVE_SNEAK_ATTACK_CASUALTIES)
• attacking subs fire (attacker SUBS_FIRE)
• attacking air fires (AIR_ATTACK_NON_SUBS)
• remaining attackers fire (attacker FIRE)
• defender chooses sub casualties (defender SELECT_SUB_CASUALTIES)
• defender chooses air casualties (???)
• defender chooses remaining casualties (defender SELECT_CASUALTIES)
• remaining defenders fire (defender FIRE)
• attacker chooses casualties (attacker SELECT_CASUALTIES)
• remaining casualties removed (REMOVE_CASUALTIES)

Is there an explicit step corresponding to "defender chooses air casualties"? Or is this really just merged into "defender chooses remaining casualties" now that you've made those two steps consecutive? (That is, in your "current behavior" list of steps, "defender chooses air casualties" would indeed be a "defender SELECT_CASUALTIES" step because "remaining attacking units fire" separates "defender chooses air casualties" and "defender chooses remaining casualties".)

Also, do the sub submerge steps (SUBS_SUBMERGE and SUBMERGE_SUBS_VS_AIR_ONLY) need to be taken into account in all the use cases?

[simon33-2]

There is no step in the step strings for defender chooses air casualties. That is another bug in the current system. Not exactly an important one though!

If the defender has a destroyer but the attacker doesn't, only the defender gets the "SUBS_SUBMERGE" option.

SUBMERGE_SUBS_VS_AIR_ONLY is in the wrong place - I wanted to fix that in #1599 IIRC but it does break serialisation, I think. It is dumb to ask if you want to submerge the subs when the next step is to check if it is mandatory. Should be the other way around obviously. So in this use case, I would presume that SUBMERGE_SUB_VS_AIR_ONLY would be dropped wrt the attacker's subs because we already know the defender has a destroyer. SUBMERGE_SUB_VS_AIR_ONLY for the defender should really be the first action, followed by defender submerges subs, then defender subs fire.

I'll update the OP to reflect that.

[simon33-2]

Sorry about that oversight there.

[simon33-2]

@DanVanAtta @ron-murhammer

The fix for this is nearly certain to break backwards compatibility. How should this be handled looking forwards? Just tell people to use their existing installation for it?

[ssoloff]

Something else that I'm not 100% sure about and would appreciate clarification...

It looks like MustFightBattle#determineStepStrings() and MustFightBattle#getBattleExecutables() are independent of each other, but the lists returned by each method should be somewhat correlated. Is that true?

I'm trying to figure out what I should actually test. I started off testing the return value of determineStepStrings() until I noticed that there are many more IExecutables returned by getBattleExecutables() that are not accounted for by determineStepStrings() (e.g. "defender chooses air casualties," as we discussed above). Most existing tests only check determineStepStrings(), but I found a couple that look at the IExecutable stack. In fact, the AttackSubs and DefendSubs nested classes were added specifically for this purpose.

[ssoloff]

Consider the following partial set of steps from the current and proposed battle orders for the case where the defender has a destroyer but the attacker does not have a destroyer (bold entries are differences):

Current	Proposed
attacking subs fire	attacking subs fire
defender chooses sub casualties	attacking air units fires
attacking air units fire	remaining attacking units fire
defender chooses air casualties	defender chooses sub casualties
remaining attacking units fire	defender chooses air casualties
defender chooses remaining casualties	defender chooses remaining casualties
remaining defenders fire	remaining defenders fire

Is the proposed battle order specific to the WW2 global map or is this the way it should be for all maps?

Regardless of whether it is specific to a map, it looks like it's going to require some major changes. The MustFightBattle#fire() method appears to be what's used for all the "attack" steps. That method simply delegates to the Fire class. That class is where the coupling between "attack" and "choose casualties" occurs. Breaking that apart is going to require some significant redesign.

I suppose it wouldn't be possible to somehow create a single Fire instance for the three steps "attacking subs fire", "attacking air units fire", and "remaining attacking units fire" to force the casualties to be selected at once? That probably wouldn't work because the available choices for casualties in each step are different, right?

So if this is going to require coming up with a more flexible alternative to the use of Fire, I think the scope of this change just increased by an order of magnitude. ☹️

[simon33-2]

Yeah, not the easiest change to make. Your correct on step strings and battle steps ought to be correlated. I don't know why it was written this way but it was.

To my knowledge, it should work that way on every map which includes a destroyer as a unit, which means revised onwards. Classic works differently to allow for the lack of a destroyer - subs can retreat after the first round if they have a SZ to go to but air units CAN hit subs.

[panther2]

Also v2 has been the last edition where Subs can always be hit by air units.
So in the given case in v2 the attacker (lacking a destroyer) can hit defending subs with air units, while from v3 on he cannot.

[simon33-2]

Thanks for that correction @panther2 .

Checking the rules, I'll note that in that edition subs still get a surprise strike only when a destroyer is not present. So in Revised (v2), as you might expect that the air attack/defend steps are never used.

So as far as you are aware, subs can never hit air in any edition, right?

[panther2]

So as far as you are aware, subs can never hit air in any edition, right?

That's correct, @simon33-2 .

[DanVanAtta]

One thing to clarify in the order list:

attacking subs fire
attacking air fires
remaining attackers fire
defender chooses sub casualties
defender chooses air casualties
defender chooses remaining casualties

With the above, the defender knows how many hits they will have to allocate total before they allocate any at all? That may prove to be some effort with the current code setup, I think the flow expects hits/casualty selection to happen in sequence. A first iteration change perhaps could just allow for that, so that selection could happen in proper sequence after a series of attack rolls. It may be some effort to get the UI to be intuitive, we would probably want to keep track and display for the user how many of each type of hit there is to allocate.

For the actual phase changes, my recommendation is we first re-work the battle sequencing code to be more functional. If the code resembled something more like a decision engine, where a single component would take state data and tell us which next battle phase to enter, it could make testing much more straight forward and code more modular perhaps.

[simon33-2]

That is indeed the problem, @DanVanAtta ! Basically, the Fire needs to be split into two methods - one rolls and one allocates casualties.

Did you ever look at the notes for the 1940 games or the list of bugs? Or the rulebook?

[simon33-2]

There's another problem in the same area of code. If you do a mixed amphibious/overland assault including planes, if/when all the land units are destroyed, the MustFightBattle code looks for the step string for "retreat planes" but what was added when the step strings were created was "retreat non amphibious units". This causes the console to flash up which is not correct.

[DanVanAtta]

@simon33-2 or @panther2 , could you re-summarize the remaining problems in this issue as individual and new tasks? There is a lot to this thread, it's very hard to grok what has been done, needs to be done, and what exactly is the problem and what the needed fix would be.

[panther2]

@DanVanAtta
I have never dealt in depth with this now three years old issue, apart from answering specific questions when having been asked.

In case @simon33-2 does not intend to continue his work he and @ssoloff already put a lot of effort in, I am of course willing to somehow look into it.

But I would need to start from the beginning, that means comparing today's TripleA conduct combat logic with those of the rulebooks with all their specific cases.
What I want to say is that in this case this task would be huge and not to be resolved in the short term.

[DanVanAtta]

Okay @panther2 , thank you for the response. It seems then that either way we need to research and define what needs to be done (which would lead to new tickets). In either case of whether that is done or not, this tracking ticket no longer seems useful nor actionable.