Generalize recipes and implement them by Limeth · Pull Request #1119 · SpongePowered/Sponge

Limeth · 2017-01-03T17:27:56Z

This is a WIP continuation of kashike's PR, with his consent.
It aims to generalize how recipes are registered and handled. Instead of checking the equality of the ingredients, it works by testing ingredient predicates. It also aims to let the user implement the recipe interfaces by themselves, making it, for example, possible to transfer NBT data from the ingredients to the result.
In this PR, ItemStackSnapshots are used extensively. This is to prevent the user from modifying the original ItemStack. I could instead provide an ItemStack#copy, but that doesn't make it visually obvious the original item won't change.

Crafting recipes

All recipes stored in CraftingManager must implement both IRecipe and CraftingRecipe.

Vanilla, mods:
delegation to native

IRecipe -- MixinIRecipe implements CraftingRecipe
ShapedRecipes -- MixinShapedRecipes implements ShapedCraftingRecipe
ShapelessRecipes -- MixinShapelessRecipes implements ShapelessCraftingRecipe

Sponge:
delegation to sponge by overriding both native & sponge

CraftingRecipe -- wrap in SpongeCustomCraftingRecipe? Is it possible to mix-in IRecipe? Feedback needed.
ShapedCraftingRecipe -- extends ShapedRecipes
ShapelessCraftingRecipe -- extends ShapelessRecipes

Zidane · 2017-01-03T19:30:24Z

