mirrors/CC-Tweaked
1
0
Fork 0
mirror of https://github.com/SquidDev-CC/CC-Tweaked synced 2024-09-28 15:08:47 +00:00
Code Issues Releases Wiki Activity

Correctly obey stack limits in OffsetStorage.extract/insert

Browse Source 
Many thanks to Lem for managing to reproduce it. It was actually an easy
bug bug to spot on second look, but having a reliable way to verify was
super helpful.

Fixes #1338
This commit is contained in:
Jonathan Coates 2023-03-15 20:07:17 +00:00
parent e96ac35d67
commit 44f945c040
No known key found for this signature in database
GPG Key ID: B9E431FF07C98D06
5 changed files with 48 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
gradle/libs.versions.toml
View File

@ -42,7 +42,7 @@ jqwik = "1.7.2"
junit = "5.9.2" junit = "5.9.2"
# Build tools # Build tools
cctJavadoc = "1.6.0" cctJavadoc = "1.6.1"
checkstyle = "10.3.4" checkstyle = "10.3.4"
curseForgeGradle = "1.0.11" curseForgeGradle = "1.0.11"
errorProne-core = "2.18.0" errorProne-core = "2.18.0"

4
projects/common/src/test/java/dan200/computercraft/shared/util/InventoryUtilTest.java
View File

@ -22,7 +22,7 @@ public class InventoryUtilTest {
var remainder = InventoryUtil.storeItemsFromOffset(container, new ItemStack(Items.COBBLESTONE, 32), 4); var remainder = InventoryUtil.storeItemsFromOffset(container, new ItemStack(Items.COBBLESTONE, 32), 4);
assertThat("Remainder is empty", remainder, isStack(ItemStack.EMPTY)); assertThat("Remainder is empty", remainder, isStack(ItemStack.EMPTY));
assertThat("Was inserted into slot", container.getItem(4), isStack(new ItemStack(Items.COBBLESTONE, 32))); assertThat("Was inserted into slot", container.getItem(4), isStack(Items.COBBLESTONE, 32));
} }
@Test @Test
@ -33,6 +33,6 @@ public class InventoryUtilTest {
var remainder = InventoryUtil.storeItemsFromOffset(container, new ItemStack(Items.COBBLESTONE, 32), 4); var remainder = InventoryUtil.storeItemsFromOffset(container, new ItemStack(Items.COBBLESTONE, 32), 4);
assertThat("Remainder is empty", remainder, isStack(ItemStack.EMPTY)); assertThat("Remainder is empty", remainder, isStack(ItemStack.EMPTY));
assertThat("Was inserted into slot", container.getItem(1), isStack(new ItemStack(Items.COBBLESTONE, 32))); assertThat("Was inserted into slot", container.getItem(1), isStack(Items.COBBLESTONE, 32));
} }
} }

5
projects/common/src/testFixtures/java/dan200/computercraft/test/shared/ItemStackMatcher.java
View File

@ -5,6 +5,7 @@
*/ */
package dan200.computercraft.test.shared; package dan200.computercraft.test.shared;
import net.minecraft.world.item.Item;
import net.minecraft.world.item.ItemStack; import net.minecraft.world.item.ItemStack;
import org.hamcrest.Description; import org.hamcrest.Description;
import org.hamcrest.Matcher; import org.hamcrest.Matcher;
@ -30,4 +31,8 @@ public class ItemStackMatcher extends TypeSafeMatcher<ItemStack> {
public static Matcher<ItemStack> isStack(ItemStack stack) { public static Matcher<ItemStack> isStack(ItemStack stack) {
return new ItemStackMatcher(stack); return new ItemStackMatcher(stack);
} }
public static Matcher<ItemStack> isStack(Item item, int size) {
return new ItemStackMatcher(new ItemStack(item, size));
}
} }

46
projects/common/src/testFixtures/java/dan200/computercraft/test/shared/platform/ContainerTransferContract.java
View File

