Make recipe placing from recipe book working with custom ingredients

[MrHell228]

No description provided.

[aromaa]

This is a lot of Mixins to achieve one goal and is hard to justify to be this invasive. Can we simply this by modifying the second parameter of the Inventory#findSlotMatchingCraftingIngredient inside the ServerPlaceRecipe#moveItemToGrid?

[MrHell228]

Most of the injections in ServerPlaceRecipeMixin are simply maintaining the list of applicable ItemStacks, which in the end is used exactly for ServerPlaceRecipe#moveItemToGrid and Inventory#findSlotMatchingCraftingIngredient (for their modified versions in RecipeBookUtil, to be more accurate).

I used as much vanilla logic as I could, and no custom logic is performed if recipe is regular, so I'm not sure what does "this invasive" mean.

[aromaa]

We don't want to duplicate vanilla code in our code base unless we really have to, its too fragile and heavily increases maintenance burden and iteration times, that is a no go.

If we can cut this down to few mixins instead of having a dozen, that would be a lot better.

[MrHell228]

I wouldn't copy these 2 vanilla methods if I could really avoid it, it's just the bare minimum to make it work (as far as I figured out).

Vanilla recipe placing logic is heavily bounded to Items, and assuming Ingredient#test can not even depend on Item (but on ItemStack data, for example), I honestly don't see the way to reduce the amount if injections here.

[aromaa]

I'm suggesting that we do the final filtering in ServerPlaceRecipe#moveItemToGrid.

[MrHell228]

You mean something like this? It does reduce duplicated code to just one simple method

[aromaa]

I mean that we literally do all the filtering in just the ServerPlaceRecipe#moveItemToGrid. It means that we need to find the items again, but thats small price to pay compared to the amount of code we would have to roll.

[MrHell228]

Wouldn't it remove like... 3 Injects and 1 ModifyArg and add the requirement to duplicate vanilla code that filters the items?

[aromaa]

I'm expecting us to nuke 90% of this PR with just that change, am I missing something? Do we need to do anything else than just filter the items?

[MrHell228]

While decomposing all the steps I understood that RecipeBridge and relevant mixins are unnecessary because it can simply be done on PlacementInfo. And I think PlacementInfoMixin will exist anyway because of the PR that fixes shapeless recipes.

About everything else:

We won't be able to capture items without duplicating if we don't use SpongeStackedItemContents where it's used now.
That's unnecessary (and might be costly) for regular recipes so that's why PlacementInfoBridge#hasCustomIngredients exists.
And if we don't capture items there, StackedItemContents#canCraft (that is used 3 times) will literally return random booleans which will make placing for some sponge recipes simply not working at all

3 Injects and ModifyArg that maintain applicable ItemStack list are simply following the vanilla List<Holder<Item>> behaviour.
If we don't maintain the list here, we would need to duplicate... I guess this code from ServerPlaceRecipe#placeRecipe?

