AI Strategic Bombs (Issue 6718)

[frigoref]

Change Summary & Additional Notes

Fixing issue 6718 AI Strategic Bombs Already Max Damage Factories

[codecov]

Codecov Report

Merging #10367 (be6e069) into master (9f090f1) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master   #10367      +/-   ##
============================================
- Coverage     27.28%   27.27%   -0.02%     
+ Complexity     8265     8264       -1     
============================================
  Files          1217     1217              
  Lines         77786    77820      +34     
  Branches      10600    10598       -2     
============================================
- Hits          21227    21223       -4     
- Misses        54601    54638      +37     
- Partials       1958     1959       +1     

Impacted Files	Coverage Δ
...games/strategy/triplea/ai/pro/ProCombatMoveAi.java	0.38% <0.00%> (-0.01%)	⬇️
.../src/main/java/games/strategy/net/nio/Decoder.java	74.13% <0.00%> (-5.18%)	⬇️
...lea/ai/flowfield/influence/InfluenceTerritory.java	96.96% <0.00%> (-3.04%)	⬇️
...a/games/strategy/triplea/ai/pro/ProPurchaseAi.java	0.32% <0.00%> (-0.01%)	⬇️
...s/strategy/triplea/ai/pro/util/ProBattleUtils.java	0.00% <0.00%> (ø)

---

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 9f090f1...be6e069. Read the comment docs.

[asvitkine]

So I have some comments on the current state of the code, but at a high level, I feel like the bomber map is adding a lot of complexity. Can't we just keep the code similar to how it was before the latest updates, but not use a map?

For example, adapting from your previous code:

             int existingDamage = 0;
             final List<Unit> potentialBombingTargetUnits =
               t.getUnitCollection().getMatches(bombingTargetMatch);
             for (final Unit targetUnit : potentialBombingTargetUnits) {
               existingDamage += targetUnit.getUnitDamage();
               if (Matches.unitIsAaForBombingThisUnitOnly().test(targetUnit)) {
                 noAaBombingDefense = 0;
               }
             }
....

             final int remainingDamagePotential = maxDamage - 3 * numExistingBombers - existingDamage;

Shouldn't that be sufficient? If not, what am I missing?

[frigoref]

@asvitkine Let me first say, that I am very glad that you are assisting me so patiently so get this one solved!

So I have some comments on the current state of the code, but at a high level, I feel like the bomber map is adding a lot of complexity. Can't we just keep the code similar to how it was before the latest updates, but not use a map?

Shouldn't that be sufficient? If not, what am I missing?

The problem described in issue 6718 leads to the need of comparison between damage potential and minimal damage needed. Adding to this the example you've made with A bombs B and X bombs Z (B and Z are different factory types) a comparison between each attacking unit and the potential target units is needed. I do not see a way around this. Am I missing anything?

[asvitkine]

@asvitkine Let me first say, that I am very glad that you are assisting me so patiently so get this one solved!

So I have some comments on the current state of the code, but at a high level, I feel like the bomber map is adding a lot of complexity. Can't we just keep the code similar to how it was before the latest updates, but not use a map?
Shouldn't that be sufficient? If not, what am I missing?

The problem described in issue 6718 leads to the need of comparison between damage potential and minimal damage needed. Adding to this the example you've made with A bombs B and X bombs Z (B and Z are different factory types) a comparison between each attacking unit and the potential target units is needed. I do not see a way around this. Am I missing anything?

My suggested code takes that into account since it's within the outer loop that's iterating over bombers and it looks at units filtered by bombingTargetMatch which should only return units that the bomber can bomb.

[frigoref]

My suggested code takes that into account since it's within the outer loop that's iterating over bombers and it looks at units filtered by bombingTargetMatch which should only return units that the bomber can bomb.

The filter by bombingTargetMatch is (attack) unit specific as according to panther's reply in issue 6718

different bombers can attack different targets

My understanding is that this leads to the requirement of the "bomber map" or am I misunderstanding your point completely here?

[asvitkine]

The filter by bombingTargetMatch is (attack) unit specific as according to panther's reply in issue 6718

