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

WCSumpton · 2024-06-10T11:41:00Z

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 · 2024-06-11T22:09:03Z

@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 · 2024-06-11T23:53:17Z

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 · 2024-06-12T00:30:28Z

@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 · 2024-06-12T09:49:14Z

@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 · 2024-06-12T09:50:36Z

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

Cernelius · 2024-06-13T01:58:01Z

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 · 2024-06-16T01:37:13Z

...e-core/src/main/java/games/strategy/triplea/delegate/power/calculator/AvailableSupports.java

    final SupportDetails details = supportUnits.get(support);
    final IntegerMap<Unit> intMap = details.supportUnits;
-    final Unit u = CollectionUtils.getAny(intMap.keySet());
+    final Unit u = CollectionUtils.getAt(intMap.keySet(), i);


I'm having trouble understanding the significance of this change.
Questions:
(1) Why do we know that the size of intMap.keySet() is less than i?
(2) Why is the i'th element significant? In other words, why that one element and not the first or last one? I think if I spent more time with this, it might "click" and I would understand, but perhaps you can readily give insight here?

Element stored in intMap are, to my understanding, linear. getAny will always return the first element until that element was removed. getAt returns the element at a given position, thus element 1 may still have a count but has already been used, so I want to skip to the next available element with a count.

(1) Why do we know that the size of intMap.keySet() is less than i?

The size of intMap.keySet() should be either greater than or equal to i and is determined prior to calling getNextAvailableSupporter by numSupportAvailableToApply = getSupportersAvailable(rule). And because getNextAvailableSupporter can decrease the size, it is again rechecked with int count = Math.min(i, getSupportersAvailable(rule)).

(2) Why is the i'th element significant? In other words, why that one element and not the first or last one? I think if I spent more time with this, it might "click" and I would understand, but perhaps you can readily give insight here?

This is the problem, IMHO, that we are having right now. CollectionUtils.getAny(intMap.keySet()) only returns the first element for u, and does not consider if that element might have already been use prior in the loop.

As of now giveSupportToUnit uses the sun total of number and is restricted by bonusType/count, and does not care how many units are available. Such that a unit can give as much support as limited by bonusType/count to a given unit on a 1 to x basis as determined by bonusType/count.
This changes giveSupportToUnit to use the number of units available to give support then restricts that by bonusType/count and then tries to choose different supporting units. In assents, a unit can give support to another unit on a 1 to 1 basis.

Hope this makes things clearer.

Cheers...

Just revisiting this, but another option that doesn't have the issue with getAt() discussed in the other thread (i.e. order not guaranteed) is to actually keep the state in an Iterator object. That way, you can just advance through it via next() etc.

DanVanAtta · 2024-06-16T02:05:40Z

lib/java-extras/src/main/java/org/triplea/java/collections/CollectionUtils.java

@@ -161,6 +162,15 @@ public static <T> T getAny(final Iterable<T> elements) {
    return elements.iterator().next();
  }