if ($$1.canCraft(var10001, $$6, $$7::add)) {
  int $$8 = clampToMaxStackSize($$6, $$7);
  if ($$8 != $$6) {
    $$7.clear();
    var10001 = $$0.value();
    Objects.requireNonNull($$7);
    if (!$$1.canCraft(var10001, $$8, $$7::add)) {
      return;
    }
  }
...

Because of the $$7::add that accumulates mentioned list.
And I feel like few injects are better than copying all of this.

RecipeBookUtil#findSlotMatchingCraftingIngredient seems to remain anyway because I don't see how vanilla method could be used there.

So... I don't understand how you suppose to reduce the PR by this much.

[aromaa]

Yes, I fully expect us to throw away the List<Holder<Item>> for custom recipes. We would be pulling the actual ItemStacks inside the ServerPlayerRecipe#moveItemToGrid from a field. The easiest thing I see is to:

• Override the methods with StackedContents<Holder<Item>> calls in SpongeStackedItemContents to StackedContents<ItemStack> (Don't duplicate the vanilla logic).
• Redirect the output to say, ArrayDequeue (Give the SpongeStackedItemContents reference to it so you are good to go).
• Poll that from ServerPlayerRecipe#moveItemToGrid.

[MrHell228]

2nd and 3rd points done. Didn't exactly understand what you meant in 1st (beacuse I think it's already done).
Managed to reduce the amount of injections to 4

[MrHell228]

Fixed all the reviewed things

[aromaa]

This still needs to account for the non empty craftInputStack. My initial suggestion was to just use it instead when its non empty, but now that I thought about it more, it actually just pulls more wrong items inside the slot. I think we should just bail when !ItemStack.isSameItemSameComponents(craftInputStack, this.impl$stackList.poll()) because theres nothing more we can do.

We should hit here when the players inventory is full and they have more stacks in the crafting input than they can fit in their inventory.

I think the following works as intended:

if (this.impl$stackList.isEmpty()) {
    return original.call(...);
} else {
    final ItemStack input = his.impl$stackList.poll();
    if (!ItemStack.isSameItemSameComponents(craftInputStack, input)) {
        return -1:
    }
    return original.call(..., input);
}

[MrHell228]

I think the condition to fail should be !craftInputStack.isEmpty() && !ItemStack.isSameItemSameComponents(craftInputStack, input), or we won't be able to move items to empty crafting slots

[MrHell228]

Made tests, found a bug in mixin and a bug in vanilla.
It tests different layouts for all placeable recipes (with regular only and custom only ingredients).
I think testing not only for recipe match but also for the expected layout (for the amount of placed items) is necessary because it's actually the place where bugs can happen (found bugs mentioned above and there were other bugs related to this while doing this PR).
Test for custom shapeless recipe is temporarily commented out unless they are fixed (tested it on different branch with recipe fix, it works too).
I was doing tests for the first time so I hope it's not that bad but at least everything works.

[aromaa]

Let's not cram in this much parameters in to one single method, its quite hard to tell whats going on. I would suggest fluent API.

These tests are going to need to have more identifiable names so you can look at the test output and Ctrl + F the source code.

Something along the lines:

test("big_pearl_recipe", ...)
    .success("Matching input", ...)
    .fail("Missing pearl", ...)
    .fail("Has stone", ...)
    .fail("Asked where the bathroom is", ...)

[MrHell228]

Replaced method with a lot of parameters with TestPopulator.
As an identifiable name for Ctrl+F I suggest TestContext#asTestName.
Updated everything else as well.

[aromaa]

This looks a lot better now! Something I didn't happen to mention explicitly and just implicitly in the code snippet is that ideally you should be able to have dynamic number of successful and failure cases.

Given this is a bit of complex surface area as ingredients and their permutations can be quite large, it would make sense to have this written more flexibly to potentially add more edge cases or programmatically generate them in the future.

[MrHell228]

Something like this? Edge cases could be added via TestPopulator#test, those tests are applied before "match" test and thus can cancel it if needed.

[aromaa]

Nothing wrong with the actual TestEntry being fixed, but rather that the inputs like the provided inventories and their expected outputs should be easily definable so you can give any number of inventories that should success or fail.

So more like record TestEntry(boolean shiftClick, int clicks, List<ItemStack> inventory, List<ItemStack> input, List<ItemStack> expectedInputs)

This makes all the partialInventory and badInventory obsolete and they can be defined using the same method. Nothing wrong with providing additional helpers to create those test cases but it makes this a lot more straightforward from consumer stand point and when people are glancing at them and wondering where and why did things go work.

[aromaa]

Isn't it.. already doable? WIth something like

Ah, right, that's actually totally fine!

Whats actually throwing me off is the fact you have crammed in the badInventory and the partialInventory in the same class, forcing you to define one successive and two failing cases at once. We should just generate one case for single entry. If you can just split them off to their own individual cases, like following the code snipped you just gave me, then that's fine! Feel free to use helper method to generate them.

Its better for tests to be verbose and expressive as they are meant to be easily understood.

[MrHell228]

Moved tests for bad layouts (which involved badInventory) to separate TestPopulator.

[aromaa]

Looks like theres still some partialInventory and partialInput and badInput leftover in the DefaultedTestPopulator

[MrHell228]

I just don't understand what you want me to do honestly. I changed it again but at this point I'm not even sure if this is what I had to change

[aromaa]

Feel free to ask before you change anything. I understand that this is a bit new to you, and I'm honestly giving you a bit too vague feedback because I'm leaving gaps and just pointing to a direction and expecting some existing knowledge, that's on me.

The general gist of unit tests are that they should be very simple and not overly complex and easy to follow (and setup) and have most (if not all) relevant information on few lines. The general problem is that the shape was overly complex, you had to understand the program flow, had multiple inputs and tests with variety of different situations with different inputs (not very obvious) crammed in together.

It's important that test code is easy to understand so people don't have to spend a whole lot of time just to see what the test is doing. Its common to see these files to be very long and have repetitive copy & paste with just few changes for this exact reason.

So instead of having something like:

test(recipe).inventory(...).input(...).badInventory(...).badInput(...)

its more natural to write something like:

test(recipe).inventory(...).input(...).state(success)
test(recipe).inventory(...).input(...).state(success)
test(recipe).inventory(...).input(...).state(fail)
test(recipe).inventory(...).input(...).state(fail)
test(recipe).inventory(...).input(...).state(fail)

Now the test cases are obvious and their inputs and their expected outcomes can be read and understood in a single line without having to look around. It's also very easy to understand how to add new tests cases.

That said, yes, the badLayoutTests method is something I prefer. Its direct and obvious and hard to miss.

[aromaa]

These should be directly asked for by generating the tests with the the appropriate badInventory and badInput.