Add count for support bonus type

[ron-murhammer]

Addresses: https://forums.triplea-game.org/topic/1560/stack-unit-support

A count can now be specified for a supportAttachment bonusType which specifies how many units can stack support onto the given support target (previously it was always just 1). The count can be any numerical value with 1 being the default but anything less than 1 will represent infinite stacking (0, -1, etc). Infinite stacking for strength support is not advised because you still have the dice sides limit it but infinite stacking for roll modifiers can make sense.

New bonusType optional count parameter

<option name="bonusType" value="Cover" count="3"/>

Infantry receives support from up to 2 artillery

        <attachment name="supportAttachmentArtillerygerman" attachTo="germanArtillery" javaClass="UnitSupportAttachment" type="unitType">
            <option name="unitType" value="germanInfantry:germanMarine:germanCombatEngineer:germanAlpineInfantry:germanParatrooper"/>
            <option name="faction" value="allied"/>
            <option name="side" value="offence"/>
            <option name="dice" value="strength"/>
            <option name="bonus" value="1"/>
            <option name="number" value="1"/>
            <option name="bonusType" value="ArtilleryBonus" count="2"/>
            <option name="impArtTech" value="false"/>
            <option name="players" value="Germany"/>
        </attachment>

Infantry receives support from up to 3 static defenses (entrenchment, fortification)

        <attachment name="supportAttachmentEntrenchmentgerman" attachTo="germanEntrenchment" javaClass="UnitSupportAttachment" type="unitType">
            <option name="unitType" value="germanInfantry:germanMarine:germanCombatEngineer:germanAlpineInfantry:germanParatrooper:italianInfantry:italianMarine:italianCombatEngineer:italianAlpineInfantry:italianParatrooper:japaneseInfantry:japaneseMarine:japaneseCombatEngineer:japaneseAlpineInfantry:japaneseParatrooper"/>
            <option name="faction" value="allied"/>
            <option name="side" value="defence"/>
            <option name="dice" value="strength"/>
            <option name="bonus" value="1"/>
            <option name="number" value="2"/>
            <option name="bonusType" value="FortBonus" count="3"/>
            <option name="impArtTech" value="false"/>
            <option name="players" value="Germany"/>
        </attachment>
        <attachment name="supportAttachmentFortificationgerman" attachTo="germanFortification" javaClass="UnitSupportAttachment" type="unitType">
            <option name="unitType" value="germanInfantry:germanMarine:germanCombatEngineer:germanAlpineInfantry:germanParatrooper:italianInfantry:italianMarine:italianCombatEngineer:italianAlpineInfantry:italianParatrooper:japaneseInfantry:japaneseMarine:japaneseCombatEngineer:japaneseAlpineInfantry:japaneseParatrooper"/>
            <option name="faction" value="allied"/>
            <option name="side" value="defence"/>
            <option name="dice" value="strength"/>
            <option name="bonus" value="2"/>
            <option name="number" value="3"/>
            <option name="bonusType" value="FortBonus" count="3"/>
            <option name="impArtTech" value="false"/>
            <option name="players" value="Germany"/>
        </attachment>

Functional Changes

[] New map or map update
[x] New Feature
[] Feature update or enhancement
[] Feature Removal
[] Code Cleanup or refactor
[] Configuration Change
[] Bug fix:
[] Other:

Testing

[] No manual testing done
[x] Manually tested

Ran a few maps like revised on all AI to ensure no regressions. Edited TWW XML to test the new parameter and opened the BC to see the calculated power.

[DanVanAtta]

Most notable comment is about 'tuple', I will need to revisit the updates in DiceRoll for a more detailed look, otherwise looks pretty straight forward

[codecov-io]

Codecov Report

Merging #5309 into master will increase coverage by 0.01%.
The diff coverage is 73.68%.

@@             Coverage Diff              @@
##             master    #5309      +/-   ##
============================================
+ Coverage     24.12%   24.13%   +0.01%     
- Complexity     6742     6746       +4     
============================================
  Files          1008     1008              
  Lines         77141    77158      +17     
  Branches      11496    11500       +4     
============================================
+ Hits          18608    18621      +13     
- Misses        56398    56401       +3     
- Partials       2135     2136       +1