different bombers can attack different targets

My understanding is that this leads to the requirement of the "bomber map" or am I misunderstanding your point completely here?

Really sorry for taking a long time to reply. Let me explain my thoughts as to why I think my suggested implementation would be sufficient without needing to keep a map:

So from the issue we're trying to fix #6718:

AI enhancement to not strategic bomb factories that cannot be damaged any further.

Basically, we want to not send additional bombers to factories that are already at max damage. That means both (a) not to send any bombers if factory is fully damaged and (b) to not send too many, to account for the damage maximum.

Furthermore, we have the requirement that different factories types may be bombable by different unit types (c).

So how does my suggested code in #10367 (review) meet these three requirements?

Recall that we're iterating through bomber units and we're deciding where to send them. For each bomber, we calculate the territory with the highest maxBombingScore, and then decide to send the bomber there.

How do we handle the fact that different factories can be bombed by different units (c)? Well, we're iterating through each individual bomber unit, and for each one, we generate a potentialBombingTargetUnits collection - of units that can actually be bombed by it. I think my code sample was missing this, but we can check if this collection is empty, then no units can be bombed by it - and we can skip that territory.

How do we make sure we don't send bombers to fully bombed factories? Well, when computing remainingDamagePotential, we subtract the amount of damage already inflicted (existingDamage). So if the damage already matches the max damage, the score will be 0 and that territory won't qualify for maxBombingScore. So this handles (a).

How do we handle (b) (not to send too many bombers)? This is similar to (a), but the additional factor is how many bombers we're planning to send to that territory, which the code already accounts for, via the numExistingBombers variable, which comes from the attackMap which is updated every time we choose to send a bomber to a territory.

Therefore, based on the above, I don't think we need the extra map and a solution similar to the one I proposed should be able to work. Do you disagree? Am I missing something in my explanation?

As an aside, there are a few things I'm not certain about with the existing code, which is worth checking:

1. First, it gets maxDamage via ta.getProduction(). Is that always correct? I thought when multiple specific units can be bombed, it may not always be based on the territory value - but I'm not an expert on the different ruleset options here. We should check how this is actually calculated during bombing battles to understand if this is correct - and ideally use the same helper function from both places. Also, if there are multiple bombable infrastructure units in that territory, is the maxDamage for each unit? If so, that's not being taken into account right now.
2. It assumes that each bomber will do 3 points of damage (via the 3 * numExistingBombers calculation). My guess is this not correct in the general case - since it depends on the map. Obviously, there is also a dice roll involved, but even ignoring that (and assuming 3 is supposed to be the average value or something), I think different maps can control the number of dice and the effect of those for the damage? Ideally, again, we could use the same helper functions that actual bombing battle code uses.

These two things above are not directly related to this change, but since we're looking at this code, would be good to fix these.

One other thing, the issue also says:

An algorithm could be added to deny bombing when expected damage is less than the expected cost of losing the plane.

I think this part is a separate orthogonal feature request that can be handled in a different PR (i.e. taking into account the expected cost of losing the plane), so I'm ignoring it for this discussion (I think your approach also wasn't handling that). That can be investigated separately.

[frigoref]

Let me try to structure this a bit
1. Requirement list

(a) not to send any bombers if factory is fully damaged
(b) to not send too many, to account for the damage maximum
(c) different factories types may be bombable by different unit types

2. Your questions/points and my answers

Therefore, based on the above, I don't think we need the extra map and a solution similar to the one I proposed should be able to work. Do you disagree? Am I missing something in my explanation?

