Upon preroll, destroy ranged ingredient.#5051
Conversation
…IllegalCallerException. TODO: replace this with something less destructive.
jurrejelle
left a comment
There was a problem hiding this comment.
I'm not sure I like .replace() as a name, maybe could be replaced with something like .collapse() or the like, since replace usually gets used in stuff like string.replace("a","b"). Apart from that LGTM
| syncDataHolder.markClientSyncFieldDirty("lastDisplayedRecipe"); | ||
| markLastRecipeDirty(); | ||
| } | ||
| recipe.doTickPrerolls(this.chanceCaches, this.lastDisplayedRecipe); |
There was a problem hiding this comment.
this taking lastDisplayedRecipe is a bit iffy since the name implies it's used for displaying, but that might have to wait until 9.0.0 recipelogic refactor
screret
left a comment
There was a problem hiding this comment.
Mostly some formatting nitpicks, code itself looks fine to me.
P.S. Maybe rename the PR to "Destroy ranged ingredients when prerolling" - I think that'd convey what it does a bit better
| FluidIngredient recipeStack = FluidRecipeCapability.CAP.of(fluidInputs.get(i).content()); | ||
| if (!recipeStack.test(fluidStack) || (recipeStack.getAmount()) > fluidStack.getAmount()) { | ||
| if (!recipeStack.test(fluidStack) || | ||
| (recipeStack instanceof IRangedIngredient ? ((IRangedIngredient) recipeStack).getMaxRoll() : |
There was a problem hiding this comment.
| (recipeStack instanceof IRangedIngredient ? ((IRangedIngredient) recipeStack).getMaxRoll() : | |
| (recipeStack instanceof IRangedIngredient ranged ? ranged.getMaxRoll() : |
| content = new Content(ranged.collapse(), content.chance(), content.maxChance()); | ||
| ranged.reset(); | ||
| } | ||
| tickInputs.computeIfAbsent(capability, c -> new ArrayList<>()).add(content); |
There was a problem hiding this comment.
| tickInputs.computeIfAbsent(capability, c -> new ArrayList<>()).add(content); | |
| this.tickInputs.computeIfAbsent(capability, c -> new ArrayList<>()).add(content); |
| content = new Content(ranged.collapse(), content.chance(), content.maxChance()); | ||
| ranged.reset(); | ||
| } | ||
| tickOutputs.computeIfAbsent(capability, c -> new ArrayList<>()).add(content); |
There was a problem hiding this comment.
| tickOutputs.computeIfAbsent(capability, c -> new ArrayList<>()).add(content); | |
| this.tickOutputs.computeIfAbsent(capability, c -> new ArrayList<>()).add(content); |
| ranged.rollSampledCount(); | ||
| for (var handler : inputs.values()) { | ||
| for (var iterator = handler.listIterator(); iterator.hasNext();) { | ||
| var content = iterator.next(); |
There was a problem hiding this comment.
Less var, please
| var content = iterator.next(); | |
| for (List<Content> input : this.inputs.values()) { | |
| for (ListIterator<Content> iterator = input.listIterator(); iterator.hasNext();) { | |
| Content content = iterator.next(); |
| for (var handler : outputs.values()) { | ||
| for (var iterator = handler.listIterator(); iterator.hasNext();) { | ||
| var content = iterator.next(); |
There was a problem hiding this comment.
| for (var handler : outputs.values()) { | |
| for (var iterator = handler.listIterator(); iterator.hasNext();) { | |
| var content = iterator.next(); | |
| for (List<Content> output : outputs.values()) { | |
| for (ListIterator<Content> iterator = output.listIterator(); iterator.hasNext();) { | |
| Content content = iterator.next(); |
|
|
||
| int amount = ing.getAmount(); | ||
| int amount; | ||
| if (ing instanceof IRangedIngredient provider) amount = provider.getMaxRoll(); |
There was a problem hiding this comment.
| if (ing instanceof IRangedIngredient provider) amount = provider.getMaxRoll(); | |
| if (ing instanceof IRangedIngredient ranged) amount = ranged.getMaxRoll(); |
| @Override | ||
| public long kjs$getAmount() { | ||
| return ingredient.getAmount(); | ||
| return (ingredient instanceof IRangedIngredient ? ((IRangedIngredient) ingredient).getMaxRoll() : |
There was a problem hiding this comment.
| return (ingredient instanceof IRangedIngredient ? ((IRangedIngredient) ingredient).getMaxRoll() : | |
| return (ingredient instanceof IRangedIngredient ranged ? ranged.getMaxRoll() : |
| .map(FluidRecipeCapability.CAP::of) | ||
| .filter(i -> !i.isEmpty()) | ||
| .mapToInt(FluidIngredient::getAmount) | ||
| .mapToInt((i -> i instanceof IRangedIngredient ? ((IRangedIngredient) i).getMaxRoll() : i.getAmount())) |
There was a problem hiding this comment.
| .mapToInt((i -> i instanceof IRangedIngredient ? ((IRangedIngredient) i).getMaxRoll() : i.getAmount())) | |
| .mapToInt((i -> i instanceof IRangedIngredient ranged ? ranged.getMaxRoll() : i.getAmount())) |
| // This will print a "Cannot get stacks" warning to the log. Ignore it. | ||
| helper.assertTrue(ingredient.getStacks().length == 0, "A ranged fluid ingredient " + | ||
| "should not return items!"); | ||
| ingredient.rollSampledCount(); | ||
| var stacks = ingredient.collapse().getStacks(); | ||
| helper.assertTrue(stacks.length == 1, "Replaced IntProviderFluidIngredient should only " + | ||
| "return 1 fluid when made with 1 fluid"); | ||
| helper.assertTrue(stacks[0].isFluidEqual(GTMaterials.Water.getFluid(1)), | ||
| "IntProviderFluidIngredient should have fluid equal to what it was made with"); | ||
| helper.assertTrue(stacks[0].isFluidStackIdentical(ingredient.getStacks()[0]), | ||
| "IntProviderFluidIngredient.getStacks shouldn't change between getStacks calls"); | ||
| ingredient.reset(); | ||
| helper.assertFalse(stacks[0].isFluidStackIdentical(ingredient.getStacks()[0]), | ||
| "IntProviderFluidIngredient.getStacks should have changed after rerolling"); | ||
| "Replaced IntProviderFluidIngredient should have fluid equal to what it was made with"); |
There was a problem hiding this comment.
Add some newlines here, for spacing
| // This will print a "Cannot get stacks" warning to the log. Ignore it. | |
| helper.assertTrue(ingredient.getStacks().length == 0, "A ranged fluid ingredient " + | |
| "should not return items!"); | |
| ingredient.rollSampledCount(); | |
| var stacks = ingredient.collapse().getStacks(); | |
| helper.assertTrue(stacks.length == 1, "Replaced IntProviderFluidIngredient should only " + | |
| "return 1 fluid when made with 1 fluid"); | |
| helper.assertTrue(stacks[0].isFluidEqual(GTMaterials.Water.getFluid(1)), | |
| "IntProviderFluidIngredient should have fluid equal to what it was made with"); | |
| helper.assertTrue(stacks[0].isFluidStackIdentical(ingredient.getStacks()[0]), | |
| "IntProviderFluidIngredient.getStacks shouldn't change between getStacks calls"); | |
| ingredient.reset(); | |
| helper.assertFalse(stacks[0].isFluidStackIdentical(ingredient.getStacks()[0]), | |
| "IntProviderFluidIngredient.getStacks should have changed after rerolling"); | |
| "Replaced IntProviderFluidIngredient should have fluid equal to what it was made with"); | |
| // This will print a "Cannot get stacks" warning to the log. Ignore it. | |
| helper.assertTrue(ingredient.getStacks().length == 0, | |
| "A ranged fluid ingredient should not return items!"); | |
| ingredient.rollSampledCount(); | |
| var stacks = ingredient.collapse().getStacks(); | |
| helper.assertTrue(stacks.length == 1, | |
| "Replaced IntProviderFluidIngredient should only return 1 fluid when made with 1 fluid"); | |
| helper.assertTrue(stacks[0].isFluidEqual(GTMaterials.Water.getFluid(1)), | |
| "Replaced IntProviderFluidIngredient should have fluid equal to what it was made with"); | |
| // This will print a "Cannot get items" warning to the log. Ignore it. | ||
| helper.assertTrue(ingredient.getItems().length == 0, "A ranged ingredient " + | ||
| "should not return items!"); | ||
| ingredient.rollSampledCount(); | ||
| var stacks = ingredient.collapse().getItems(); | ||
| helper.assertTrue(stacks.length == 1, "Replaced IntProviderIngredient should only " + | ||
| "return 1 item when made with 1 item"); | ||
| helper.assertTrue(stacks[0].is(new ItemStack(Items.BRICK, 1).getItem()), | ||
| "IntProviderIngredient should have item equal to what it was made with"); | ||
| helper.assertTrue(TestUtils.areItemStacksEqual(stacks, ingredient.getItems()), | ||
| "IntProviderIngredient.getItems shouldn't change between getStacks calls"); | ||
| ingredient.reset(); | ||
| helper.assertFalse(TestUtils.areItemStacksEqual(stacks, ingredient.getItems()), | ||
| "IntProviderIngredient.getItems should have changed after rerolling"); | ||
| "Replaced IntProviderIngredient should have item equal to what it was made with"); |
There was a problem hiding this comment.
More spacing here too
| // This will print a "Cannot get items" warning to the log. Ignore it. | |
| helper.assertTrue(ingredient.getItems().length == 0, "A ranged ingredient " + | |
| "should not return items!"); | |
| ingredient.rollSampledCount(); | |
| var stacks = ingredient.collapse().getItems(); | |
| helper.assertTrue(stacks.length == 1, "Replaced IntProviderIngredient should only " + | |
| "return 1 item when made with 1 item"); | |
| helper.assertTrue(stacks[0].is(new ItemStack(Items.BRICK, 1).getItem()), | |
| "IntProviderIngredient should have item equal to what it was made with"); | |
| helper.assertTrue(TestUtils.areItemStacksEqual(stacks, ingredient.getItems()), | |
| "IntProviderIngredient.getItems shouldn't change between getStacks calls"); | |
| ingredient.reset(); | |
| helper.assertFalse(TestUtils.areItemStacksEqual(stacks, ingredient.getItems()), | |
| "IntProviderIngredient.getItems should have changed after rerolling"); | |
| "Replaced IntProviderIngredient should have item equal to what it was made with"); | |
| // This will print a "Cannot get items" warning to the log. Ignore it. | |
| helper.assertTrue(ingredient.getItems().length == 0, | |
| "A ranged ingredient should not return items!"); | |
| ingredient.rollSampledCount(); | |
| var stacks = ingredient.collapse().getItems(); | |
| helper.assertTrue(stacks.length == 1, | |
| "Replaced IntProviderIngredient should only return 1 item when made with 1 item"); | |
| helper.assertTrue(stacks[0].is(new ItemStack(Items.BRICK, 1).getItem()), | |
| "Replaced IntProviderIngredient should have item equal to what it was made with"); | |
What
Big enough structural change that it deserves its own PR into the PR branch.
Upon prerolling a Ranged Ingredient, destroy it and replace it with a Sized Ingredient of the rolled size.
Implementation Details
I realized this was a necessity after trying to fix #5048 to account for NeoForge's versions of SizedIngredient and SizedFluidIngredient.
AI Usage
Outcome
The version of a recipe a machine actually runs will now never contain a Ranged Ingredient. This hugely simplifies the logic needed in
Notifiable[X]Handler.handleRecipe().How Was This Tested
Automated tests pass after being edited to account for it.
Additional Information
Four tests (ranged ingredient sabotage; ranged ingredients json) have been deleted as (1) there isn't a way to do those tests in this new system and (2) the behavior those tests were designed to catch for cannot exist in the new system either.
This is being done as an explicit PR into #5015 to verify it, and will then be replicated as a part of #5048.
Currently logged as a draft because, while it passes tests, I want to do a clean-up pass to remove any cases where something tries to get amount for of a ranged ingredient and really shouldn't.
Potential Compatibility Issues
If anything this resolves potential compatibility issues.