Changing how stacked supports are handled. Considering the supporting unit, number of units supported and bonusType/count.

[WCSumpton]

As discussed here.

This changes the way AvailableSupports handles giving supports to units.

In AvailableSupports method getSupportsAvailable is renamed getSupporterAvailable and now returns the count of supporting units instead of the number of supports. Although method getSupportLeft has not been changed, it is now only used for testing. Method giveSupportToUnit will now send the count to method getNextAvailableSupporter. There is a check for the number of supporters prior to this call because the number of supporters can be reduced in getNextAvailableSupporter while looping through supporters. Method getNextAvailableSupporter is changed to allow a count to be passed (getAny changed to getAt) when selecting a supporting unit.

In CollectionUtils a new method, getAt, is designed to loop through a collection and pass back an element instead of selecting the first element.

In AvailableSupportsTest only method twoSupportersInAStackWithSupportNumberOfTwoGiveSupportToTwoUnits was changed to reflect that support and bonusType are divided between units, and not given to one unit by supporting units.

As an added bonus method getNextAvailableSupporter was changed to now allow number to be set to -1 (infinite), which could not be done because AvailableSupports was counting supports and a -1 value would cause the supporter to be removed after giving support the first time.

Cheers...

[DanVanAtta]

@WCSumpton - could you describe how to test this (and perhaps what testing you did)? Specifically, which maps & which supporter scenarios would really show a difference?

[WCSumpton]

This was tested using the same test that was used to test that was used, AvailableSupportsTest. Using this process a multi-unit support is possible. Whereas the present process does not allow that kind of support. Suppose you wanted to create a "WolfPack" support for German Submarines. Submarines attack at 2 defend at 1. Now, if 3 Submarines are together you want them to attack at 3 and defend at 2.

<!-- German WolfPack support -->
<!-- Requires 3 Submarines stacked together -->
<attachment name="supportAttachmentSubmarineBuff" attachTo="Submarine" javaClass="UnitSupportAttachment" type="unitType">
    <option name="unitType" value="Submarine"/>
    <option name="faction" value="allied"/>
    <option name="side" value="offence:defence"/>
    <option name="dice" value="strength"/>
    <option name="bonus" value="1"/>
    <option name="number" value="3"/>
    <option name="bonusType" value="SubmarineBuff" count="3"/>
    <option name="players" value="German"/>
</attachment>

<attachment name="supportAttachmentSubmarineDebuff" attachTo="Submarine" javaClass="UnitSupportAttachment" type="unitType">
    <option name="unitType" value="Submarine"/>
    <option name="faction" value="allied"/>
    <option name="side" value="offence:defence"/>
    <option name="dice" value="strength"/>
    <option name="bonus" value="-1"/>
    <option name="number" value="2"/>
    <option name="bonusType" value="SubmarineDebuff" count="2"/>
    <option name="players" value="German"/>
</attachment>

The above support will grant the bonus in groups of 3. Any odd number is not supported. If number in the above supports is changed to "-1" then the support is granted when there are more than three units. This type of support is undoable at present. Or maybe you want Anti-Tank Guns to support Infantry when an enemy Armor is attacking? Can't do it. But with this change it's possible.

Cheers...

[WCSumpton]

@DanVanAtta

The above works, what I'm having problem with is getting the BattleRound counter to be read inside AvailableSupports so that a maxRounds process can be added. But the above still works as stated.

Cheers...

[Cernelius]

@WCSumpton - could you describe how to test this (and perhaps what testing you did)? Specifically, which maps & which supporter scenarios would really show a difference?

I don't think any existent games are going to change (but I cannot be sure). My understanding is that this is for example to change the answer to the question "if I have an infantry which can be supported by up to two artilleries and an artillery which can support up to two infantries, can the infantry be supported twice by a single artillery?" from "yes" to "no", which I think makes more logical sense and gives more options without substantially removing any (because you can still answer "yes" by exchanging the implied attachment with number equal to 2 with two otherwise identical attachments each with number equal to 1).

[Cernelius]

I believe the existent behaviour was (maybe not completely intentionally) defined by @ron-murhammer (if he wants to weight in).

[Cernelius]

I want to add that I'm not actually reading this: what I said is based on what I understood @WCSumpton is doing.

For example, I'm just assuming that, if an artillery has two support attachments at count 2 each, this single artillery can support the same infantry twice both before and after these changes, even though the title of this PR seems to state otherwise...

[DanVanAtta]

@WCSumpton my apologies for the drawn out review. (A) this existing code was really hard to understand & this makes it difficult to review. (B) I'm really stretched thin between life and the things needed for 2.6 to be live. That latter is really important since no updates will be seen by anyone until that happens. Once 2.6 is out for real, the release model changes and suddenly updates are going to be live right away.

I don't know when I can finish reviewing... @asvitkine perhaps you might be able to take over the review?

@WCSumpton I think one question I would have - how confident are you that the updated test cases comprehensively validate the existing and updated logic? In other words - is the test suite very comprehensive? Do you have very high confidence this works based on manual testing, based on high test coverage from automated tests, or both?

I think if we are very confident in the automated tests and have extensively manually tested to cover all the variety of map scenarios, then perhaps we can skip further review. While I hear you when you say "this is correct" - we all always have to try and make sure, and try to make sure that is the case ideally in more than just one way too.

[asvitkine]

As discussed here.

This changes the way AvailableSupports handles giving supports to units.

In AvailableSupports method getSupportsAvailable is renamed getSupporterAvailable and now returns the count of supporting units instead of the number of supports. Although method getSupportLeft has not been changed, it is now only used for testing. Method giveSupportToUnit will now send the count to method getNextAvailableSupporter. There is a check for the number of supporters prior to this call because the number of supporters can be reduced in getNextAvailableSupporter while looping through supporters. Method getNextAvailableSupporter is changed to allow a count to be passed (getAny changed to getAt) when selecting a supporting unit.

In CollectionUtils a new method, getAt, is designed to loop through a collection and pass back an element instead of selecting the first element.

In AvailableSupportsTest only method twoSupportersInAStackWithSupportNumberOfTwoGiveSupportToTwoUnits was changed to reflect that support and bonusType are divided between units, and not given to one unit by supporting units.

As an added bonus method getNextAvailableSupporter was changed to now allow number to be set to -1 (infinite), which could not be done because AvailableSupports was counting supports and a -1 value would cause the supporter to be removed after giving support the first time.

Cheers...

I'm going to review the code changes in detail, but first I'd like to ask you to update the PR description to clearly describe the behavior change (i.e. as would be visible to a player or mapmaker) - separately from the description about which functions/code was changed. I can figure out the latter myself by reviewing the code, but the former should be clearly documented without mentioning specific code - i.e. document the spec that this is implementing and how it differs before vs. after. Provide examples if possible!

[beelee1]

@WCSumpton so did this get merged ? Part of latest pre ?

Edit nvm :) just saw your post at the site :)