+  /** Like getAny, but skip elements if possible */
+  public static <T> T getAt(final Iterable<T> elements, final Integer count) {


I don't quite fully understand the context behind why count is significant.
With that caveat, the interface strikes me as odd. Typically the ordering of an iterable is not deterministic, yet we are getting the "n'th" element seemingly?

Ignoring the 'no next element' issue, this example highlights the difference:

// deterministic getAt(new LinkedHashMap().keySet(), 0) vs // not deterministic getAt(new HashMap().keySet(), 0)

At a surface level, this can be somewhat fixed by moving this method to the class that uses it,and making it private and more context specific.

I don't quite fully understand the context behind why count is significant.

count represents the number of times for (int i = 1; i <= numSupportAvailableToApply; i++) has proceeded through the loop adjusted for the size of intMap.keySet(), which can be reduced in getNextAvailableSupporter.

The collection intMap is not being recreated, to my understanding. The same collection is being passed around.

At a surface level, this can be somewhat fixed by moving this method to the class that uses it,and making it private and more context specific.

Ok.

Cheers...

count represents the number of times for (int i = 1; i <= numSupportAvailableToApply; i++) has proceeded through the loop adjusted for the size of intMap.keySet(), which can be reduced in getNextAvailableSupporter.

What does it mean for the value to be adjusted by the size of that intmap? It's not clear to me yet what 'count' represents, what is being counted. Is that the count of supporters that have been 'used' up so far?

DanVanAtta · 2024-07-04T17:31:56Z

@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 · 2024-07-05T18:00:26Z

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!

asvitkine · 2024-07-05T18:02:56Z

lib/java-extras/src/main/java/org/triplea/java/collections/CollectionUtils.java

@@ -161,6 +162,15 @@ public static <T> T getAny(final Iterable<T> elements) {
    return elements.iterator().next();
  }

+  /** Like getAny, but skip elements if possible */
+  public static <T> T getAt(final Iterable<T> elements, final Integer count) {
+    Iterator<T> iterator = elements.iterator();


I don't think there's any guarantee that iterator() will return elements in the same order each time. It can, if the collection is a List, for example, but not if it's a Set (which in this case it is).

So for example, you can have a scenario where the container has elements A, B, C and the first time you call iterator(), it will give you A, B, C order, but the next time, order will be B, A, C. So any calling logic that expects getAt() to be consistent won't work correctly.

If you really want these semantics, you should just convert the collection to a List first outside and just use standard .get().

asvitkine · 2024-07-05T18:04:59Z

...e-core/src/main/java/games/strategy/triplea/delegate/power/calculator/AvailableSupports.java

@@ -149,8 +150,11 @@ int giveSupportToUnit(final Unit unit) {
    return amountOfSupportGiven;
  }

-  private int getSupportAvailable(final UnitSupportAttachment support) {
-    return Math.max(0, Math.min(support.getBonusType().getCount(), getSupportLeft(support)));
+  /** change to retrun the number of supporters (units) not supports */


Comments should describe what the code is doing, not what it got changed to do from some other version of the code.

Also, I don't know what the difference is between "supporters" and "supports" - both sound like they're talking about units. Is "supports" meant to mean something other than units? If so, we should explain.

Also, I don't know what the difference is between "supporters" and "supports" - both sound like they're talking about units. Is "supports" meant to mean something other than units? If so, we should explain.

supporters are units, supports are the number of times this unit can give support.

By counting "supports" as is done presently, a supporter can support a single unit multiple time. A supporter should only support a unit once. If the "support" rule allows for multiple stacking (by "bonusType"/"count"), then multiple "supporter" units should be present to complete that stack.

Cheers...

Also, I don't know what the difference is between "supporters" and "supports" - both sound like they're talking about units. Is "supports" meant to mean something other than units? If so, we should explain.

supporters are units, supports are the number of times this unit can give support.

Are they? I understood that "supporters" are actually supports (so not necessarily units unless in the game there are not support attachments sharing both the same type and the attached unit). That is, if I have two supports for the same unit (that is two support attachments attached to the same unit), they will be able to stack on the same target just as if they come from two different units, just, if they share the same type, they (or at least one of them if you are supporting different counts for the same type across different attachments) need a count greater than 1 for it to do so. I understood that only the case of a single support would never be applied more than once to the same target (regardless of its number, which defines the number of units which that support can support and currently allows to apply the same multiple times to a single unit if the count for the type is greater than 1). Am I not right?

Mind you that I'm not a developer, and I've read none of the coding at this PR.

supports are the number of times this unit can give support

Ah! I think it's a very subtle definition, so I would actually suggest to spell it out (at least in comments).
i.e. when you want to talk about that concept, in comments, actually write "the number of times the unit can give support".

asvitkine · 2024-07-05T18:06:24Z

...e-core/src/main/java/games/strategy/triplea/delegate/power/calculator/AvailableSupports.java

-          final Unit supporter = getNextAvailableSupporter(rule);
+        final int numSupportAvailableToApply = getSupportersAvailable(rule);
+        for (int i = 1; i <= numSupportAvailableToApply; i++) {
+          int count = Math.min(i, getSupportersAvailable(rule));


I don't understand this logic. Wouldn't count always be i in this case?

I don't understand this logic. Wouldn't count always be i in this case?

No, The number of supporters can be reduced by getNextAvailableSupporter. Because of this there has to check if how many supporters available with getSupportersAvailable.

Cheers...

I see, that's very subtle. From reading the code, it's entirely not obvious that getSupportersAvailable() would return different values as we iterate through the loop. At the very least, that should be commented about right above this line.

But perhaps we could also find a way to make it clearer from the code itself - e.g. by renaming that function to give a hint that it's changing (e.g. howManySupportersAreLeft(rule) or something).

asvitkine · 2024-07-05T18:10:24Z

...e-core/src/main/java/games/strategy/triplea/delegate/power/calculator/AvailableSupports.java

+  private int getSupportersAvailable(final UnitSupportAttachment support) {
+    final SupportDetails details = supportUnits.get(support);
+    final IntegerMap<Unit> intMap = details.supportUnits;
+    return Math.max(0, Math.min(support.getBonusType().getCount(), intMap.size()));


Previously it was using getSupportLeft(), which was using details.totalSupport, but now it's using details.intMap.size(). It's not clear why the latter is more correct. A comment in the code would be useful.

asvitkine · 2024-07-05T18:10:53Z

...e-core/src/main/java/games/strategy/triplea/delegate/power/calculator/AvailableSupports.java

+  /** change to retrun the number of supporters (units) not supports */
+  private int getSupportersAvailable(final UnitSupportAttachment support) {
+    final SupportDetails details = supportUnits.get(support);
+    final IntegerMap<Unit> intMap = details.supportUnits;


Previous code was checking whether details was not null. Is it intentional that you're not checking for that anymore - i.e. you're convinced it can't be null? Or an oversight?

asvitkine

I left a number of comments, so marking as "changes requested". Happy to take another look when you've had a chance to address them.

WCSumpton · 2024-07-06T12:18:47Z

...re/src/test/java/games/strategy/triplea/delegate/power/calculator/AvailableSupportsTest.java

@DanVanAtta, @asvitkine, @Cernelius

This change shows the desired results. Presently, "supportUnit" gives 2 support to "unit", while "supportUnit2" gives 2 support to "unit2". If "supportUnit2" is removed, "supportUnit" still gives 2 support to "unit" while "unit2" receives no support. This change changes "supportUnit" to give 1 support to each "unit" and "unit2", while "supportUnit2" gives the stacking support to both "unit" and "unit2". Removing "supportUnit2" will only remove the stacking support as "supportUnit" will still give the initial 1 support to both units.

Maybe these results could be handled better with a list instead of a collection, I'm not sure. I have created test maps using "neuschwabenland" and also "invasion_usa" and have received no errors.

Cheers...

beelee1 · 2024-07-16T04:10:35Z

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

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

DanVanAtta reviewed Jun 16, 2024

View reviewed changes

asvitkine reviewed Jul 5, 2024

View reviewed changes

asvitkine requested changes Jul 5, 2024

View reviewed changes

WCSumpton commented Jul 6, 2024

View reviewed changes

WCSumpton changed the title ~~Remapping of AvailableSupports to count supporter (units) instead of supports.~~ Changing how stacked supports handled. Considering the supporting unit, number of units supported and bonusType and count. Jul 7, 2024

WCSumpton changed the title ~~Changing how stacked supports handled. Considering the supporting unit, number of units supported and bonusType and count.~~ Changing how stacked supports handled. Considering the supporting unit, number of units supported and bonusType/count. Jul 7, 2024

WCSumpton changed the title ~~Changing how stacked supports handled. Considering the supporting unit, number of units supported and bonusType/count.~~ Changing how stacked supports are handled. Considering the supporting unit, number of units supported and bonusType/count. Jul 7, 2024

WCSumpton closed this Jul 13, 2024

WCSumpton force-pushed the master branch from 5f1a345 to d8c5876 Compare July 13, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

WCSumpton commented Jun 10, 2024

DanVanAtta commented Jun 11, 2024

WCSumpton commented Jun 11, 2024 •

edited

Loading

WCSumpton commented Jun 12, 2024

Cernelius commented Jun 12, 2024

Cernelius commented Jun 12, 2024

Cernelius commented Jun 13, 2024

DanVanAtta Jun 16, 2024

WCSumpton Jun 16, 2024

WCSumpton Jun 18, 2024 •

edited

Loading

asvitkine Jul 10, 2024

DanVanAtta Jun 16, 2024

WCSumpton Jun 18, 2024

DanVanAtta Jul 4, 2024

DanVanAtta commented Jul 4, 2024

asvitkine commented Jul 5, 2024 •

edited

Loading

asvitkine Jul 5, 2024

asvitkine Jul 5, 2024

asvitkine Jul 5, 2024

WCSumpton Jul 6, 2024

Cernelius Jul 6, 2024 •

edited

Loading

asvitkine Jul 6, 2024

asvitkine Jul 5, 2024

WCSumpton Jul 6, 2024

asvitkine Jul 6, 2024

asvitkine Jul 5, 2024

asvitkine Jul 5, 2024

asvitkine left a comment

WCSumpton Jul 6, 2024

beelee1 commented Jul 16, 2024 •

edited

Loading

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

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

Conversation

WCSumpton commented Jun 10, 2024

DanVanAtta commented Jun 11, 2024

WCSumpton commented Jun 11, 2024 • edited Loading

WCSumpton commented Jun 12, 2024

Cernelius commented Jun 12, 2024

Cernelius commented Jun 12, 2024

Cernelius commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WCSumpton Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanVanAtta commented Jul 4, 2024

asvitkine commented Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cernelius Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvitkine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beelee1 commented Jul 16, 2024 • edited Loading

WCSumpton commented Jun 11, 2024 •

edited

Loading

WCSumpton Jun 18, 2024 •

edited

Loading

asvitkine commented Jul 5, 2024 •

edited

Loading

Cernelius Jul 6, 2024 •

edited

Loading

beelee1 commented Jul 16, 2024 •

edited

Loading