@ -11,6 +11,8 @@ import net.minecraft.world.SimpleContainer;
import net.minecraft.world.item.ItemStack; import net.minecraft.world.item.ItemStack;
import net.minecraft.world.item.Items; import net.minecraft.world.item.Items;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import java.util.Collections; import java.util.Collections;
import java.util.IdentityHashMap; import java.util.IdentityHashMap;
@ -34,7 +36,7 @@ public interface ContainerTransferContract {
var move = wrap(inv).singleSlot(0).moveTo(wrap(inv).singleSlot(0), 64); var move = wrap(inv).singleSlot(0).moveTo(wrap(inv).singleSlot(0), 64);
assertEquals(ContainerTransfer.NO_SPACE, move); assertEquals(ContainerTransfer.NO_SPACE, move);
assertThat(inv.getItem(0), isStack(new ItemStack(Items.DIRT, 64))); assertThat(inv.getItem(0), isStack(Items.DIRT, 64));
assertNoOverlap(inv); assertNoOverlap(inv);
} }
@ -48,7 +50,7 @@ public interface ContainerTransferContract {
assertEquals(64, move); assertEquals(64, move);
assertThat(inv.getItem(0), isStack(ItemStack.EMPTY)); assertThat(inv.getItem(0), isStack(ItemStack.EMPTY));
assertThat(inv.getItem(1), isStack(new ItemStack(Items.DIRT, 64))); assertThat(inv.getItem(1), isStack(Items.DIRT, 64));
assertNoOverlap(inv); assertNoOverlap(inv);
} }
@ -64,7 +66,7 @@ public interface ContainerTransferContract {
assertEquals(64, move); assertEquals(64, move);
assertThat(source.getItem(0), isStack(ItemStack.EMPTY)); assertThat(source.getItem(0), isStack(ItemStack.EMPTY));
assertThat(destination.getItem(0), isStack(new ItemStack(Items.DIRT, 64))); assertThat(destination.getItem(0), isStack(Items.DIRT, 64));
assertNoOverlap(source, destination); assertNoOverlap(source, destination);
} }
@ -85,12 +87,40 @@ public interface ContainerTransferContract {
assertThat(source.getItem(0), isStack(ItemStack.EMPTY)); assertThat(source.getItem(0), isStack(ItemStack.EMPTY));
for (var i = 0; i < 4; i++) { for (var i = 0; i < 4; i++) {
assertThat("Stack in slot " + i, destination.getItem(i), isStack(new ItemStack(Items.DIRT, 64))); assertThat("Stack in slot " + i, destination.getItem(i), isStack(Items.DIRT, 64));
} }
assertNoOverlap(source, destination); assertNoOverlap(source, destination);
} }
@ParameterizedTest(name = "offset = {0}")
@ValueSource(ints = { 0, 1, 2 })
default void testMoveDistributeWithLimit(int offset) {
// We create a specific setup here such that each slot has room for strictly less than the limit(=17).
// There was a bug (https://github.com/cc-tweaked/CC-Tweaked/issues/1338) where this would insert items beyond
// the limit as it did not account for the number of items already transferred.
var destination = new SimpleContainer(4);
destination.setItem(offset, new ItemStack(Items.DIRT, 48));
destination.setItem((offset + 1) % 4, new ItemStack(Items.DIRT, 48));
var source = new SimpleContainer(4);
source.setItem(0, new ItemStack(Items.DIRT, 64));
source.setItem(1, new ItemStack(Items.DIRT, 64));
var move = wrap(source).moveTo(wrap(destination).rotate(offset), 17);
assertEquals(17, move);
assertThat("Source stack in slot 0", source.getItem(0), isStack(Items.DIRT, 47));
assertThat("Source stack in slot 1", source.getItem(1), isStack(Items.DIRT, 64));
assertThat("Dest stack in slot 0", destination.getItem(offset), isStack(Items.DIRT, 64));
assertThat("Dest stack in slot 2", destination.getItem((offset + 1) % 4), isStack(Items.DIRT, 49));
assertThat("Dest stack in slot 3", destination.getItem((offset + 2) % 4), isStack(ItemStack.EMPTY));
assertThat("Dest stack in slot 3", destination.getItem((offset + 3) % 4), isStack(ItemStack.EMPTY));
assertNoOverlap(source, destination);
}
@Test @Test
default void testMoveSkipFullSlot() { default void testMoveSkipFullSlot() {
var destination = new SimpleContainer(4); var destination = new SimpleContainer(4);
@ -103,8 +133,8 @@ public interface ContainerTransferContract {
assertEquals(64, move); assertEquals(64, move);
assertThat(source.getItem(1), isStack(ItemStack.EMPTY)); assertThat(source.getItem(1), isStack(ItemStack.EMPTY));
assertThat(destination.getItem(0), isStack(new ItemStack(Items.DIRT, 64))); assertThat(destination.getItem(0), isStack(Items.DIRT, 64));
assertThat(destination.getItem(1), isStack(new ItemStack(Items.DIRT, 64))); assertThat(destination.getItem(1), isStack(Items.DIRT, 64));
assertNoOverlap(source, destination); assertNoOverlap(source, destination);
} }
@ -124,7 +154,7 @@ public interface ContainerTransferContract {
var move = wrap(source).moveTo(wrap(destination), 64); var move = wrap(source).moveTo(wrap(destination), 64);
assertEquals(ContainerTransfer.NO_SPACE, move); assertEquals(ContainerTransfer.NO_SPACE, move);
assertThat(source.getItem(0), isStack(new ItemStack(Items.DIRT, 64))); assertThat(source.getItem(0), isStack(Items.DIRT, 64));
assertThat(destination.getItem(0), isStack(ItemStack.EMPTY)); assertThat(destination.getItem(0), isStack(ItemStack.EMPTY));
assertNoOverlap(source, destination); assertNoOverlap(source, destination);
@ -143,7 +173,7 @@ public interface ContainerTransferContract {
assertEquals(32, move); assertEquals(32, move);
assertThat("Source is empty", source.getItem(0), isStack(ItemStack.EMPTY)); assertThat("Source is empty", source.getItem(0), isStack(ItemStack.EMPTY));
assertThat("Was inserted into slot", destination.getItem(1), isStack(new ItemStack(Items.COBBLESTONE, 32))); assertThat("Was inserted into slot", destination.getItem(1), isStack(Items.COBBLESTONE, 32));
} }
static void assertNoOverlap(Container... containers) { static void assertNoOverlap(Container... containers) {

4
projects/fabric/src/main/java/dan200/computercraft/shared/platform/FabricContainerTransfer.java
View File

@ -111,7 +111,7 @@ public class FabricContainerTransfer implements ContainerTransfer {
long transferred = 0; long transferred = 0;
for (var i = 0; i < size; i++) { for (var i = 0; i < size; i++) {
transferred += slots.get(wrap(i, size)).insert(resource, maxAmount, transaction); transferred += slots.get(wrap(i, size)).insert(resource, maxAmount - transferred, transaction);
if (transferred >= maxAmount) break; if (transferred >= maxAmount) break;
} }
@ -125,7 +125,7 @@ public class FabricContainerTransfer implements ContainerTransfer {
long transferred = 0; long transferred = 0;
for (var i = 0; i < size; i++) { for (var i = 0; i < size; i++) {
transferred += slots.get(wrap(i, size)).extract(resource, maxAmount, transaction); transferred += slots.get(wrap(i, size)).extract(resource, maxAmount - transferred, transaction);
if (transferred >= maxAmount) break; if (transferred >= maxAmount) break;
} }