+/**
+ * Delegates all NMS methods to the abstract methods
+ */
+public abstract class AbstractSpongeShapedCraftingRecipe extends ShapedRecipes implements ShapedCraftingRecipe {


Nope.

Mix ShapedCraftingRecipe into ShapedRecipes. Otherwise, mod recipes won't be API recipes (and neither will Mojang ones...)

ST-DDT · 2017-01-07T12:29:37Z

+    private final ShapedCraftingRecipe delegate;
+
+    public DelegateShapedCraftingRecipe(ShapedCraftingRecipe delegate) {
+        Preconditions.checkNotNull(delegate, "The delegate must not be null");


This method is usually statically imported:
`checkNotNull(delegate, "delegate");

ST-DDT · 2017-01-07T12:31:51Z

+                return false;
+            }
+
+            if (recipeNMS.getMetadata() != 32767 && recipeNMS.getMetadata() != inventoryNMS.getMetadata())


What is 32767 for a magic number here?
Either create a constant with a meaningful name or add a comment.

I don't unfortunately know what exactly it is.

kashike · 2017-01-09T12:10:14Z

+                                .map(itemStack -> itemStack.getItem() == ItemTypes.NONE)
+                                .orElse(true);
+
+                        if (!empty)


Missing brackets.

kashike · 2017-01-09T12:10:22Z

+                                        finalAisleX + "; " + finalAisleY + "] is not present. There should be" +
+                                        " one for each symbol in the aisle!"));
+
+                        if (!ingredientPredicate.test(itemStack.createSnapshot()))


Missing brackets.

kashike · 2017-01-09T12:10:58Z

+        Preconditions.checkNotNull(mainItem);
+
+        this.mainItem = mainItem;
+        this.remainingItems = remainingItems == null ? ImmutableList.of() : ImmutableList.copyOf(remainingItems);


Mark the parameter @Nullable if it is to be null.

kashike · 2017-01-09T12:11:09Z

+
+    @Override
+    public ItemStackSnapshot getMainItem() {
+        return mainItem;


kashike · 2017-01-09T12:11:13Z

+
+    @Override
+    public List<ItemStackSnapshot> getRemainingItems() {
+        return remainingItems;


kashike · 2017-01-09T12:12:23Z

+
+    @Override
+    public int getHeight() {
+        return recipeHeight;


kashike · 2017-01-09T12:12:26Z

+
+    @Override
+    public Optional<Predicate<ItemStackSnapshot>> getIngredientPredicate(int x, int y) {
+        return Optional.ofNullable(ingredients.get(x, y));


kashike · 2017-01-09T12:12:48Z

+        checkState(!this.aisle.isEmpty(), "aisle must be set before setting aisle symbols");
+
+        if (ingredient != null) {
+            ingredientMap.put(symbol, ingredient);


kashike · 2017-01-09T12:13:18Z

+
+    @Final
+    @Shadow
+    protected int recipeWidth;


squash it onto one line:
@Final @Shadow protected int recipeWidth

kashike · 2017-01-09T12:13:33Z

+    private net.minecraft.item.ItemStack[] recipeItems;
+
+    public Optional<Predicate<ItemStackSnapshot>> sponge$getIngredientPredicate(int x, int y) {
+        if(x < 0 || x >= recipeWidth || y < 0 || y >= recipeHeight)


Missing brackets; space if (

Limeth · 2017-01-09T20:58:31Z

I am currently kind of stuck. I can't quite figure out how to fix this mixin error I'm getting. I thought that maybe I had the hierarchy wrong and tried making MixinShapedRecipes implement MixinIRecipe, but that creates another error.

As a side question, does anyone know if there is an easy way to achieve this?

Maxqia · 2017-01-12T04:24:57Z

^ and that sir, is why you don't copy names from classes you're trying to mixin :P

ST-DDT · 2017-01-13T19:27:07Z

@Limeth You asked for help? How can I help you?

Limeth · 2017-01-13T20:32:47Z

I think I may have figured out the conversion between native and Sponge inventories, yay. See the latest commit.

Limeth · 2017-01-13T20:48:18Z

@ST-DDT Thanks for the response! The errors seem to have magically disappeared when I switched machines and I think I've just made breakthrough step in the implementation of this PR, so I don't need help for now. I'm sure I'll get stuck again very soon, though! :D

Limeth · 2017-01-14T20:06:05Z

Crafting recipes are working now. \o/

gabizou

Minor style changes.

gabizou · 2017-01-15T17:43:50Z

    public abstract Slot getSlot(int slotId);

+    @Shadow
+    public ItemStack slotClick(int slotId, int dragType, ClickType clickTypeIn, EntityPlayer player) {


If the method is public, you can mark it as abstract and avoid the actual method body.

Also, we prefer that the @Shadow annotations are on the same line as the method (unlike @Override).

I would, but then I wouldn't be able to call it from here.

Ah yes, good point.

gabizou · 2017-01-15T17:44:53Z

+})
+public abstract class MixinContainerCrafting extends MixinContainer {
+
+    private final Container this$ = (Container) (Object) this;


Just inline this field, this otherwise is ugly.

gabizou · 2017-01-15T17:49:36Z

+
+    @SuppressWarnings("unchecked")
+    private List<CraftingRecipe> spongeRecipes() {
+        return (List<CraftingRecipe>) (List<?>) recipes;


Why is there a level of indirection? It doesn't make sense unless there's something I'm missing.

You have getRecipes which is an override, and this spongeRecipes, which is something else? You can safely just eliminate this method entirely.

gabizou · 2017-01-15T17:49:59Z

+    boolean matches(InventoryCrafting inv, net.minecraft.world.World worldIn);
+
+    @Shadow
+    net.minecraft.item.ItemStack getCraftingResult(InventoryCrafting inv);


Same line, and preferably keep these without spaces at the top of the mixin.

gabizou · 2017-01-15T17:51:24Z

+import java.util.stream.Collectors;
+
+@Mixin(IRecipe.class)
+public interface MixinIRecipe extends CraftingRecipe {


What you may want to do is annotate the class with @Implements(iface = @Interface(CraftingRecipe.class), prefix = "", unique = true)

I had it like that before, but ended up making the interface extend CraftingRecipe directly. Why should I put it back?

the unique bits, it ensures that the methods won't overlap vanilla methods, but you can have both extending and @Implements. Right, @Mumfrey?

[22:18:47] [main/ERROR] [mixin]: mixins.common.core.json:item.recipe.crafting.MixinIRecipe: Prefix for iface Lorg/spongepowered/api/item/recipe/crafting/CraftingRecipe; is not valid

@gabizou ^ that's what is printed to the console when I use @Implements, I guess I should remove it?

gabizou · 2017-01-15T17:52:53Z

    @Override
-    public RecipeRegistry getRecipeRegistry() {
+    public SmeltingRecipeRegistry getSmeltingRecipeRegistry() {
        throw new UnsupportedOperationException(); // TODO


Does this PR not implement the smelting recipe registry?

Not yet. I can either start implementing the smelting recipe registry in this PR, or make another one when this is one is merged.

simon816 · 2017-01-15T20:48:24Z

+
+    @Nonnull
+    public static Optional<ItemStack> getContainerItem(@Nonnull ItemStack itemStack) {
+        Preconditions.checkNotNull(itemStack, "The itemStack must not be null");


methods from Preconditions should be statically imported.

simon816 · 2017-01-15T20:50:21Z

@@ -36,7 +36,7 @@
 // TODO


This class was a stub. you should either implement it or remove if not needed

Somehow forgot to remove it.

simon816 · 2017-01-15T20:51:32Z

+
+    @Override
+    public ItemStackSnapshot getExemplaryResult() {
+        return recipe.getExemplaryResult();


Field accesses should be qualified with this. consistently throughout the codebase

Protip: most IDEs can do this automatically

simon816 · 2017-01-15T20:54:58Z

+        net.minecraft.item.ItemStack inventoryNMS = ItemStackUtil.fromSnapshotToNative(inventoryStack);
+
+        if (!recipeNMS.isEmpty() || !inventoryNMS.isEmpty())
+        {


incorrect formatting, should have brace on previous line
(I know this is copied from vanilla code but formatting should be consistent with sponge's code)

Limeth · 2017-01-16T21:40:41Z

Smelting recipes are now working too. \o/

gabizou

Code cleanup. Lots of mixed coding style, not sure if it's because it's from multiple authors or different IDE setups, but needs to match the current coding standards.

gabizou · 2017-01-17T06:34:10Z

+
+    // Crafting
+
+    @Nonnull


This is already included in the package level annotations for parameters, methods, and fields. Don't clutter it up in the implementation.

It's giving me warnings when it's not there. ¯_(ツ)_/¯

You have to tell your IDE what the NonnullByDefault annotation is, look it up for your ide.

gabizou · 2017-01-17T06:35:05Z

+        checkNotNull(itemStack, "The itemStack must not be null");
+
+        net.minecraft.item.ItemStack nmsStack = ItemStackUtil.toNative(itemStack);
+        Item nmsItem = nmsStack.getItem();


Could probably do some short circuit to ItemStack.isValid() which was added in 1.11.

I can't seem to find that method.

Sorry, isEmpty().

gabizou · 2017-01-17T06:35:16Z

    }

    @Override
+    @Nonnull


No, more clutter.

gabizou · 2017-01-17T06:36:54Z

+import java.util.function.Supplier;
+
+public abstract class AbstractSpongeCraftingRecipe implements CraftingRecipe, IRecipe {
+    @Override


Spacing here, group static methods together versus mixed in with instance methods. Or, you can mark the groups with comments.

gabizou · 2017-01-17T06:37:55Z

+        return matches(this::isValid, inv, worldIn);
+    }
+
+    public static boolean matches(BiFunction<GridInventory, World, Boolean> isValid, InventoryCrafting inv, net.minecraft.world.World worldIn) {


Do recipes require World instances? I'd imagine that we'd only ever want to deal with WorldServer instances (not client based worlds).

Yes, see CraftingManager#getRemainingItems(InventoryCrafting, World).

gabizou · 2017-01-17T07:07:34Z

+
+    @Override
+    @Nonnull
+    public Optional<Predicate<ItemStackSnapshot>> getIngredientPredicate(int x, int y) {


Why is this even an Optional? Sounds like bad use to be honest. If anything, it can just be any or none predicate.

gabizou · 2017-01-17T07:08:28Z

+    private final List<SmeltingRecipe> recipes = Lists.newArrayList();
+
+    /**
+     * @author Limeth


Need more than this. Needs a date, needs some @reason explanation.

gabizou · 2017-01-17T07:08:41Z

+     */
+    @Overwrite
+    public void addSmeltingRecipe(ItemStack input, ItemStack stack, float experience)
+    {


The coding style is awkward here.

gabizou · 2017-01-17T07:09:09Z

+        Predicate<ItemStackSnapshot> ingredientPredicate = new MatchSmeltingVanillaItemStack(exemplaryIngredient);
+
+        this.recipes.add(new SpongeSmeltingRecipe(exemplaryResult,
+                exemplaryIngredient, ingredientPredicate, experience));


Why the awkward line break?

gabizou · 2017-01-17T07:09:29Z

 import org.spongepowered.api.event.cause.entity.damage.DamageType;
-import org.spongepowered.api.event.cause.entity.damage.source.*;
-import org.spongepowered.api.event.cause.entity.spawn.*;
+import org.spongepowered.api.event.cause.entity.damage.source.BlockDamageSource;


Don't touch star imports from things that you're not touching.

Deamon5550 · 2017-01-17T07:38:42Z

+
+    @Override
+    public int getRecipeSize() {
+        return getRecipeSize(this::getSize);


Consistency with other static methods in this class; see usage in SpongeShapedCraftingRecipe.

wat? I'm still very confused as to why this is necessary? seems like a massive hack/anti-pattern.

SpongeShapedCraftingRecipe must extend ShapedRecipes, but cannot extend two classes at the same time, so methods in AbstractSpongeCraftingRecipe are declared statically and are called from within SpongeShapedCraftingRecipe.

Move them to an interface?

or just duplicate the implementations, anything but this.

The pattern is just IoC but that simple fact is being occluded by the actual method of implementation. I had the same problem with Adapter but solved it in a slightly clearer way of abstracting the logic away to an inner class Adapter.Logic and then any Trait mixins call back against the static methods therein.

Honestly for consistency a similar approach should be used here. Instead of static methods in a base class, wrap the static methods in a Logic or Operations class and call back against it. The mixins should also probably be Trait in this case to denote their usage.

I can't quite figure out what you mean by the Trait and can't quite picture how the end result would look, would you please link me the code you wrote?

Deamon5550 · 2017-01-17T08:01:13Z

+    /**
+     * @author Limeth
+     */
+    @Overwrite


All these overwrites make me twitchy. You should atleast maintain the old functionality of the class (ie. still put things into the maps) or you're going to break mods.

Limeth · 2017-01-18T19:05:05Z

@Deamon5550 I've rewritten the MixinFurnaceRecipes class to make it mod-friendly.

Cybermaxke · 2017-01-18T21:19:58Z

-     */
-    @Overwrite
-    public float getSmeltingExperience(ItemStack stack) {
+    @Inject(method = "getSmeltingExperience", at = @At("RETURN"))


Shouldn't this be also set to cancellable?

It's using setReturnValue so yes, it needs to be cancellable or it will throw an exception if that code path is followed.

ST-DDT · 2017-01-18T21:07:00Z

+    }
+
+    public static MatchCraftingVanillaItemStack never() {
+        return new MatchCraftingVanillaItemStack();


IMO it would be better if you just return/use a simple predicate -> false here.
You could even cache the instance, if it is needed at all. (See the other comment)

ST-DDT · 2017-01-18T21:11:36Z

+    @Override
+    public Predicate<ItemStackSnapshot> getIngredientPredicate(int x, int y) {
+        if (x < 0 || x >= recipeWidth || y < 0 || y >= recipeHeight) {
+            return MatchCraftingVanillaItemStack.never();


I would rather throw an IndexOutOfBoundsException here.

ST-DDT · 2017-01-18T21:20:56Z

+     * @return Whether the stacks match according to the vanilla Minecraft
+     *         behavior
+     */
+    public static boolean matchesVanillaItemStack(ItemStackSnapshot recipeStack, ItemStackSnapshot inventoryStack) {


Suggestion: We should probably expose this somewhere in the API. (Maybe in the CraftingRecipeRegistry or ISS?)
Either as method or as static field storing a Function<ISS,Predicate<ISS>>.

That way we allow some default matching combined with .and(someCostumConditions).

What do you think about it? Or shall we do that later?

The behavior is specific to crafting recipes, smelting recipes use a different method, so I think it's a good idea to expose it in the CraftingRecipeRegistry and do the same for smelting recipes.

I'm not fond of just a "catch all" matching for ItemStacks since there can be various "this is allowed" matches, including things where it just matches the ItemType, ItemType and quantity, ItemType and NBT, all of the above, etc.

What do you propose?

Limeth · 2017-01-25T13:06:55Z

Some feedback would be welcome.

kashike

Please reformat to follow our codestyle. You are missing this. everywhere, braces missing/on wrong lines, etc.
Additionally, be sure to checkNotNull parameters to give a nicer NullPointerException message.

kashike · 2017-01-25T14:04:03Z

+        Iterator<IInventory> inventories = fabric.allInventories().iterator();
+        InventoryCrafting inventoryCrafting = (InventoryCrafting) inventories.next();
+
+        if(inventories.hasNext())


if (
missing braces

kashike · 2017-01-25T14:04:47Z

+        ItemStack nativeIngredient = ItemStackUtil.fromSnapshotToNative(ingredient);
+
+        for (Map.Entry<ItemStack, ItemStack> entry : this.smeltingList.entrySet())
+        {


Wrong formatting.

kashike · 2017-01-25T14:05:15Z

+
+    @Override
+    public int getWidth() {
+        return recipeWidth;


kashike · 2017-01-25T14:05:17Z

+
+    @Override
+    public int getHeight() {
+        return recipeHeight;


kashike · 2017-01-25T14:05:22Z

+    @Override
+    @Nonnull
+    public List<Predicate<ItemStackSnapshot>> getIngredientPredicates() {
+        return recipeItems.stream()


kashike · 2017-01-25T14:08:53Z

+    public MatchSmeltingVanillaItemStack(ItemStackSnapshot itemStackSnapshot) {
+        checkNotNull(itemStackSnapshot, "The itemStackSnapshot must not be null");
+
+        this.itemStackSnapshot = itemStackSnapshot;


this.itemStackSnapshot = checkNotNull(itemStackSnapshot, "The itemStackSnapshot must not be null");

kashike · 2017-01-25T14:09:23Z

+
+    @Override
+    public int getSize() {
+        return ingredients.size();


kashike · 2017-01-25T14:09:44Z

+
+    @Override
+    public List<ItemStackSnapshot> getRemainingItems(GridInventory grid) {
+        return StreamSupport.stream(grid.<Slot>slots().spliterator(), false)


checkNotNull(grid, "grid")

kashike · 2017-01-25T14:09:55Z

+
+    @Override
+    public ItemStackSnapshot getExemplaryResult() {
+        return exemplaryResult;


kashike · 2017-01-25T14:10:00Z

+
+    @Override
+    public List<Predicate<ItemStackSnapshot>> getIngredientPredicates() {
+        return ingredients;


Limeth · 2017-01-25T15:34:29Z

Changes made as suggested, waiting for more feedback.

Limeth · 2017-02-06T07:38:02Z

This PR is not dead! I am determined to get it merged. :D

Aaron1011 · 2017-02-06T18:23:53Z

@Limeth: Can you rebase this against bleeding?

Limeth · 2017-02-07T21:20:12Z

Rebased against bleeding.

Aaron1011 · 2017-02-07T21:28:33Z

@Limeth: There still appear to be merge conflicts

Limeth · 2017-02-08T14:37:41Z

Conflicting commits appeared within that hour. Rebased again, should be fine now.

Limeth · 2017-02-13T07:32:07Z

@bloodmc @gabizou @Zidane Thoughts?

gabizou

Multiple changes needed, mostly style, but there's too much noise at the moment.

gabizou · 2017-02-15T08:31:24Z

+            ItemStack containerStack = ItemStackUtil.fromNative(nmsContainerStack);
+
+            return Optional.of(containerStack);
+        } else {


Useless else statement.

gabizou · 2017-02-15T08:32:04Z

+        checkNotNull(world, "world");
+
+        return Sponge.getRegistry().getCraftingRecipeRegistry()
+                .findMatchingRecipe(this.craftingGrid, world);


Awkwardly separated line.

gabizou · 2017-02-15T08:32:12Z

    @Override
    protected void init(SlotProvider<IInventory, ItemStack> slots) {
        super.init(slots, false);
+


I like to separate variable assignments and method calls with a space.

That's fine, but you shouldn't do this here because you haven't changed anything else in this class.

gabizou · 2017-02-15T08:35:12Z

+     * @return Whether the stacks match according to the vanilla Minecraft
+     *         behavior
+     */
+    public static boolean matchesVanillaItemStack(ItemStackSnapshot recipeStack, ItemStackSnapshot inventoryStack) {


I'm not fond of just a "catch all" matching for ItemStacks since there can be various "this is allowed" matches, including things where it just matches the ItemType, ItemType and quantity, ItemType and NBT, all of the above, etc.

gabizou · 2017-02-15T08:37:19Z

+    @Override
+    public ShapedCraftingRecipe.Builder where(char symbol, Predicate<ItemStackSnapshot> ingredient) throws IllegalArgumentException {
+        checkState(!this.aisle.isEmpty(), "aisle must be set before setting aisle symbols");
+


You have a lot of extra new lines that just add empty space, and it's getting distracting/excessive. Condense where possible.

Should I just remove all empty lines? How can I tell which ones you want preserved? Removing this one in particular goes against my skin, because not separating a code block with a new line looks confusing. I'd like to point out that imo I've spent too much time fixing code style by reading the reviews, as most of them have been about it. It's quite annoying having to wait for the reviews and finding out it's just another one about code style. It would be much easier for me if I had a clear understandable guide on what code style you prefer.

Perhaps it is: https://docs.spongepowered.org/stable/en/contributing/implementation/codestyle.html

Thanks @ziceptor, I must've missed that. The question stays, though: Which empty lines are distracting? Which should I keep/remove?

I'm commenting where I'm finding useless new lines for you to get an idea.

gabizou · 2017-02-15T08:38:24Z

+
+        checkState(width > 0, "The aisle cannot be empty.");
+
+        do {


This is a very weird loop. I'm confused as to why it can't just be a for-loop with the iterator...

gabizou · 2017-02-15T08:40:42Z

+            byIncreasingTheSlotIndex:
+            for (int x = 0; x < grid.getColumns(); x++) {
+                ItemStackSnapshot itemStackSnapshot = grid.getSlot(x, y)
+                        .flatMap(Slot::peek).map(ItemStack::createSnapshot)


Don't have multiple calls on the same line for chaining in this case. split up between two separate lines.

gabizou · 2017-02-15T08:41:12Z

+
+                    if (ingredient.test(itemStackSnapshot)) {
+                        iterator.remove();
+


More useless empty lines.

gabizou · 2017-02-15T08:41:42Z

+                    continue;
+                }
+
+                Iterator<Predicate<ItemStackSnapshot>> iterator = ingredients.iterator();


This logic is lacking comments to see what is actually going on (Yes, it's valid checking, but how is it valid checking?)

stephan-gh · 2017-02-18T15:29:30Z

+
+        NonNullList<ItemStack> result = NonNullList.withSize(inv.getSizeInventory(), ItemStack.EMPTY);
+
+        for(int i = 0; i < spongeResult.size(); i++) {


You need a space after for

stephan-gh · 2017-02-18T15:31:54Z

+
+        for (IRecipe irecipe : this.recipes) {
+            if (irecipe.matches(nativeInventory, (net.minecraft.world.World) world))
+            {


Should be on the previous line

gabizou

Commented for some clarification of changes requested.

gabizou · 2017-02-18T19:46:43Z

-        return RecipeUtil.findMatchingRecipe(this.inventory, this.craftingLens.getCraftingGrid(), this.craftingLens.getOutputSlot());
+    public Optional<CraftingRecipe> getRecipe(World world) {
+        checkNotNull(world, "world");
+


This line is a distracting useless line, adding an extra whitespace where it's not really necessary or used.

gabizou · 2017-02-18T19:47:17Z

+    public static GridInventory toSpongeInventory(InventoryCrafting inv) {
+        DefaultInventoryFabric fabric = new DefaultInventoryFabric(inv);
+        GridInventoryLensImpl lens = new GridInventoryLensImpl(0, inv.getWidth(), inv.getHeight(), inv.getWidth(), SlotLensImpl::new);
+


This line is unnecessary extra, there's no real reason for it to be honest.

gabizou · 2017-02-18T19:47:31Z

+        if (inventories.hasNext()) {
+            throw new IllegalStateException("Another inventory found: " + inventories.next());
+        }
+


This line can be removed.

gabizou · 2017-02-18T19:47:51Z

+        Fabric<IInventory> fabric = ((GridInventoryAdapter) inv).getInventory();
+        Iterator<IInventory> inventories = fabric.allInventories().iterator();
+        InventoryCrafting inventoryCrafting = (InventoryCrafting) inventories.next();
+


This line is "ok" since you have some logic going on in the next few lines.

gabizou · 2017-02-18T19:48:24Z

+    public static ItemStack getCraftingResult(Function<GridInventory, ItemStackSnapshot> getResult, InventoryCrafting inv) {
+        ItemStackSnapshot result = getResult.apply(toSpongeInventory(inv));
+
+        Preconditions.checkNotNull(result, "The Sponge implementation returned a `null` result.");


You can static import the checkNotNull method (We do so throughout the implementation)

gabizou · 2017-02-18T19:55:30Z

+                        int gridY = gapY + (gapY >= offsetY ? getHeight() : 0);
+                        boolean empty = grid.getSlot(gridX, gridY)
+                                .flatMap(Slot::peek)
+                                .map(itemStack -> itemStack.getItem() == ItemTypes.NONE)


Does this need to check for isEmpty() itemstacks?

gabizou · 2017-02-18T19:57:45Z

+        if (aisle != null) {
+            Collections.addAll(this.aisle, aisle);
+        }
+


Useless empty line.

gabizou · 2017-02-18T20:04:52Z

+    @Override
+    public ShapedCraftingRecipe.Builder where(char symbol, Predicate<ItemStackSnapshot> ingredient) throws IllegalArgumentException {
+        checkState(!this.aisle.isEmpty(), "aisle must be set before setting aisle symbols");
+


I'm commenting where I'm finding useless new lines for you to get an idea.

gabizou · 2017-02-18T20:05:05Z

+    @Override
+    public ShapedCraftingRecipe.Builder result(ItemStackSnapshot result) {
+        Preconditions.checkNotNull(result, "result");
+


Useless empty line.

gabizou · 2017-02-18T20:05:11Z

+        Preconditions.checkNotNull(result, "result");
+
+        this.result = result;
+


Useless empty line.

kashike · 2017-06-15T12:57:42Z

Superseded by #1395

Limeth mentioned this pull request Jan 3, 2017

Generalize recipes and implement them SpongePowered/SpongeAPI#1449

Closed

Zidane reviewed Jan 3, 2017

View reviewed changes

ST-DDT reviewed Jan 7, 2017

View reviewed changes

ST-DDT suggested changes Jan 7, 2017

View reviewed changes

Limeth force-pushed the feature/recipe-ii branch from ade6f80 to c3a0c37 Compare January 8, 2017 16:58

kashike suggested changes Jan 9, 2017

View reviewed changes

Limeth mentioned this pull request Jan 14, 2017

Generalize recipes and implement them SpongePowered/SpongeForge#1188

Closed

gabizou requested changes Jan 15, 2017

View reviewed changes

simon816 suggested changes Jan 15, 2017

View reviewed changes

gabizou added version: 1.11 (u) API: 6 (unsupported since Jan 1st 2018) system: inventory status: wip Work in progress labels Jan 17, 2017

gabizou requested changes Jan 17, 2017

View reviewed changes

Deamon5550 reviewed Jan 17, 2017

View reviewed changes

Cybermaxke reviewed Jan 18, 2017

View reviewed changes

ST-DDT suggested changes Jan 18, 2017

View reviewed changes

Limeth changed the title ~~[WIP] Generalize recipes and implement them~~ Generalize recipes and implement them Jan 19, 2017

kashike suggested changes Jan 25, 2017

View reviewed changes

Limeth force-pushed the feature/recipe-ii branch from 2d6e838 to e4544b3 Compare January 25, 2017 15:31

Limeth force-pushed the feature/recipe-ii branch 2 times, most recently from ee4c52c to 9e86bcb Compare February 7, 2017 21:00

Implement crafting and smelting recipes

f5d4cec

Limeth force-pushed the feature/recipe-ii branch from 9e86bcb to f5d4cec Compare February 8, 2017 14:33

gabizou requested changes Feb 15, 2017

View reviewed changes

stephan-gh reviewed Feb 18, 2017

View reviewed changes

gabizou requested changes Feb 18, 2017

View reviewed changes

Faithcaio mentioned this pull request Jun 15, 2017

Implement Recipe API #1395

Closed

kashike closed this Jun 15, 2017


		NonNullList<ItemStack> result = NonNullList.withSize(inv.getSizeInventory(), ItemStack.EMPTY);

		for(int i = 0; i < spongeResult.size(); i++) {

		Preconditions.checkNotNull(result, "result");

		this.result = result;

Uh oh!

Conversation

Limeth commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Crafting recipes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Limeth commented Jan 9, 2017

Uh oh!

Maxqia commented Jan 12, 2017

Uh oh!

ST-DDT commented Jan 13, 2017

Uh oh!

Limeth commented Jan 13, 2017

Uh oh!

Limeth commented Jan 13, 2017

Uh oh!

Limeth commented Jan 14, 2017

Uh oh!

gabizou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Limeth commented Jan 3, 2017 •

edited

Loading