From c53a41c10f0ef09ced86ce6cc384ed83c4ee2341 Mon Sep 17 00:00:00 2001 From: Sara Freimer Date: Sat, 20 Apr 2024 10:37:04 -0500 Subject: [PATCH] Improve remainder distribution to try and split the remainder as evenly as possible between the various destinations before falling back to sending to the first one it will fit in (#6617) (#8062) Co-authored-by: Thiakil --- .../distribution/FloatingLongSplitInfo.java | 18 +++++ .../lib/distribution/IntegerSplitInfo.java | 34 ++++++++- .../lib/distribution/LongSplitInfo.java | 32 +++++++- .../common/lib/distribution/SplitInfo.java | 45 ++++++++++++ .../common/lib/distribution/Target.java | 71 +++++++++++++++--- .../lib/distribution/DistributionTest.java | 73 +++++++++++++++++-- .../handler/LyingAmountIntegerHandler.java | 25 +++++++ 7 files changed, 274 insertions(+), 24 deletions(-) create mode 100644 src/test/java/mekanism/common/lib/distribution/handler/LyingAmountIntegerHandler.java diff --git a/src/main/java/mekanism/common/lib/distribution/FloatingLongSplitInfo.java b/src/main/java/mekanism/common/lib/distribution/FloatingLongSplitInfo.java index 79240ab7a0c..4ade8f8102b 100644 --- a/src/main/java/mekanism/common/lib/distribution/FloatingLongSplitInfo.java +++ b/src/main/java/mekanism/common/lib/distribution/FloatingLongSplitInfo.java @@ -20,10 +20,18 @@ public void send(FloatingLong amountNeeded) { //If we are giving it, then lower the amount we are checking/splitting boolean recalculate; if (amountNeeded.isZero()) { + if (!decrementTargets) { + //If we are not decrementing targets, then don't remove that as a valid target, or update how much there is per target + return; + } recalculate = true; } else { amountToSplit = amountToSplit.minusEqual(amountNeeded); sentSoFar = sentSoFar.plusEqual(amountNeeded); + if (!decrementTargets) { + //If we are not decrementing targets, then don't remove that as a valid target, or update how much there is per target + return; + } recalculate = !amountNeeded.equals(amountPerTarget); } toSplitAmong--; @@ -50,6 +58,16 @@ public FloatingLong getRemainderAmount() { return amountPerTarget; } + @Override + public FloatingLong getUnsent() { + return amountToSplit; + } + + @Override + public boolean isZero(FloatingLong value) { + return value.isZero(); + } + @Override public FloatingLong getTotalSent() { return sentSoFar; diff --git a/src/main/java/mekanism/common/lib/distribution/IntegerSplitInfo.java b/src/main/java/mekanism/common/lib/distribution/IntegerSplitInfo.java index 8b790572d2c..5677c7486fd 100644 --- a/src/main/java/mekanism/common/lib/distribution/IntegerSplitInfo.java +++ b/src/main/java/mekanism/common/lib/distribution/IntegerSplitInfo.java @@ -5,11 +5,13 @@ public class IntegerSplitInfo extends SplitInfo { private int amountToSplit; private int amountPerTarget; private int sentSoFar; + private int remainder; public IntegerSplitInfo(int amountToSplit, int totalTargets) { super(totalTargets); this.amountToSplit = amountToSplit; amountPerTarget = toSplitAmong == 0 ? 0 : amountToSplit / toSplitAmong; + remainder = toSplitAmong == 0 ? 0 : amountToSplit % toSplitAmong; } @Override @@ -17,12 +19,22 @@ public void send(Integer amountNeeded) { //If we are giving it, then lower the amount we are checking/splitting amountToSplit -= amountNeeded; sentSoFar += amountNeeded; + if (!decrementTargets) { + //If we are not decrementing targets, then don't remove that as a valid target, or update how much there is per target + int difference = amountNeeded - amountPerTarget; + if (difference > 0) { + //If we removed more than we have per target, we need to remove the excess from our remainder + remainder -= difference; + } + return; + } toSplitAmong--; //Only recalculate it if it is not willing to accept/doesn't want the // full per side split if (amountNeeded != amountPerTarget && toSplitAmong != 0) { int amountPerLast = amountPerTarget; amountPerTarget = amountToSplit / toSplitAmong; + remainder = amountToSplit % toSplitAmong; if (!amountPerChanged && amountPerTarget != amountPerLast) { amountPerChanged = true; } @@ -31,15 +43,29 @@ public void send(Integer amountNeeded) { @Override public Integer getShareAmount() { + //TODO: Should we make this return a + 1 if there is a remainder, so that we can factor out those cases that can accept exactly amountPerTarget + 1 + // while doing our initial loop rather than handling it via getRemainderAmount? return amountPerTarget; } @Override public Integer getRemainderAmount() { - //Add to the remainder amount the entire remainder so that we try to use it up if we can - // The remainder then if it cannot be fully accepted slowly shrinks across each target we are distributing to - //TODO: Evaluate making a more even distribution of the remainder - return toSplitAmong == 0 ? amountPerTarget : amountPerTarget + (amountToSplit % toSplitAmong); + if (toSplitAmong != 0 && remainder > 0) { + //If we have a remainder, be willing to provide a single unit as the remainder + // so that we split the remainder more evenly across the targets. + return amountPerTarget + 1; + } + return amountPerTarget; + } + + @Override + public Integer getUnsent() { + return amountToSplit; + } + + @Override + public boolean isZero(Integer value) { + return value == 0; } @Override diff --git a/src/main/java/mekanism/common/lib/distribution/LongSplitInfo.java b/src/main/java/mekanism/common/lib/distribution/LongSplitInfo.java index ff8f401e0f9..e71359d3bf7 100644 --- a/src/main/java/mekanism/common/lib/distribution/LongSplitInfo.java +++ b/src/main/java/mekanism/common/lib/distribution/LongSplitInfo.java @@ -5,11 +5,13 @@ public class LongSplitInfo extends SplitInfo { private long amountToSplit; private long amountPerTarget; private long sentSoFar; + private long remainder; public LongSplitInfo(long amountToSplit, int totalTargets) { super(totalTargets); this.amountToSplit = amountToSplit; amountPerTarget = toSplitAmong == 0 ? 0 : amountToSplit / toSplitAmong; + remainder = toSplitAmong == 0 ? 0 : amountToSplit % toSplitAmong; } @Override @@ -17,12 +19,22 @@ public void send(Long amountNeeded) { //If we are giving it, then lower the amount we are checking/splitting amountToSplit -= amountNeeded; sentSoFar += amountNeeded; + if (!decrementTargets) { + //If we are not decrementing targets, then don't remove that as a valid target, or update how much there is per target + long difference = amountNeeded - amountPerTarget; + if (difference > 0) { + //If we removed more than we have per target, we need to remove the excess from our remainder + remainder -= difference; + } + return; + } toSplitAmong--; //Only recalculate it if it is not willing to accept/doesn't want the // full per side split if (amountNeeded != amountPerTarget && toSplitAmong != 0) { long amountPerLast = amountPerTarget; amountPerTarget = amountToSplit / toSplitAmong; + remainder = amountToSplit % toSplitAmong; if (!amountPerChanged && amountPerTarget != amountPerLast) { amountPerChanged = true; } @@ -36,10 +48,22 @@ public Long getShareAmount() { @Override public Long getRemainderAmount() { - //Add to the remainder amount the entire remainder so that we try to use it up if we can - // The remainder then if it cannot be fully accepted slowly shrinks across each target we are distributing to - //TODO: Evaluate making a more even distribution of the remainder - return toSplitAmong == 0 ? amountPerTarget : amountPerTarget + (amountToSplit % toSplitAmong); + if (toSplitAmong != 0 && remainder > 0) { + //If we have a remainder, be willing to provide a single unit as the remainder + // so that we split the remainder more evenly across the targets. + return amountPerTarget + 1; + } + return amountPerTarget; + } + + @Override + public Long getUnsent() { + return remainder; + } + + @Override + public boolean isZero(Long value) { + return value == 0; } @Override diff --git a/src/main/java/mekanism/common/lib/distribution/SplitInfo.java b/src/main/java/mekanism/common/lib/distribution/SplitInfo.java index c2ca0893ad7..faa97d3dd6b 100644 --- a/src/main/java/mekanism/common/lib/distribution/SplitInfo.java +++ b/src/main/java/mekanism/common/lib/distribution/SplitInfo.java @@ -2,18 +2,63 @@ public abstract class SplitInfo> { + /** + * Number of targets to split the contents among. + */ protected int toSplitAmong; + /** + * Represents whether the amount per target distribution has changed. This may happen if a target doesn't need as much as we are willing to offer it in the split. + */ public boolean amountPerChanged = false; + /** + * Determines whether the number of targets to split amount should be decreased. + * + * @implNote This is only set to false briefly when handling accepting contents with remainders to allow them to accept some of the contents without being marked as + * fully accounted for. + */ + protected boolean decrementTargets = true; protected SplitInfo(int totalTargets) { this.toSplitAmong = totalTargets; } + /** + * Marks the given amount as being accounted for and "sent". Decrements {@link #getUnsent() how much we have left to send} and increments + * {@link #getTotalSent() how much we have sent}. If {@link #decrementTargets} is true, this also will reduce the number of targets to split among, and recalculate + * how much we can provide each target. + * + * @param amountNeeded Amount needed by the target and that we are accounting as having been sent to the target. + */ public abstract void send(TYPE amountNeeded); + /** + * {@return the "share" each target should get when distributing in an even split} + */ public abstract TYPE getShareAmount(); + /** + * Gets the "share" including a potential remainder that targets should get when handling remainders. This is used for actually sending providing the split share to + * any targets that can accept more than we are able to offer in an even split. In general this number will either be equal to {@link #getShareAmount()} or greater + * than it by one while we still have an excess remainder. + * + * @return the "share" plus any potential remainder. + */ public abstract TYPE getRemainderAmount(); + /** + * {@return the amount of contents that has not been sent anywhere yet} + */ + public abstract TYPE getUnsent(); + + /** + * {@return true if the value is equal to zero} + * + * @param value Value to check + */ + public abstract boolean isZero(TYPE value); + + /** + * {@return the total amount of contents that have been sent} + */ public abstract TYPE getTotalSent(); } \ No newline at end of file diff --git a/src/main/java/mekanism/common/lib/distribution/Target.java b/src/main/java/mekanism/common/lib/distribution/Target.java index 4b7b05202bd..fbb864d8862 100644 --- a/src/main/java/mekanism/common/lib/distribution/Target.java +++ b/src/main/java/mekanism/common/lib/distribution/Target.java @@ -61,8 +61,43 @@ public int getHandlerCount() { */ public void sendRemainingSplit(SplitInfo splitInfo) { //If needed is not empty then we default it to the given calculated fair split amount of remaining energy - for (HandlerType recipient : needed) { - acceptAmount(recipient.handler(), splitInfo, splitInfo.getRemainderAmount()); + if (!needed.isEmpty() && !splitInfo.isZero(splitInfo.getRemainderAmount())) { + Iterator> iterator = needed.iterator(); + while (iterator.hasNext()) { + TYPE remainderAmount = splitInfo.getRemainderAmount(); + if (splitInfo.isZero(remainderAmount)) { + //We finished inserting everything we wanted to, we can just exit + return; + } + HandlerType needInfo = iterator.next(); + //Accept the remaining amount + TYPE amountNeeded = needInfo.amount(); + if (amountNeeded.compareTo(remainderAmount) <= 0) { + acceptAmount(needInfo.handler(), splitInfo, amountNeeded); + //If the amount we needed was the less than or the same as our remaining amount + // we can remove the value as it has now been sent + iterator.remove(); + } else { + splitInfo.decrementTargets = false; + acceptAmount(needInfo.handler(), splitInfo, remainderAmount); + splitInfo.decrementTargets = true; + } + } + //TODO: If we remove buffers maybe we should evaluate not caring if we don't actually send the full excess remainder? + // Given ideally we wouldn't attempting to insert the excess remainder to handlers as a second call to the handler on the same tick + if (!splitInfo.isZero(splitInfo.getUnsent())) { + //If we still have some of a remainder after trying to evenly distribute the remainder just send it to the first target willing to accept it + // This might happen if one of the destinations was only able to accept part of the remaining amount, though in general that case will be + // covered by shifting the needed values + for (HandlerType recipient : needed) { + TYPE remaining = splitInfo.getUnsent(); + if (splitInfo.isZero(remaining)) { + //We finished, exit + return; + } + acceptAmount(recipient.handler(), splitInfo, remaining); + } + } } } @@ -96,14 +131,27 @@ public void sendRemainingSplit(SplitInfo splitInfo) { * @param splitInfo Information about current overall split. */ public void sendPossible(EXTRA toSend, SplitInfo splitInfo) { - for (HANDLER entry : handlers) { - TYPE amountNeeded = simulate(entry, toSend); - if (amountNeeded.compareTo(splitInfo.getShareAmount()) <= 0) { - //Add the amount, in case something changed from simulation only mark actual sent amount - // in split info - acceptAmount(entry, splitInfo, amountNeeded); - } else { - needed.add(new HandlerType<>(entry, amountNeeded)); + if (splitInfo.isZero(splitInfo.getShareAmount())) { + //We are all remainder, just calculate how much each can accept + for (HANDLER entry : handlers) { + TYPE amountNeeded = simulate(entry, toSend); + if (!splitInfo.isZero(amountNeeded)) { + needed.add(new HandlerType<>(entry, amountNeeded)); + } + } + } else { + for (HANDLER entry : handlers) { + TYPE amountNeeded = simulate(entry, toSend); + if (amountNeeded.compareTo(splitInfo.getShareAmount()) <= 0) { + //Add the amount, in case something changed from simulation only mark actual sent amount + // in split info + if (!splitInfo.isZero(amountNeeded)) { + //Note: We can skip actually running it if it doesn't need anything + acceptAmount(entry, splitInfo, amountNeeded); + } + } else { + needed.add(new HandlerType<>(entry, amountNeeded)); + } } } } @@ -114,6 +162,9 @@ public void sendPossible(EXTRA toSend, SplitInfo splitInfo) { * @param splitInfo The new split to (re)check. */ public void shiftNeeded(SplitInfo splitInfo) { + if (splitInfo.isZero(splitInfo.getShareAmount())) { + return; + } Iterator> iterator = needed.iterator(); //Use an iterator rather than a copy of the keySet of the needed subMap // This allows for us to remove it once we find it without having to diff --git a/src/test/java/mekanism/common/lib/distribution/DistributionTest.java b/src/test/java/mekanism/common/lib/distribution/DistributionTest.java index 76b792fbb8e..cb77914bb8c 100644 --- a/src/test/java/mekanism/common/lib/distribution/DistributionTest.java +++ b/src/test/java/mekanism/common/lib/distribution/DistributionTest.java @@ -4,6 +4,7 @@ import java.util.function.Supplier; import mekanism.common.lib.distribution.handler.InfiniteIntegerHandler; import mekanism.common.lib.distribution.handler.IntegerHandler; +import mekanism.common.lib.distribution.handler.LyingAmountIntegerHandler; import mekanism.common.lib.distribution.handler.PartialIntegerHandler; import mekanism.common.lib.distribution.handler.SpecificAmountIntegerHandler; import mekanism.common.lib.distribution.target.IntegerTarget; @@ -35,6 +36,20 @@ void testEvenDistribution() { int toSend = 10; IntegerTarget availableAcceptors = getTargets(toSend, 0, 0); Assertions.assertEquals(toSend, EmitUtils.sendToAcceptors(availableAcceptors, toSend, toSend)); + for (IntegerHandler handler : availableAcceptors.handlers) { + Assertions.assertEquals(1, handler.getAccepted()); + } + } + + @Test + @DisplayName("Test sending to targets where the amounts divide evenly with more than 1 each") + void testEvenDistribution2() { + int toSend = 40; + IntegerTarget availableAcceptors = getTargets(toSend / 4, 0, 0); + Assertions.assertEquals(toSend, EmitUtils.sendToAcceptors(availableAcceptors, toSend, toSend)); + for (IntegerHandler handler : availableAcceptors.handlers) { + Assertions.assertEquals(4, handler.getAccepted()); + } } @Test @@ -43,6 +58,17 @@ void testRemainderDistribution() { int toSend = 10; IntegerTarget availableAcceptors = getTargets(7, 0, 0); Assertions.assertEquals(toSend, EmitUtils.sendToAcceptors(availableAcceptors, toSend, toSend)); + int singleAccepted = 0, twoAccepted = 0; + for (IntegerHandler handler : availableAcceptors.handlers) { + Assertions.assertTrue(handler.getAccepted() == 1 || handler.getAccepted() == 2); + if (handler.getAccepted() == 1) { + singleAccepted++; + } else { + twoAccepted++; + } + } + Assertions.assertEquals(4, singleAccepted); + Assertions.assertEquals(3, twoAccepted); } @Test @@ -51,10 +77,18 @@ void testAllRemainder() { int toSend = 3; IntegerTarget availableAcceptors = getTargets(7, 0, 0); Assertions.assertEquals(toSend, EmitUtils.sendToAcceptors(availableAcceptors, toSend, toSend)); - //TODO: Make it so that we try to send this more evenly initially before falling back to send to remainders? - /*for (IntegerTarget availableAcceptor : availableAcceptors) { - System.out.println("Amount accepted: " + availableAcceptor.getAccepted()); - }*/ + int destinationsAccepted = 0; + int destinationsNotAccepted = 0; + for (IntegerHandler availableAcceptor : availableAcceptors.handlers) { + if (availableAcceptor.getAccepted() > 0) { + destinationsAccepted++; + Assertions.assertEquals(1, availableAcceptor.getAccepted()); + } else { + destinationsNotAccepted++; + } + } + Assertions.assertEquals(3, destinationsAccepted); + Assertions.assertEquals(4, destinationsNotAccepted); } @Test @@ -65,12 +99,39 @@ void testCorrectRemainder() { //First one can accept exactly one //total to send -> 4, to split among -> 2, to send -> 2 (remainder none) IntegerTarget availableAcceptors = new IntegerTarget(); - IntegerHandler handler = new SpecificAmountIntegerHandler(1); - availableAcceptors.addHandler(handler); + availableAcceptors.addHandler(new SpecificAmountIntegerHandler(1)); addTargets(availableAcceptors, () -> new SpecificAmountIntegerHandler(3), 2); int sent = EmitUtils.sendToAcceptors(availableAcceptors, toSend, toSend); if (sent > toSend) { Assertions.fail(String.format(Locale.ROOT, "expected: <%s> to be greater or equal to: <%s>", toSend, sent)); } } + + @Test + @DisplayName("Test to check if the remainder is able to be sent when having to fall back") + void testCorrectFallbackRemainder() { + int toSend = 9; + IntegerTarget availableAcceptors = new IntegerTarget(); + IntegerHandler specificHandler = new SpecificAmountIntegerHandler(8); + IntegerHandler lyingHandler = new LyingAmountIntegerHandler(1, 10); + availableAcceptors.addHandler(specificHandler); + availableAcceptors.addHandler(lyingHandler); + Assertions.assertEquals(toSend, EmitUtils.sendToAcceptors(availableAcceptors, toSend, toSend)); + Assertions.assertEquals(1, lyingHandler.getAccepted()); + Assertions.assertEquals(8, specificHandler.getAccepted()); + } + + @Test + @DisplayName("Test to check if the remainder is able to be sent when having to fall back using the reversed order for the handlers") + void testCorrectFallbackRemainderAltOrder() { + int toSend = 9; + IntegerTarget availableAcceptors = new IntegerTarget(); + IntegerHandler specificHandler = new SpecificAmountIntegerHandler(8); + IntegerHandler lyingHandler = new LyingAmountIntegerHandler(1, 10); + availableAcceptors.addHandler(lyingHandler); + availableAcceptors.addHandler(specificHandler); + Assertions.assertEquals(toSend, EmitUtils.sendToAcceptors(availableAcceptors, toSend, toSend)); + Assertions.assertEquals(1, lyingHandler.getAccepted()); + Assertions.assertEquals(8, specificHandler.getAccepted()); + } } \ No newline at end of file diff --git a/src/test/java/mekanism/common/lib/distribution/handler/LyingAmountIntegerHandler.java b/src/test/java/mekanism/common/lib/distribution/handler/LyingAmountIntegerHandler.java new file mode 100644 index 00000000000..70b901adff6 --- /dev/null +++ b/src/test/java/mekanism/common/lib/distribution/handler/LyingAmountIntegerHandler.java @@ -0,0 +1,25 @@ +package mekanism.common.lib.distribution.handler; + +/** + * Lies about how much it can actually accept when simulating. This is to simulate if multiple handlers end up affecting the same backing tank. As then during simulation + * it will have reported it could accept more than we actually can. + */ +public class LyingAmountIntegerHandler extends SpecificAmountIntegerHandler { + + private final int amountToLieBy; + + public LyingAmountIntegerHandler(int toAccept, int amountToLieBy) { + super(toAccept); + this.amountToLieBy = amountToLieBy; + } + + @Override + public int perform(int amountOffered, boolean isSimulate) { + int canAccept = super.perform(amountOffered, isSimulate); + if (isSimulate) { + //If we are simulating, "lie" and say we can accept more than we actually have room for + return Math.min(amountOffered, canAccept + amountToLieBy); + } + return canAccept; + } +} \ No newline at end of file