Skip to content

Commit

Permalink
fix: resources not being used up correctly when there are missing items
Browse files Browse the repository at this point in the history
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...)
  • Loading branch information
raoulvdberge committed Dec 26, 2024
1 parent 36bbd76 commit c173636
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,7 @@ CalculationResult calculate() {
result = CalculationResult.MISSING_RESOURCES;
}
}
if (result == CalculationResult.SUCCESS) {
craftingState.addOutputsToInternalStorage(pattern, amount);
}
craftingState.addOutputsToInternalStorage(pattern, amount);
return result;
}

Expand All @@ -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;
}
}
}
Expand Down Expand Up @@ -116,14 +126,14 @@ private CraftingState.ResourceState calculateChild(final IngredientState ingredi
final CraftingState.ResourceState resourceState) {
final ChildCalculationResult<T> 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;
}
Expand Down Expand Up @@ -153,37 +163,34 @@ private ChildCalculationResult<T> 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<T> result) {
final ChildCalculationResult<T> 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;
});
}

private record ChildCalculationResult<T>(boolean success,
long amountCrafted,
CraftingCalculatorListener<T> childListener,
CraftingState childCraftingState) {
CraftingTree<T> childTree) {
}

enum CalculationResult {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Check failure on line 130 in refinedstorage-autocrafting-api/src/test/java/com/refinedmods/refinedstorage/api/autocrafting/calculation/CraftingCalculatorImplTest.java

View workflow job for this annotation

GitHub Actions / build / build

Line is longer than 120 characters (found 121).
.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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down

0 comments on commit c173636

Please sign in to comment.