From c173636bddc28cf1c624c42dfb2ce5f0aefb7878 Mon Sep 17 00:00:00 2001 From: raoulvdberge Date: Thu, 26 Dec 2024 20:49:10 +0100 Subject: [PATCH] fix: resources not being used up correctly when there are missing items This now always makes it so that output resources are being added to the internal storage. If we do not do this, when there are missing resources, and we have the same ingredient being used multiple times, it would start from scratch for each of that same ingredient, causing a calculation that needs too many resources. So we have to *always* return resources so they can be reused later down the line, even if there are missing resources. However, we must use these resources up correctly if we do end up with missing resources! Otherwise, if another ingredient needs this resource later down the calculation, it will use them when it's not allowed to (the yields of the child calculation must be used up...) --- .../calculation/CraftingState.java | 6 +++ .../calculation/CraftingTree.java | 41 +++++++++------ .../CraftingCalculatorImplTest.java | 52 +++++++++++++++++++ .../common/autocrafting/CraftingPattern.java | 2 +- 4 files changed, 83 insertions(+), 18 deletions(-) diff --git a/refinedstorage-autocrafting-api/src/main/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingState.java b/refinedstorage-autocrafting-api/src/main/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingState.java index 6e287893b..d782d7662 100644 --- a/refinedstorage-autocrafting-api/src/main/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingState.java +++ b/refinedstorage-autocrafting-api/src/main/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingState.java @@ -32,6 +32,12 @@ void addOutputsToInternalStorage(final Pattern pattern, final Amount amount) { ); } + void removeOutputsFromInternalStorage(final Pattern pattern, final Amount amount) { + pattern.getOutputs().forEach( + output -> internalStorage.remove(output.resource(), output.amount() * amount.iterations()) + ); + } + CraftingState copy() { return new CraftingState(storage.copy(), internalStorage.copy()); } diff --git a/refinedstorage-autocrafting-api/src/main/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingTree.java b/refinedstorage-autocrafting-api/src/main/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingTree.java index eb6d2b5b5..256acdbc0 100644 --- a/refinedstorage-autocrafting-api/src/main/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingTree.java +++ b/refinedstorage-autocrafting-api/src/main/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingTree.java @@ -59,9 +59,7 @@ CalculationResult calculate() { result = CalculationResult.MISSING_RESOURCES; } } - if (result == CalculationResult.SUCCESS) { - craftingState.addOutputsToInternalStorage(pattern, amount); - } + craftingState.addOutputsToInternalStorage(pattern, amount); return result; } @@ -81,9 +79,21 @@ private CalculationResult calculateIngredient(final IngredientState ingredientSt remaining -= toTake; } if (remaining > 0) { - resourceState = tryCalculateChild(ingredientState, resourceState, remaining); - if (resourceState == null) { + final CraftingState.ResourceState newState = tryCalculateChild( + ingredientState, + resourceState, + remaining + ); + if (newState == null) { + // If we end up with missing resources, we need to use up all the resulting + // resources from the internal storage so that it cannot be used later for other ingredients + // that happen to use the same resource. + // The goal was to use up the resources created by the child calculation in the next iteration, + // but since we have missing resources, we need to use them up now. + craftingState.extractFromInternalStorage(resourceState.resource(), remaining); return CalculationResult.MISSING_RESOURCES; + } else { + resourceState = newState; } } } @@ -116,14 +126,14 @@ private CraftingState.ResourceState calculateChild(final IngredientState ingredi final CraftingState.ResourceState resourceState) { final ChildCalculationResult result = calculateChild(remaining, childPatterns, resourceState); if (result.success) { - this.craftingState = result.childCraftingState; + this.craftingState = result.childTree.craftingState; final CraftingState.ResourceState updatedResourceState = craftingState.getResource( resourceState.resource() ); listener.childCalculationCompleted( updatedResourceState.resource(), updatedResourceState.inInternalStorage(), - result.childListener + result.childTree.listener ); return updatedResourceState; } @@ -153,28 +163,26 @@ private ChildCalculationResult calculateChild(final long remaining, return new ChildCalculationResult<>( true, craftingState.getResource(resourceState.resource()).inInternalStorage(), - childTree.listener, - childTree.craftingState + childTree ); } return new ChildCalculationResult<>( false, requireNonNull(lastChildAmount).getTotal(), - requireNonNull(lastChildTree).listener, - lastChildTree.craftingState + requireNonNull(lastChildTree) ); } @Nullable private CraftingState.ResourceState cycleToNextIngredientOrFail(final IngredientState ingredientState, final CraftingState.ResourceState resourceState, - final ChildCalculationResult result) { + final ChildCalculationResult childResult) { return ingredientState.cycle().map(craftingState::getResource).orElseGet(() -> { - this.craftingState = result.childCraftingState; + this.craftingState = childResult.childTree.craftingState; listener.childCalculationCompleted( resourceState.resource(), - result.amountCrafted, - result.childListener + childResult.amountCrafted, + childResult.childTree.listener ); return null; }); @@ -182,8 +190,7 @@ private CraftingState.ResourceState cycleToNextIngredientOrFail(final Ingredient private record ChildCalculationResult(boolean success, long amountCrafted, - CraftingCalculatorListener childListener, - CraftingState childCraftingState) { + CraftingTree childTree) { } enum CalculationResult { diff --git a/refinedstorage-autocrafting-api/src/test/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingCalculatorImplTest.java b/refinedstorage-autocrafting-api/src/test/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingCalculatorImplTest.java index 918b882dd..01db24c68 100644 --- a/refinedstorage-autocrafting-api/src/test/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingCalculatorImplTest.java +++ b/refinedstorage-autocrafting-api/src/test/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingCalculatorImplTest.java @@ -97,6 +97,58 @@ void shouldCalculateForSingleRootPatternSingleIngredientAndAllResourcesAreAvaila .build()); } + @ParameterizedTest + @ValueSource(longs = {1, 2}) + void shouldCalculateForSingleRootPatternSingleIngredientSpreadOutOverMultipleIngredientsAndThereAreMissingResources( + final long requestedAmount + ) { + // Arrange + final RootStorage storage = storage(); + final PatternRepository patterns = patterns( + pattern() + .ingredient(OAK_LOG, 1) + .output(OAK_PLANKS, 4) + .build(), + pattern() + // problem is that, if oak logs are missing, + // the planks for ingredient 1 will be never added to the available result set + // but it would add toCraft Logs=1 + // then for ingredient 2, since planks are not found, + // it would add toCraft logs=1 (so tocraft logs=2 = INCORRECT!). + // BUT ... if we make planks available to the result set so you don't have this problem + // (at that point it can reuse planks) + // the calculation might make a wrong assumptions and reuse UNEXISTING planks for .. for example + // sticks in the sign recipe..... + // so in some cases we do *need* the planks to be available for the calculation + // but in others this cannot be! + // Now, is the real problem here that one item is spread over multiple ingredients? + // or is the problem that the calculation is not able to handle this? + // it does make a bit of sense though, the current behavior, + // a new ingredient creates a disjoint in the possibility tree... + // THIS ISSUE DOES NOT OCCUR IF: + // *there are enough resources OR + // *the resources are not spread out over multiple ingredients (try ingredient(OAK_PLANKS,4) for example) + .ingredient(OAK_PLANKS, 1) + .ingredient(OAK_PLANKS, 1) + .ingredient(OAK_PLANKS, 1) + .ingredient(OAK_PLANKS, 1) + .output(CRAFTING_TABLE, 1) + .build() + ); + final CraftingCalculator sut = new CraftingCalculatorImpl(patterns, storage); + + // Act + final Preview preview = calculateAndGetPreview(sut, CRAFTING_TABLE, requestedAmount); + + // Assert + assertThat(preview).usingRecursiveComparison(PREVIEW_CONFIG).isEqualTo(PreviewBuilder.ofType(MISSING_RESOURCES) + .addToCraft(CRAFTING_TABLE, requestedAmount) + .addToCraft(OAK_PLANKS, requestedAmount * 4) + .addMissing(OAK_LOG, requestedAmount) + .build()); + } + + @Test void shouldNotCalculateForSingleRootPatternSingleIngredientAndAlmostAllResourcesAreAvailable() { // Arrange diff --git a/refinedstorage-common/src/main/java/com/refinedmods/refinedstorage/common/autocrafting/CraftingPattern.java b/refinedstorage-common/src/main/java/com/refinedmods/refinedstorage/common/autocrafting/CraftingPattern.java index 0e5c42426..d2da98a6d 100644 --- a/refinedstorage-common/src/main/java/com/refinedmods/refinedstorage/common/autocrafting/CraftingPattern.java +++ b/refinedstorage-common/src/main/java/com/refinedmods/refinedstorage/common/autocrafting/CraftingPattern.java @@ -29,7 +29,7 @@ class CraftingPattern implements Pattern { this.output = output; this.inputResources = inputs.stream().flatMap(List::stream).collect(Collectors.toSet()); this.outputResources = Set.of(output.resource()); - this.ingredients = inputs.stream().map(i -> new Ingredient(1, i)).toList(); + this.ingredients = inputs.stream().map(i -> new Ingredient(i.isEmpty() ? 0 : 1, i)).toList(); this.outputs = Stream.concat(Stream.of(output), byproducts.stream()).toList(); }