Incorrect handling of Facility bombing

panther2 commented 7 years ago

My Operating System:

Windows 7.64

TripleA version:

1.9.0.0.6454 (and earlier)

Map:

WWII_Global_1940

Can you describe how to trigger the error? (eg: what sequence of actions will recreate it?)

TripleA does not handle bombing facilitites correctly, in case facilities are already at their maximum damage.

In the starting scenario, 6/6 damage is already assigned to as well the air base as the naval base. The factory has no damage. See savegame SBR_StartPosition.tsvg SBR_StartPosition.zip

When in combat move all planes from Holland Belgium are brought to UK at the same time with the intention to bomb the facilitites, the engine incorrectly sends them to a territory fight. TripleA does not ask for bombing, does not initiate a fight against interceptors, does not trigger facilities' AAA-fire. Instead it executes the territory fight. See savegame SBR_all_chosen.tsvg (history mode) SBR_all_chosen.zip

What should happen is

the German player should be asked to bomb the facilities or fight in the territory
in case the German player decides for bombing, the British player should be asked to intercept
after the fight against the interceptors the Germany player should be asked to assign the bombers to whatever facility and the tactical bombers to the bases, regardless to whatever extent those are already damaged
facilities AA-fire should be fired (damage does not affect the self-defense antiaircraft ability of the facility)
bombing should be executed
damage should be assigned where possible

There is nothing in the rules that prevents bombers from bombing damaged (to whatever extent) facilities ... but damage exceeding the limits is not applied.

Rule discussion starting from here: http://www.axisandallies.org/forums/index.php?topic=28562.msg1690237#msg1690237

When - from the same starting position - first the bombers are combat moved to UK, the prompt whether to bomb does appear. But when the tactical bombers are combat moved there, the prompt to bomb is missing - and they are automatically subject to territory fight, what is incorrect. Instead they should be able to bomb the bases (regardless of the amount of their damage). See savegame SBR-chosen_separately.tsvg (history mode) SBR_chosen_separately.zip

prastle commented 7 years ago

Excellent description @panther2 I can confirm this as it just occurred testing trains in @hepps new tww yesterday. we were using .6454 as well. It does function fine in the original tww using the current stable.

ssoloff commented 7 years ago

@panther2 The errors you attached are unrelated to the primary bug you reported above and are possibly a regression caused by #2313. I'll investigate and open a separate issue, if necessary.

ssoloff commented 7 years ago

@panther2 Confirmed. Those error logs are a regression caused by #2313. I opened #2323 and should have a fix prepared by the end of the day. Thanks for always being complete in your error reporting, even when things seem unrelated. :smile:

prastle commented 7 years ago

i would like to reinforce the pt that escorts no longer seem to be supported as well. he did mention that above not sure if that is same problem. On a bombing run escorts sent are not included and act as combat units.

panther2 commented 7 years ago

Indeed, @prastle. As said, the complete SBR-sequence is suppressed, when the bug occurs. Send fighters (as escorts) with the bombers only and it will work as expected. But if you send them together with bombers and tac bombers, the bug will occur. You can't send them as escorts together with the tac bombers (for bombing), either.

ssoloff commented 7 years ago

I did some debugging and think I found some useful information, although I'm not sure where the fault lies for causing this bug.

When sending the bombers and tactical bombers from Holland Belgium to attack the UK, no offer is made to choose SBR because the UnitAttachment#getAllowedBombingTargetsIntersection() method is returning the following collection of allowed unit types to bomb:

[UnitType{name=harbour}, UnitType{name=airfield}]