Yes, I do disagree and I think you are missing the following:
(c) cannot be fulfilled when the logic is just using the number of existing bombers with final int numExistingBombers = attackMap.get(t).getBombers().size(); without taking into account which unit they can actually bomb or not. (this goes in addition to your point 2 (assumption of 3 damage for each bomber)

1. First, it gets maxDamage via ta.getProduction(). Is that always correct?

According to the A&A rules for Strategic Bombing the rule is
"This is the ability for a bomber to attack a factory. If the bomber survives AA Gun fire, it will proceed to attack the factory for either cash or unit production."
I am not certain that "cash or unit production" is correctly determined, but otherwise, I would think that the maximum damage possible during SBR is the production of the territory attachment. Therefore, maxDamage = ta.getProduction(); is correct.

An algorithm could be added to deny bombing when expected damage is less than the expected cost of losing the plane.

I think this part is a separate orthogonal feature request that can be handled in a different PR

I agree, this should be handled separately.

3. My questions
Concerning the case of different types of factories I would assume that each bomber can only bomb one factory type in a territory. So either there is a relation bomber types to factory types they can bomb being n:1, or this relation is ensured by how the factories are placed in the territories.
3.a Do you agree to this assumption?

Considering (c) I would think that only the view of all bombers combined can result in a good choice for the attacker where to send the bombers.
3.b Do you agree to this view?
Example: In the case where 2 bombers A and B can either
(Opt1) be send to territory 1 or
(Opt2) only A can be send to territory 2
the individual view of each bomber might result for A to be send to 2 and then for B the needed damage in 1 is not reached alone.

[asvitkine]

Thanks, got a chance to do another review pass. A few more comments, but I think it's on the right track.

[asvitkine]

Previous logic was actually to exclude territories for which this was true (canCreateAirBattle).

    if (canBeBombedByThisUnit
              && !canCreateAirBattle
              && canAirSafelyLandAfterAttack(unit, t)) {

What's the reason for having this logic here (before calling determineBestBombingAttackForBomber()) and the rest of the conditions inside that function. Why not have everything there? Is it just to avoid looking up raidsMayBePrecededByAirBattles multiple times? If so, you can still pass that as a param.

I think it's cleaner if we check all the conditions in one place. It could also be here, in this function, but splitting them makes it harder to follow the logic.

[frigoref]

I agree that there is something wrong in the rework.

However, I also think, that there is something odd about the variable canCreateAirBattle: It seems like the original code wanted to avoid potential air battles (hence !canCreateAirBattle), in case

1. the property RAIDS_MAY_BE_PRECEEDED_BY_AIR_BATTLES is set AND
2. the territory has potential air units that can cause an air battle
   As 1. is a generic property independent of the territory I wanted to extract this. If my understanding is correct, the call of method determineBestBombingAttackForBomber is not needed in case the 1. is true. This can be checked already at the beginning of method determineTerritoriesThatCanBeBombed.

My problem here is that it seems to assume that raids in general are not possible if the property RAIDS_MAY_BE_PRECEEDED_BY_AIR_BATTLES is not set. Is that true?

The other part of your post refers to the split of logic and the justification here is that the check for territory properties and checks for its contained units (=potential target units). I still think this split is a clean one.

[asvitkine]

I don't think we can check RAIDS_MAY_BE_PRECEEDED_BY_AIR_BATTLES at the beginning of the method, because we checked !(1 && 2), which is the same as (!1 || !2), so in particular, if 2 is false (aka there are no potential air units for the battle), we would execute the code, regardless of what 1 is.

[frigoref]

So we want to avoid air battles: If the property allows preceding air battels, only territories without the potential to have one should be kept, else all territories are relevant.
I made a change and added a comment for this.
Note that I use a not now: !AirBattle.territoryCouldPossiblyHaveAirBattleDefenders

[asvitkine]

Don't we want to only add if all targets correspond? Right now, you add if any correspond.

You can change to something like:

for (Unit existingBomber : existingAttackingBombers) {
  if (targetUnits.stream().allMatch(u -> Matches.unitIsLegalBombingTargetBy(existingBomber).test(u))) {
    sameTargetBombers.add(existingBomber);
  }
}

And have a separate loop to sum up neededDamageUnits.

[frigoref]

In case a bomber cannot raid against all units, but against one (do not know if this is actually possible) this code should consider the bomber as co-bomber if any common target unit exists. Why should the co-bombers be limited to those being able to bomb all target units?

Considering neededDamageUnits the idea is that all target units can consume damage before the production is harmed.

[asvitkine]

@asvitkine Fixes made. If you have further points, maybe we should set up a call instead of an extended back-and-forth here.

I think this the tricky code around bombing requires offline studying to have meaningful discussion, so while I appreciate the offer, I don't think I could provide good feedback in a live discussion on this.

[frigoref]

Okay, then we'll leave it?

[asvitkine]

In case a bomber cannot raid against all units, but against one (do not know if this is actually possible)

I believe this is possible, since each unit type can specify getBombingTargets() separately.

this code should consider the bomber as co-bomber if any common target unit exists. Why should the co-bombers be limited to those being able to bomb all target units?

I think it gets tricky because you don't know which units something is targeting. For example, you have A that can target X and Y and B that can target Z and X. Let's say X,Y,Z are all in the same territory.

When considering B after A is already scheduled, you don't know if A's damage should reduce B's potential or not. So if you just consider units that both can target, you miss out on the fact that A could bomb just Y and B still has full potential between X and Z.

However, I think the case where we consider "all targets possible" is also not right, so I think neither of these simple approaches handle the above example well. As such, I'm OK with your current code on this - it's not perfect, but for most maps it will work well enough.

In the future, the logic could be expanded to handle complicated cases well, if some map runs into it, but that doesn't seem like a big priority currently, so leaving as-is seems fine.

Maybe just add a comment about the limitation?

[asvitkine]

I don't understand the maxDamageProduction - neededDamageUnits expression.

That sounds to me that the territory production value gets reduced by how much damage units can still take.
That's not right, is it?

I would expect it should either be the sum of the two (e.g. max bombing damage = production + damage that individual factories can take), or choosing one or the other, depending on game rules, or the max or min of the two.
Subtracting just seems not right to me.

Can we verify with the engine code where damage is actually determined from bombing?

[frigoref]

It should be maxDamageProduction + neededDamageUnits and I corrected it.

Concerning the damage logic from bombing you can check StrategicBombingRaidBattle especially variable bombingRaidDamage and call of method getTarget.

[asvitkine]

Thanks. The relevant code seems to be:

      int damageLimit = TerritoryAttachment.getProduction(battleSite);
      final boolean limitDamage =
          Properties.getWW2V2(gameData.getProperties())
              || Properties.getLimitRocketAndSbrDamageToProduction(gameData.getProperties());

        if (limitDamage) {
          costThisUnit = Math.min(costThisUnit, damageLimit);
        }

and:

      // Limit PUs lost if we would like to cap PUs lost at territory value
      if (Properties.getPuCap(gameData.getProperties())
          || Properties.getLimitSbrDamagePerTurn(gameData.getProperties())) {
        final int alreadyLost = gameData.getMoveDelegate().pusAlreadyLost(battleSite);
        final int limit = Math.max(0, damageLimit - alreadyLost);
        cost = Math.min(cost, limit);
        if (!targets.isEmpty()) {
          for (final Unit u : bombingRaidDamage.keySet()) {
            if (bombingRaidDamage.getInt(u) > limit) {
              bombingRaidDamage.put(u, limit);
            }
          }
        }
      }

So if those properties are in place, then we should consider that damageLimit (which is TerritoryAttachment.getProduction(battleSite)) should be the max, both for the overall cost and to the cost to each target unit.

Additionally, right after there's code that also enforces the damageLimit per unit if Properties.getDamageFromBombingDoneToUnitsInsteadOfTerritories() is set.

Importantly, if Properties.getDamageFromBombingDoneToUnitsInsteadOfTerritories() is false, bombingRaidDamage isn't used.

That means, we should take that account in the AI code you're changing. Specifically, neededDamageUnits should be 0 in that case.

Otherwise, the other properties should be used to cap the damage per bomber, so I think instead of 3, it should be Math.min(3, maxDamageProduction), if Properties.getPuCap(gameData.getProperties()) || Properties.getLimitSbrDamagePerTurn(gameData.getProperties())).
Note: The 3 is still bogus and would ideally be calculated from the expected value of the bomber's dice, but that can be left as a TODO (3 is close to the expected value of 3.5 for a regular d6). Adding the Math.min() seems a simple enough change that I think it's worthwhile to do here.

[asvitkine]

One other note:

Really what we care about is the application of the limit - and it is pretty peculiar.

if Properties.getDamageFromBombingDoneToUnitsInsteadOfTerritories() is set, then the limit is per unit, so it's straight up maxDamageProduction * targetUnits.size(), if the limit should be applied (based on properties).

If not, then the target units are not relevant, and it's just the overall limit (either infinity or the production value, depending on properties).

[frigoref]

I agree to the case where the property is not set (then only the territory is relevant, meaning its production value/infinitive).
But in the case where the property is set, each unit needs to be checked how much damage it can take and this is added to the territory part (again its production value/infinitive). This is not the same as your formula maxDamageProduction * targetUnits.size().
If that is true the only thing I see that could be changed is the target units part if the property is not set.
Did you wanted to express a different part or did I get you right?

[asvitkine]

Yes, sorry you're right. If the property is set, we actually do damageLimit = current.getHowMuchMoreDamageCanThisUnitTake(battleSite) - so it's not based on maxDamageProduction. My mistake.

[frigoref]

Then this part is resolved, right?

[frigoref]

@asvitkine Fixes made. If you have further points, maybe we should set up a call instead of an extended back-and-forth here.

[frigoref]

@asvitkine Did we cover everything now?

[asvitkine]

@asvitkine Did we cover everything now?

My earlier comments about the different properties were meant to suggest that the code in question should be checking those, because maxDamageProduction + neededDamageUnits is not correct.

For example, if Properties.getDamageFromBombingDoneToUnitsInsteadOfTerritories() is false, then the neededDamageUnits shouldn't be added.

Looking closer at getHowMuchDamageCanThisUnitTakeTotal(), which your code is using, it does check the above property, but if that property is not set, then it returns Integer.MAX_VALUE. So this would be actually a problem for this code, because then you'd be adding a bunch of Integer.MAX_VALUE's together, resulting in overflows, etc.

So you should check the above property yourself.

[frigoref]

My earlier comments about the different properties were meant to suggest that the code in question should be checking those, because maxDamageProduction + neededDamageUnits is not correct.

...

So you should check the above property yourself.

Done, please check.

[asvitkine]

My earlier comments about the different properties were meant to suggest that the code in question should be checking those, because maxDamageProduction + neededDamageUnits is not correct.
...
So you should check the above property yourself.

Done, please check.

I'm not seeing the changes, did you push everything? To make sure it's not just the github UI, I even looked at the raw diff here: https://patch-diff.githubusercontent.com/raw/triplea-game/triplea/pull/10367.diff

I couldn't find the string getDamageFromBombingDoneToUnitsInsteadOfTerritories().

[asvitkine]

Suggestion:

      Collection<Territory> bomberTargetTerritories = bomberEntry.getValue();
      if (raidsMayBePrecededByAirBattles) {
        bomberTargetTerritories =
            CollectionUtils.getMatches(
                bomberTargetTerritories,
                t -> !AirBattle.territoryCouldPossiblyHaveAirBattleDefenders(t, player, data, true));
      }

[frigoref]

Done.

[frigoref]

I'm not seeing the changes, did you push everything? To make sure it's not just the github UI, I even looked at the raw diff here: https://patch-diff.githubusercontent.com/raw/triplea-game/triplea/pull/10367.diff

Apparently it didn't go through. Should be now.

[asvitkine]

OK, looks good now, thanks for working on this!

Just a couple code style suggestions.

[asvitkine]

Nit: I find the negation of this property hard to read, perhaps you can remove the ! and switch the order of the two branches?

[frigoref]

Done. With your below suggestion the switch makes sense, otherwise I would have preferred the one-line-block first.

[asvitkine]

Instead of this contains() check, can you make sameTargetBombers a Set?
Then, this can just be:

Predicate<Unit> canBombTarget = u -> Matches.unitIsLegalBombingTargetBy(u).test(target);
sameTargetBombers.addAll(CollectionUtils.getMatches(existingAttackingBombers, canBombTarget));

[frigoref]

Done.

[asvitkine]

Thanks, merging!