Triplea: Player cannot share war bond income with allies when player is unable to collect income

[ssoloff]

I discovered this issue while recently refactoring some of the delegate classes. Unfortunately, I don't have in-game reproduction steps and can only provide evidence via code inspection.

In the AbstractEndTurnDelegate#start() method, the canPlayerCollectIncome() method is called to determine if the player can collect income for this turn. If it returns false, and the player has the War Bond tech, an attempt is made to share the war bond income with one of the player's allies:

    // can't collect unless you own your own capital
    if (!canPlayerCollectIncome(m_player, data)) {
      endTurnReport.append(rollWarBondsForFriends(m_bridge, m_player, data));
      // we do not collect any income this turn
    } else {
      // just collect resources

Within rollWarBondsForFriends(), canPlayerCollectIncome() is called for each of the allies in order to locate the first ally who is capable of receiving the war bond income:

    // take first one
    PlayerID giveWarBondsTo = null;
    for (final PlayerID p : shareWith) {
      final int pCount = TechAbilityAttachment.getWarBondDiceNumber(p, data);
      final int pSides = TechAbilityAttachment.getWarBondDiceSides(p, data);
      if (pSides <= 0 && pCount <= 0) {
        // if both are zero, then it must mean we did not share our war bonds tech with them, even though we are sharing
        // all tech (because
        // they cannot have this tech)
        if (canPlayerCollectIncome(p, data)) {
          giveWarBondsTo = p;
          break;
        }
      }
    }

Unfortunately, canPlayerCollectIncome() ignores its arguments and always uses the player and game data associated with the delegate. Thus, because canPlayerCollectIncome() for the player associated with the delegate is returning false, this call will always return false regardless of its arguments, and no ally will be able to receive the war bond income (i.e. giveWarBondsTo will always be null upon exit from the loop).

canPlayerCollectIncome() was changed to ignore its parameters in 2f024b0. From the context of that commit alone, I cannot tell if it was intentional. I'm leaning towards it being unintentional because, for the call to canPlayerCollectIncome() within start(), the player and data arguments would be equivalent (i.e. player is m_player and data is m_bridge.getData() [aka this.getData()]). Thus, I can't think of a reason why it would be changed to ignore them.

Can someone please review this code to confirm whether or not this is an actual bug?

[ssoloff]

I discovered a secondary issue, as well. I'm reporting it here because it only affects the feature concerned in this issue. I'll break it out into a separate issue, if necessary.

I'm confused by the logic of the TechAbilityAttachment#getWarBondDiceSides() method. It simply sums the war bond dice sides from all attachments:

  public static int getWarBondDiceSides(final PlayerID player, final GameData data) {
    int rVal = 0;
    for (final TechAdvance ta : TechTracker.getCurrentTechAdvances(player, data)) {
      final TechAbilityAttachment taa = TechAbilityAttachment.get(ta);
      if (taa != null) {
        final int sides = taa.getWarBondDiceSides();
        if (sides > 0) {
          rVal += sides;
        }
      }
    }
    return Math.max(0, rVal);
  }

There is similar code for TechAbilityAttachment#getWarBondDiceNumber(), which sums the war bond dice count from all attachments.

Ultimately, the summed number and sides are used in a call to DiceRoll#rollNDice(). While I have no issue with summing the dice number from all attachments, I don't see how summing the dice sides can possibly work, even if the dice sides from different attachments are equal.

For example, assume one attachment contributes 1d6 income from war bonds and another attachment contributes 2d6 income from war bonds. Logically, that should sum to a total of 3d6, but with the current logic, it is actually 3d12.

The only case where this logic appears to work is if zero or one attachment contributes income from war bonds. Two or more attachments contributing income will result in potentially more war bond income than should be possible.

It seems like there should be a single TechAbilityAttachment#getWarBondDice() method that returns a collection of tuples (or some other class representing a die roll), where the elements of the tuple are the die number and sides. Then for each die roll in the tuple, DiceRoll#rollNDice() is called and the results are aggregated.

Is this a problem or am I not understanding something fundamental about dice rolls in this context?

[ron-murhammer]

@ssoloff Sorry to say but this is a little hard to follow. I'd recommend picking a map and trying to reproduce the bug or bugs within an actual game. Otherwise its really hard to tell given the mess of code in the delegates.

[ssoloff]

@ron-murhammer Ok, I'll try to reproduce in-game or with a unit test.

[simon33-2]

Sounds like the system is working correctly. I cannot see a reason why war bond income should be shared with allies according to the rules. Does this apply to any map?

[ssoloff]

@ron-murhammer @simon33-2

After reproducing the problem, I realized the issue title is not quite accurate. Instead of saying just "allies", I should have said "allies with whom the player shares technology but can't research war bonds." That's a mouthful... :smiley:

The bug requires the following game configuration:

• warBonds technology researchable by player
• shareTechnology player attachment between player and one of their allies
• warBonds technology must not be researchable by allied player

The World War II Global 1940 2nd Edition map satisfies this criteria with British as the player and UK_Pacific as the allied player. (In fact, from the comments in the code, it appears this feature was added specifically for this map.)

I've attached a save game at the start of the British player's turn after their capital has been captured. Note that I modified the game XML to immediately give the British player the warBonds technology to speed up the repro steps. This shouldn't have any effect on the bug, but I'm not sure if it will prevent someone from loading the above save game. For reference, here's the change I made to ww2global40_2nd_edition.xml:

diff --git a/map/games/ww2global40_2nd_edition.xml b/map/games/ww2global40_2nd_edition.xml
index fd29b10..ea91be6 100644
--- a/map/games/ww2global40_2nd_edition.xml
+++ b/map/games/ww2global40_2nd_edition.xml
@@ -2060,7 +2060,7 @@
       <option name="improvedArtillerySupport" value="false"/>
       <option name="rocket" value="false"/>
       <option name="increasedFactoryProduction" value="false"/>
-      <option name="warBonds" value="false"/>
+      <option name="warBonds" value="true"/>
     </attachment>
     <attachment name="techAttachment" attachTo="UK_Pacific" javaClass="games.strategy.triplea.attachments.TechAttachment" type="player">
       <option name="superSub" value="false"/>

Now for the repro...

---

Bugged code on master (b1af04e; 1.9.0.0.4423)

After playing through the British player's turn, I see no messages pop up, including the end-of-turn message. I am immediately presented with the UK_Pacific player's end-of-turn message:

You can see that the UK_Pacific player only receives the PUs they produced during that turn. They do not receive any war bond income from the British player.

Fixed code

I made the following modification to AbstractEndTurnDelegate#canPlayerCollectIncome():

diff --git a/src/main/java/games/strategy/triplea/delegate/AbstractEndTurnDelegate.java b/src/main/java/games/strategy/triplea/delegate/AbstractEndTurnDelegate.java
index 4b3809a..d610e1d 100644
--- a/src/main/java/games/strategy/triplea/delegate/AbstractEndTurnDelegate.java
+++ b/src/main/java/games/strategy/triplea/delegate/AbstractEndTurnDelegate.java
@@ -53,7 +53,7 @@ public abstract class AbstractEndTurnDelegate extends BaseTripleADelegate implem
   }

   public boolean canPlayerCollectIncome(final PlayerID player, final GameData data) {
-    return TerritoryAttachment.doWeHaveEnoughCapitalsToProduce(m_player, getData());
+    return TerritoryAttachment.doWeHaveEnoughCapitalsToProduce(player, data);
   }

   /**

After playing through the British player's turn, I first see the war bonds roll message:

I then see the British player's end-of-turn message, which indicates the UK_Pacific player receives the war bond income:

Finally, I see the UK_Pacific player's end-of-turn message, which indicates they received additional income (from the war bonds) beyond their normal PU production:

---

Just to be sure the above change didn't cause a regression, I re-started the game to ensure the British player still correctly collects their war bond income when their capital has not been captured:

[ron-murhammer]

@ssoloff Interesting. There are a lot of global players around so I wonder if they've seen this bug before.

[panther2]

@ssoloff
@ron-murhammer

Playing global with tech enabled is not very common so chances are that it had not been realized by many players before.

But the issue is addressed in the Global game notes:

Edit Mode:
... The Warbonds technology will give the PUs to the British, please use edit mode if you want some of it to go to UK Pacific. ...

I cannot read from the above what is or has been incorporated until now so I add what - according to the rules - should happen:

As technology applies to the UK nation as a whole, the roll for War Bonds should not occur before the Collect Income phase of UK-pacific. Maybe (for programming reasons) it would be even better to let it occur immediately after that as a separate step. In any case there should be a pop-up asking the player how to distribute the dice result to the two economies.

This is according to the official FAQ:

Q. If the United Kingdom gains the War Bonds breakthrough, which economy gets the IPCs?
A. The IPCs may be divided between the two economies each turn in whichever way the United Kingdom player likes, including all of them to one economy and none of them to the other.

In case one regional capital gets captured, the War bonds result of course completely goes to the other one.

[ssoloff]

Playing global with tech enabled is not very common so chances are that it had not been realized by many players before.

Thanks, @panther2, for reminding me that I forgot to include a very important requirement in the repro steps: namely, that Tech Development must be enabled in the Map Options. I forgot to mention that because, as I said above, I cheated and edited the game configuration to automatically grant War Bonds to the British from the start of the game. :smiley:

I can confirm the code currently does not allow the player with War Bonds to split the War Bond income between the two economies. There are currently only two code paths:

• If the player can collect income, all War Bond income goes to the player
• If the player cannot collect income, all War Bond income goes to the first ally with which the player shares tech but does not possess the War Bonds tech ("first" means in the order player attachments are returned; I don't know if that's deterministic or not)

So, as you pointed out from the game notes, the only way to split War Bond income is via edit mode.

[ssoloff]

@panther2 Since you are here, let me ask your opinion on the secondary issue I brought up in this PR, but from a player's perspective.

I can't think of a realistic use case where a map would have more than one War Bonds tech attachment. Maybe the only reason would be if the map maker wanted to generate War Bond income using two dice with different sides? For example, if the map maker wanted War Bond income to be generated using the expression "1d6+1d8":

<attachment name="techAbilityAttachment" attachTo="warBonds" javaClass="games.strategy.triplea.attachments.TechAbilityAttachment" type="technology">
  <option name="warBondDiceSides" value="6"/>
  <option name="warBondDiceNumber" value="1"/>
</attachment>
<attachment name="techAbilityAttachment" attachTo="warBonds" javaClass="games.strategy.triplea.attachments.TechAbilityAttachment" type="technology">
  <option name="warBondDiceSides" value="8"/>
  <option name="warBondDiceNumber" value="1"/>
</attachment>

Have you ever seen something like that (not necessarily just for War Bonds) on any map? Even if you haven't, do you think this use case is reasonable?

Regardless, as I said above, if someone did create multiple War Bonds tech attachments like this, the code currently would not roll "1d6+1d8" but would rather roll "2d14".

[panther2]

I agree with you, @ssoloff, that the secondary issue you have pointed out most likely has no realistic use case. Of course there are maps that use other than d6 but I never experienced any "War Bond Case" with different sided dice, neither have I ever heard of multiple War Bond tech attachments.
But honestly, I do not know every map in play ...

But at least the "logic" behind the "addition" of the dice roll you pointed out, appears to be a bug rather than a feature, IMHO.

[simon33-2]

Interesting. It wouldn't be too annoying to use edit mode to split the war bond income but if it isn't generated at all that is another level of irritation. I'm sure this has never happened in any game I have played but it would still be good to have the fix.

Is #1645 too difficult?

[ssoloff]

@simon33-2 #1645 appears that it is going to require significant redesign and refactoring within the delegates. I was holding off until I got the serialization proxies in place so I didn't have to worry about backwards compatibility.

Also, MustFightBattle is almost 3,000 lines long and there's a lot of code that isn't covered by tests (it's got 67% instruction coverage and 65% branch coverage). determineStepStrings() itself has a complexity score of 79 and only about 2/3 of it is covered. I don't feel comfortable modifying such an integral piece of the engine without having tests in place to prevent regressions. So my first step is to add test vices around some of this existing code. I actually planned to start doing that a few weeks ago but got side-tracked by other things.

[ron-murhammer]

@simon33-2 @ssoloff Yeah, let's be very careful about major delegate refactors. They are very complex and very brittle. Lots of weird side effects that are hard to track as well. Making a major change to MustFightBattle would require a massive amount of testing.

[simon33-2]

That change to must fight battle needs to be done. We shouldn't be shy about making an incompatible release for it.