Impacted Files	Coverage Δ	Complexity Δ
...s/strategy/triplea/attachments/UnitAttachment.java	41.57% <0%> (ø)	270 <0> (ø)	⬇️
.../games/strategy/engine/data/DefaultAttachment.java	95.55% <100%> (ø)	25 <1> (ø)	⬇️
...egy/triplea/attachments/UnitSupportAttachment.java	72.29% <58.82%> (-1.23%)	57 <2> (+1)
...java/games/strategy/triplea/delegate/DiceRoll.java	66.13% <89.47%> (+0.39%)	145 <1> (+2)	⬆️
.../src/main/java/games/strategy/net/nio/Decoder.java	67.24% <0%> (+1.72%)	12% <0%> (+1%)	⬆️

---

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 37e2014...680a78a. Read the comment docs.

[DanVanAtta]

Understanding a bit better, I really wonder about '0' being infinite. I'm concerned it is a bit of a magic number. I wonder if there could be a case where an initial support count is '0', but say a technology can be made that would increase the value, for example, infantry with a 'combined arms tech' start to give a bonus to tanks.

I wonder if we can't just make this explicit, eg:

<option name="bonusType" value="Cover" unlimited="true"/>

@ron-murhammer what do you think? In part I was looking at how the '0' is translated to a max integer, which seems a bit odd that we go from 0 to a max, but then we do a min against that max. I wonder as well if that logic can't be simplified so that the 'max' is not more or less just ignored, but particularly avoiding the complexity where a zero is translated to another value that is then effectively a no-op.

[ron-murhammer]

@DanVanAtta 0 and -1 or anything less than those are used as infinite for a number of other properties. If you think there is a case for setting to 0 then I'm open to having that essentially just mean it doesn't apply and only <0 is infinite.

[DanVanAtta]

You seem opposed to an explicit "unlimited" or "infinite" string, any reason to not have that be an explicit property? As noted, tech advances perhaps could seek to adjust the support limit and plausibly could introduce a use-case for a '0'. More fundamentally, I would like to avoid the magic number altogether.

[DanVanAtta]

I'm really thinking the logic in DiceRoll is already beyond the tipping point of poorly designed, and we'll have yet more functionality in it. I'm deeply concerned for the maintainability of this code, to grok what is going on, one needs to be aware of multiple game features and be sure not any of them break. It's good we have some unit test coverage, that gives us some confidence in updating the code, nonetheless it's a herculean task to understand the logic in 'dice roll'.

I really think this is over the tipping point for maintenance cost. Moving logic and new logic into the new value object I think can help keep the status quo, otherwise something needs to happen to keep the code understandable, we're already beyond that tipping point here and this code needed/needs some help.

[Cernelius]

You seem opposed to an explicit "unlimited" or "infinite" string, any reason to not have that be an explicit property? As noted, tech advances perhaps could seek to adjust the support limit and plausibly could introduce a use-case for a '0'. More fundamentally, I would like to avoid the magic number altogether.

I really despise this "-1 is infinite duh" convention, and opened a feature request about it here:
https://forums.triplea-game.org/topic/912/a-general-way-to-define-infinite

Tho, in all fairness, either you rework all or nothing. So I would go with the -1 thing here, for now, rather than being inconsistent with anything else.

I would personally prefer that a "+" means positive infinite and a "-" means negative infinite.

In the case of this property, this would be my suggestion of infinite:
<option name="bonusType" value="ArtilleryBonus" count="+"/>

If not supporting two infinite, then I would have infinite being an empty count, that would be:
<option name="bonusType" value="ArtilleryBonus" count=""/>

I don't like having any letters in numeral fields. For the usualy infinity symbol, I don't like using symbols that are not part of common keyboards (meaning I would only use symbols available for both the QWERTY and DVORAK keyboards).

[ron-murhammer]

@DanVanAtta Refactored a bit more. Without a major refactoring of DiceRoll, I don't think there is much more than can be done here.

[DanVanAtta]

Thanks for reviewing that @ron-murhammer ; I recognize the value of adding features, and it's unfair that we have to pay the technical debt now and before. I think the bar is that we just do not increase complexity, keep same or decrease and we'll be in better shape.

I appreciate the update 👍
Agree a larger overhaul is needed, but it feels like the complexity added is no longer that much more.

[DanVanAtta]

@Cernelius agree, good point on how the other attributes are working