As both the harbor and airfield in this territory are at maximum damage, no option is given for SBR. (@panther2, you mentioned above that there is nothing in the rules that prevents bombing units at maximum damage, but the TripleA code seems to filter such units out from the available target list. I'm not sure when this "feature" was introduced.)

The obvious omission in the above collection is the factory_major unit type. I think this is because of the following configuration in ww2global40_2nd_edition.xml:

The tactical_bomber unit attachment has isStrategicBomber set to true.
The tactical_bomber unit attachment has bombingTargets set to airfield:harbour.

Because of (1), both the bombers and the tactical bombers are included in the bombersOrRockets parameter of getAllowedBombingTargetsIntersection(). Because the tactical bombers are part of this collection, the value of their bombingTargets property is considered when calculating the allowed bombing targets intersection. Thus, because of (2), the intersection of all possible bombing targets shared between bombers and tactical bombers ends up only including airfields and harbors.

The code that calculates the possible SBR targets is in MovePerformer.java:

final Collection<Unit> enemyTargets =
    Match.getMatches(enemyTargetsTotal,
        Matches.unitIsOfTypes(UnitAttachment
            .getAllowedBombingTargetsIntersection(Match.getMatches(arrived, Matches.unitIsStrategicBomber()),
                data)));

It looks like all this code was introduced in dc0cf72 and has pretty much remained unchanged since then (2012). So I'm not sure how a group of bombers and tactical bombers would be permitted to SBR anything other than an airfield or harbor unless

The bombingTargets setting for tactical bombers included the different factory types sometime in the past, or
The isStrategicBomber setting for tactical bombers was false, or
The code somehow previously excluded tactical bombers from the call to getAllowedBombingTargetsIntersection() based on some other heuristic (e.g. it decided to treat the tactical bombers as escorts rather than as participants in the SBR).

@prastle You mentioned that the bombers + tactical bombers SBR scenario works in the "original TWW." Can you point me to a copy of that map? I'm curious if it predates the 2012 "bombingTargets" feature or if it has a different value than what's in the current version.

panther2 commented 7 years ago

Interesting, @ssoloff , thanks.

Just let me add that the engine would not allow bombers to further SBR max. damaged factories, either, not only in Global but for example in WWII_v5, too. What would not bother me because it would be absolutely pointless to send bombers to AA-fire for nothing or to not use them in any territory fight, instead.

But it supports your above statement

... there is nothing in the rules that prevents bombing units at maximum damage, but the TripleA code seems to filter such units out from the available target list. I'm not sure when this "feature" was introduced.

prastle commented 7 years ago

@ssoloff actually I wasn't looking at the bombing. I was observing the escorts not escorting in the new and working fine in the old. The original Total World War in the download maps list works for escorts. The newer one with trains did not.

ssoloff commented 7 years ago

@prastle Thanks for the clarification, and sorry for the confusion. Would it be correct to say then that the escort issue is separate from the facility bombing issue @panther2 originally reported? If so, would you mind opening a separate issue for that so it doesn't get lost in the details of this ticket?

@panther2 I tried your save game in 3635 and experienced the same result as in the latest pre-release. Given all the information we've discovered so far, this bug doesn't sound like it's actually a regression but has been present for some time. Do you have any evidence that would suggest your scenario ever behaved as expected? (Just trying to nail down if this is a regression or not; if we know it's not, we won't waste time hunting through code history looking to find when it was broken. :smile:)

prastle commented 7 years ago

@ssoloff actually i think it might be the same issue and engine related. Since while playing today with hepps using .6481 it still occurs but i will test the bomber issue separately. What I can clarify is that if using tww the fighter's escort on a bombing run with .3635 and current tww in download list.

prastle commented 7 years ago

and btw what is this amazing new list of functions im looking at LOL in github (off topic) :)

panther2 commented 7 years ago

@ssoloff I would consider it being a bug rather than a regression. I suppose it always had been there and became only evident because of that special scenario that has been discussed. If the TripleA code seems to filter such units out from the available target list it needed a special scenario to discover this issue (as bombing max. damaged factories only is pointless and thus would not occur).

prastle commented 7 years ago

after discussing with hepps it is def a separate issue. I will open a new issue.

Heppisorus commented 7 years ago

Yes the escort function is definitely not working in the latest pre-release even though using the same XML in .3685 yields perfect functioning of the escort properties.

ron-murhammer commented 7 years ago

@panther2 @ssoloff So does this essentially boil down to SBR units should be able to target units that are at max damage but currently aren't allowed to? And the logical reason this needs to be allowed is for maps that have air battles or want to take casualties for other bombers?

Heppisorus commented 7 years ago

I haven't tested the specifics surrounding sending bomber in against (what I would consider ineligible targets) facilities that are already at maximum damage. This because in TWW there is no such thing... as all facilities are destroyed when they reach maximum damage.

My comment was based on an observation that escorts where not being prompted to either escort or attack while accompanying legitimate bombing runs... instead being forced into a direct assault on the terr. in question.

On 09/06/17, Ron notifications@github.com wrote:

@panther2(https://github.com/panther2) @ssoloff(https://github.com/ssoloff) So does this essentially boil down to SBR units should be able to target units that are at max damage but currently aren't allowed to? And the logical reason this needs to be allowed is for maps that have air battles or want to take casualties for other bombers?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub(https://github.com/triplea-game/triplea/issues/2322#issuecomment-327667155), or mute the thread(https://github.com/notifications/unsubscribe-auth/AdgpoFZjuSjtCP_azB5aeS-Ht5DNySp9ks5sf1fHgaJpZM4PL6NB).

ron-murhammer commented 7 years ago

@Heppisorus That appears to be a separate problem as what @panther2 describes is the same behavior in the current stable release. Can you open a different issue describing the issue you are seeing around escorts (save game showing the behavior would be helpful as well).

Heppisorus commented 7 years ago

Aye Capt.

ssoloff commented 7 years ago

@ron-murhammer The problem is that attacking a territory with both bombers and tactical bombers in a single move gives the user different options than when the bombers and tactical bombers are moved separately. In the specific scenario described by @panther2 above, there are two independent engine rules responsible for this difference in behavior:

SBR target list only includes facility unit types that can be SBRed by all air units in the sortie.
SBR target list only includes facilities that are not at maximum damage.

When both bombers and tactical bombers are moved in a single sortie, (1) produces an SBR target list that only contains airfields and harbors. (2) further filters the airfield and harbor from the list because both are at max damage. The result being an empty SBR target list, and thus no option to SBR is offered to the user, and all units in the sortie are forced to air attack.

When the bombers and tactical bombers are moved in two sorties, the above rules are applied to each sortie individually:

For the bomber sortie, (1) produces an SBR target list that contains all facility types. (2) filters the airfield and harbor from the list because both are at max damage. The result being an SBR target list that contains the undamaged factory, and the user is offered the option to SBR.
For the tactical bomber sortie, (1) produces an SBR target list that only contains airfields and harbors. (2) further filters the airfield and harbor from the list because both are at max damage. The result being an empty SBR target list, and thus no option to SBR is offered to the user, and the tactical bombers are forced to air attack.

IMO, rule (1) is the primary cause of the difference in behavior. Rule (2) may be incorrect according to the tabletop rules, but I don't think changing it would fully solve the difference in behavior between moving using one or two sorties.

For example, let's say we modify rule (2) to not filter out fully-damaged units, so that the airfield and harbor are in the target list. In this case, the user would be prompted to SBR in the single sortie case, because the SBR target list after applying (1) and (2) would be non-empty. However, it would still only contain the airfield and harbor. Thus, (and I'm guessing here because I haven't verified) SBR damage rolls will only be applied to the fully-damaged airfield and harbor; no damage will be applied to the facility. If that's what the desired behavior is, then great, it's simply a matter of removing rule (2). But I would think the user would expect bomber damage to be applied to all targets (specifically the factory), while tactical bomber damage would be applied to the airfield and harbor (even though it has no effect).

Otherwise, if the goal is for the one- and two-sortie moves to produce identical behavior (i.e. bombers have the option to SBR the factory and tactical bombers always air attack), this will probably be non-trivial. I don't see how simply changing the behavior of rule (1) can achieve this. It seems the engine would somehow have to partition the units in the sortie to give the attacker the most bang for their buck.

ron-murhammer commented 7 years ago

@ssoloff So the reason I believe for (1) is that you'd end up with a complex SBR target selection window. Imagine that instead of ALL it did ANY so you get a full list of any that could be bombed, then it would have to limit which target various SBR units could target (couldn't assign tac to factories for example). It instead makes you move them in chunks based on target so you move strats and choose targets then move tacts and choose targets unless the targets are the same then you can move all at once. You could argue this isn't great and a better UI to allow moving all at once then assigning to targets would potentially be better but that UI would be fairly complex if you have multiple targets and multiple types of bombers.

My thought is to leave (1) alone for now and fix (2) as that is the real bug and would allow you to do the appropriate bombings as long as you make multiple moves.

ssoloff commented 7 years ago

@ron-murhammer I agree that trying to modify (1) to accommodate every possible SBR scenario would just be a mess. You're right that putting the onus on the user to split units across multiple moves to achieve the desired attack behavior is the best compromise.

panther2 commented 7 years ago

So does this essentially boil down to SBR units should be able to target units that are at max damage but currently aren't allowed to? And the logical reason this needs to be allowed is for maps that have air battles or want to take casualties for other bombers?

This is my understanding, yes.

Heppisorus commented 7 years ago

Not that I am fluent in what goes on behind the magic curtain... but is the mechanism that governs bombarding of any use in this situation? Seems like when you have lots of bombard options in the same SZ the engine does a very efficient job of separating out the different options available to the player. Just a random suggestion.

panther2 commented 7 years ago

@ron-murhammer @ssoloff I am sorry, this is still not resolved correctly, will open a new issue....

triplea-game / triplea