New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recipe API #1582

Closed
wants to merge 1 commit into
base: bleeding
from

Conversation

@Faithcaio
Copy link
Contributor

Faithcaio commented Jun 15, 2017

SpongeAPI|SpongeCommon|SpongeForge

Continuation of #1449 of #1098.
Updated for MC 1.12


RecipeAPI:

Recipe: Base Recipe only has a basic getExemplaryResult(). For most Recipe this is also the actual result but it may vary.
CraftingRecipe: Recipes for a Crafting Window.
Ingredient: Predicate to match in CraftingRecipes.
ShapedCraftingRecipe: Ingredients must be arranged specifically
ShaplessCraftingRecipe: Ingredients can be arranged randomly
SmeltingRecipe: Recipes for Furnaces (uses ITemStacks not Ingredients)
RecipeRegistry: Base Registry for Recipes
CraftingRecipeRegistry: Registration of CraftingRecipes
SmeltingRecipeRegistry: Registration of SmeltingRecipes
CraftingResult: Result of a crafted Recipe


Example Usage

// Shapeless
Sponge.getRegistry().getCraftingRecipeRegistry().register(
                CraftingRecipe.shapelessBuilder()
                        .addIngredient(Ingredient.of(APPLE))
                        .result(ItemStack.of(APPLE, 2))
                        .build("moreapples", plugin));
// Shaped

Sponge.getRegistry().getCraftingRecipeRegistry().register(
        CraftingRecipe.shapedBuilder()
                .aisle("aa", "as", " s")
                .where('a', Ingredient.of(DIAMOND_AXE))
                .where('s', Ingredient.of(LOG, LOG2))
                .result(ItemStack.of(DIAMOND_AXE, 1))
                .build("cheapaxe", plugin));

OR implement the CraftingRecipe yourself to allow customizing the result based on input:
img


TODO and Discuss:

  • Recipe removal? (RecipeRegistry#remove)
  • Expose Recipe Group in API? (Used on the client to decide in what tab a recipe is shown)
  • Expose Ingredient.matchingStacks in API? (displayedItems) used client side to show the items used in the recipe
  • setting matchingstacks in IngredientBuilder?
  • add ShapedRecipeBuilder set Ingredients with a Map<String/Char, Ingredient>?
  • IngredientBuilder#from cannot always recreate a passed Ingredient. Mention this in builder.
  • Chars in aisle without assigned ingredients? Exception except space with is default Ingredient.NONE
  • RecipeBuilders convenience method with Predicate for adding ingredients?
  • SmeltingRecipes dont use ingredients.
  • expose IRecipe.isHidden? or always set to true? otherwise doLimitedCrafting gamerule could block all recipes added via API.
  • Javadocs everywhere

Ideas for further enhancement:
see #1585

@pschichtel

This comment has been minimized.

Copy link

pschichtel commented Jun 15, 2017

I have a few suggestions for the shaped builder API:

The current API statically allows to build incomplete recipes. For example a simple recipe with .aisle("a", " a").where('a', DIAMOND_AXE) could be changed to .aisle("a", " b") while forgetting to define 'b'. This would still compile, but eventually fail at runtime. I don't really see any benefit in using the weird separation.
So my suggestion would be to change aisle(String...) to aisle(Ingredient[][]), so the example from above could look something like this:

Ingredient a = ItemTypes.DIAMOND_AXE;
Ingredient s = Ingredient.builder().with(ItemTypes.LOG, ItemTypes.LOG2).build();
Sponge.getRegistry().getCraftingRecipeRegistry().register(plugin, "chopchop",
        CraftingRecipe.shapedBuilder()
                .aisle({{a, a}, {a, s}, {NONE, s}})
                .result(ItemStack.of(ItemTypes.DIAMOND_AXE, 1))
                .build());

aisle also doesn't really seem like the correct term here and with Java's array syntax it may not be the most usable experience, so I'd further suggest to replace aisle(Ingredient[][]) by something like row(Ingredient...). This method could then be called once for each row top to bottom. Additional helpers like emptyRow() (leave the row empty) and row(int, Ingredient...) (indent the row by n cells).

The above example:

Ingredient a = ItemTypes.DIAMOND_AXE;
Ingredient s = Ingredient.builder().with(ItemTypes.LOG, ItemTypes.LOG2).build();
Sponge.getRegistry().getCraftingRecipeRegistry().register(plugin, "chopchop",
        CraftingRecipe.shapedBuilder()
                .row(a, a)
                .row(a, s)
                .row(1, s)
                .result(ItemStack.of(ItemTypes.DIAMOND_AXE, 1))
                .build());

If the original intention of the string-pattern was to be able to reuse patterns, I can only say: functions.

* {@link GridInventory}
*/
public List<ItemStackSnapshot> getRemainingItems() {
return remainingItems;

This comment has been minimized.

@kashike

kashike Jun 15, 2017

Member

this.

// TODO expose group String?

/**
* Checks if the given {@link GridInventory} fits the required constraints

This comment has been minimized.

@Zidane

Zidane Jun 15, 2017

Member

Not a GridInventory anymore

@kashike

This comment has been minimized.

Copy link
Member

kashike commented Jun 15, 2017

@pschichtel It should be expected that people who want to write plugins/mods should test their plugin/mod before releasing it for use. If they test, they'll see an exception mentioning an ingredient was forgotten, just like they would if they forgot to specify something on another builder (the name on a boss bar builder, for example).
Additionally, vanilla Minecraft, as well as Bukkit and Forge, use the same aisle/where method (vanilla calls it pattern/key), so it's far from new:

* Crafting recipes can only be crafted when all of the ingredients match
* the items in the input grid.
*/
public interface Ingredient extends Predicate<ItemStack> {

This comment has been minimized.

@liach

liach Jun 15, 2017

You may consider creating an ingredient from a game dictionary entry.

@Faithcaio Faithcaio force-pushed the feature/recipe branch from 96a1a03 to 0c76943 Jun 15, 2017

@pschichtel

This comment has been minimized.

Copy link

pschichtel commented Jun 15, 2017

@kashike by this logic, we should all start writing our projects in JavaScript or LUA. If we don't use the type-system, we even have one? Sure, if I write good code and have a 100% test coverage before every release, everything will work perfectly fine with either API. Then again, we're talking about Minecraft plugins, often written by beginner or intermediate skilled developers, barely tested if at all. Even skilled developers benefit from faster development cycles as the compiler can show the error much faster than deploying the plugin in a server, joining the server and testing the functionality.
Generally I think a builder that allows me to end the building process (e.g. call .build()) in an incomplete or inconsistent state is just bad. If other Sponge builders are like that, then that doesn't make the design good, just consistent(-ly sub-optimal).
Conceptually my suggestion isn't much different than the mentioned APIs, it just reduces the noise. Again: Just because others are doing it this way, doesn't imply it's a good way. I don't see any benefit in the shape definition using strings, let alone one that justifies sacrificing static verification support. And if the character based ingredient mapping is required for Minecraft/Vanilla compatibility, then that could easily be generated inside the builder implementation (assign a consecutive letter to every unique ingredient). This is nothing more than an implementation detail.
After all, isn't Sponge about improving upon its predecessors?

@kashike

This comment has been minimized.

Copy link
Member

kashike commented Jun 15, 2017

An exception would be thrown when build is called, as with a ton (all?) of our other builders when required information has not been provided. If someone can't be bothered to even start a server with their plugin, well, then that's just silly of them.

@pschichtel

This comment has been minimized.

Copy link

pschichtel commented Jun 15, 2017

@kashike consider larger plugins, developed by fairly inexperienced developers (there exist quite a few of those, some of them widely used). Even my first Bukkit plugin was downloaded like 20k times on bukkit dev. I didn't always test every little feature. Who has even the time for that? Or the userbase to run beta phases?
Recipes are not bound to be registered on plugin startup (or are they?), so plugins could activate recipes upon certain events under certain conditions. Those would all need to be tested before a release.

A good builder kind of guides a user to a properly build object. Take a look at the jOOQ project, which is basically just a builder for SQL statements.

@liach

This comment has been minimized.

Copy link

liach commented Jun 15, 2017

@pschichtel This "oh crap" feature presents on 90% of the builders. Then, in your theory, builders should be never used because they are extremely likely for noobs to have issues on.

@Deamon5550

This comment has been minimized.

Copy link
Member

Deamon5550 commented Jun 15, 2017

A builder having an incomplete state at some point before build() is called is perfectly fine (normal even) as you almost always have certain bits of information that are required to be set. You can't have a recipe that has no result yet your not going to get a compile error if you don't pass that result to your builder. This is a fact of life with a builder pattern and is offset against the positives of the pattern.

@kashike

This comment has been minimized.

Copy link
Member

kashike commented Jun 15, 2017

Sure, an issue like that (registering a recipe later down the line, after a server has been running) can pop up. As can an NPE, SOE, IOOBE, etc.

@liach

This comment has been minimized.

Copy link

liach commented Jun 15, 2017

@pschichtel This is harder than you've expected to screw up. I've seen many "stupid modders" doing silly things around, but never one who fucked up the most basic recipes. It is easy to use and easy to understand.

@pschichtel

This comment has been minimized.

Copy link

pschichtel commented Jun 15, 2017

@liach Probably. That makes them consistent (which is not a bad property!), but in cases where a static verification is possible and easy, why not do it? I'm also perfectly aware, that these recipe builders are not hard to use. It's not about hard or easy, it's about preventing accidental mistakes, that can happen to anyone at any skill level in projects of any size.

@Deamon5550 It is fine sure. There are also cases where a complete static verification is not possible. For example if Vanilla MC doesn't support non-square, that could not be modelled in the builder, without potentially breaking support for mods. I also don't think every builder in Sponge MUST be completely statically safe, as that would introduce a lot of changes, but it would easily possible to model them that way. As mentioned, take a look at jOOQ.

@kashike Sure, any error can popup during run time, with or without recipes. This is not about making Plugins uncrashable, but about preventing one kind of error while at the same time increasing code quality.

While the static safety is a good reason to consider my approach, it is also not the only one (actually not even my main one, I might have focused to much on that). Has anyone a compelling reason to require these characters for the ingredient mapping? I don't see any reason why they need to be there. They don't add any new information to the code, just a potential (possibly unlikely) source of errors. The fact, that they can easily be generated confirms this. "Because it was always this way" should not be a reason in a discussion about a new API.

@liach

This comment has been minimized.

Copy link

liach commented Jun 15, 2017

@Faithcaio Can you add a shortcut for plugin makers to turn an oredict entry into an ingredient?

* @param entry The GameDictionary entry.
* @return This Builder, for chaining
*/
default Builder with(GameDictionary.Entry entry) {

This comment has been minimized.

@liach

liach Jun 15, 2017

You might consider adding display items from these entries too.

@ryantheleach

This comment has been minimized.

Copy link
Member

ryantheleach commented Jun 16, 2017

@Faithcaio How are recipes with Logic handled? e.g. buckets returning empty buckets, clone recipes, Fireworks, etc.

@liach

This comment has been minimized.

Copy link

liach commented Jun 16, 2017

@ryantheleach They are registered separately in code, not as jsons.

@Faithcaio

This comment has been minimized.

Copy link
Contributor

Faithcaio commented Jun 16, 2017

Vanilla has a concept of items that are a container for the bucket logic.
Anything else has to implement CraftingRecipe#getResult(grid) and CraftingRecipe#getRemainingItems(grid)

@ST-DDT
Copy link
Contributor

ST-DDT left a comment

I really like this Recipe API and its good that it receives some attention again.

*/
void register(Recipe recipe);
void register(Object plugin, String id, T recipe);

This comment has been minimized.

@ST-DDT

ST-DDT Jun 16, 2017

Contributor

Why do I have to set the id here instead of on the recipe itself?

This comment has been minimized.

@liach

liach Jun 16, 2017

So the recipes should be a catalog type? That's a good point.

* @param items The list of items.
* @return This Builder, for chaining
*/
Builder withDisplay(List<ItemStack> items);

This comment has been minimized.

@ST-DDT

ST-DDT Jun 16, 2017

Contributor

Maybe also add a vararg variant and a variant with Snapshots and ItemTypes, because in the methods above you use them as well.

* @param result The resultant stack
* @return The builder
*/
Builder result(ItemStack result);

This comment has been minimized.

@ST-DDT

ST-DDT Jun 16, 2017

Contributor

You should default implement this method to use the ItemStackSnapshot one. And remove the default impl for the other one, because ItemStacks are mutable. Similar to the ShapelessRecipe one.

This comment has been minimized.

@Faithcaio

Faithcaio Jun 16, 2017

Contributor

vanilla expects ItemStacks. Seems pointless to convert Itemstacks into Snapshots just to turn them back into ItemStacks and copy them in the impl.

This comment has been minimized.

@ST-DDT

ST-DDT Jun 16, 2017

Contributor

Well all other builders do the same (using Snapshots). And you have to copy them anyway.

Input RecipeCreation Usage
ItemStack Copy Copy 2
ItemStack Snapshot ItemStack Copy

Because the input stack needs to be copied to prevent edits after recipe creation because of a dangling reference. And just for consistency i suggest using the snapshots everywhere.

PS: Default implementations can be overwritten

This comment has been minimized.

@Faithcaio

Faithcaio Jun 16, 2017

Contributor

Still don't get it why I should convert everything into a Snapshot.
In the impl I have to convert everything back into ItemStack and then copy.
So it would convert ItemStack -> Snapshot -> ItemStack -> Copy
Instead of ItemStack -> Copy

Also why would anyone overwrite the Builder?

This comment has been minimized.

@liach

liach Jun 16, 2017

As long as the item stack don't change after they are sent into the registry, things will be fine. We just need to copy it in the implementation before we send it into the registry.

This comment has been minimized.

@dualspiral

dualspiral Jun 16, 2017

Contributor

@ST-DDT If you provide both the ItemStackSnapshot and the ItemStack options, why does it matter which one is default implemented? It certainly shouldn't matter from a developer/API consumer standpoint - just because something is default implemented doesn't mean that it changes the API signature.

From a consistency standpoint, yes, including an option to supply an ItemStackSnapshot as a result makes sense to me. However, default methods really are an implementation detail bleeding into the API package anyway, so if we want to be really pure about it, I'd say don't add default methods to new APIs at all, only to existing interfaces that get extended and plugin developers are expected to implement.

* @param ingredient The required ingredient
* @return This builder, for chaining
*/
Builder ingredient(ItemStackSnapshot ingredient);

This comment has been minimized.

@ST-DDT

ST-DDT Jun 16, 2017

Contributor

Maybe default implement this?

This comment has been minimized.

@liach

/**
* Retrieves the recipe which would be crafted when the player clicks
* the output slot.

This comment has been minimized.

@ST-DDT

ST-DDT Jun 16, 2017

Contributor

The javadocs seem to be wrong as smelting does not require clicking, does it?

@Override
public int hashCode() {
int result1 = this.result.hashCode();
long temp = Double.doubleToLongBits(this.experience);

This comment has been minimized.

@ST-DDT

ST-DDT Jun 16, 2017

Contributor

Use Double.hashCode(experience)

This comment has been minimized.

@liach

liach Jun 16, 2017

Just Objects.hashcode should be fine, I think.

This comment has been minimized.

@Faithcaio

Faithcaio Jun 16, 2017

Contributor

apparantly this is how IntelliJ generates it

*/
Builder result(ItemStack result);

// TODO results dependent on craftinggrid & world?

This comment has been minimized.

@ST-DDT

ST-DDT Jun 16, 2017

Contributor

Such a method would be nice. However I would split the builder for it in multiple subclasses to keep the interfaces and implementations as clean as possible (= less confusing).

  • AisleCraftingRecipeBuild or Mapping....
  • RowBased....
  • Dynamic...
@liach

This comment has been minimized.

Copy link

liach commented Jun 16, 2017

@Faithcaio As of Minecraft 1.12, recipes are organized into registries. I wonder if it is necessary to put recipes into catalog types and Use a class to contain all vanilla recipes like Keys class.

@ST-DDT

ST-DDT approved these changes Jun 16, 2017

Copy link
Contributor

ST-DDT left a comment

Interesting way of splitting the builders (not what i imagined, but i guess that works as well)

@liach

This comment has been minimized.

Copy link

liach commented Jun 16, 2017

@Faithcaio Although people cannot remove recipe in the way they remove advancements, however, the recipes will be disabled by recipe books. Do we add recipe books as a way to disable, rather than ban, recipes?

@Faithcaio

This comment has been minimized.

Copy link
Contributor

Faithcaio commented Jun 16, 2017

We could add them, but i'd like to keep it separate from this PR.

@kashike kashike referenced this pull request Jun 17, 2017

Closed

Recipe Refactor #1419

@Faithcaio Faithcaio referenced this pull request Jun 17, 2017

Closed

Implement Recipe API #1593

@Faithcaio Faithcaio removed the status: wip label Jun 17, 2017

@Faithcaio

This comment has been minimized.

Copy link
Contributor

Faithcaio commented Jun 17, 2017

So this and Impl in Common is basicly done.
SpongeForge will have to wait a bit.

Can we get this merged this weekend?

@kashike
Copy link
Member

kashike left a comment

good aside from these, and some missing javadocs

@@ -304,6 +305,8 @@

public static final Class<WorldGeneratorModifier> WORLD_GENERATOR_MODIFIER = WorldGeneratorModifier.class;

public static final Class<CraftingRecipe> CRAFTING_RECIPES = CraftingRecipe.class;

This comment has been minimized.

@kashike

kashike Jun 17, 2017

Member

needs to be sorted


/**
* The group this CraftingRecipe belongs to.
* The group defines on which Tab the Recipe is shown in the RecipeBook.

This comment has been minimized.

@kashike

kashike Jun 17, 2017

Member

no it doesn't

@Faithcaio Faithcaio force-pushed the feature/recipe branch from 24b9daa to 8cefab6 Jun 17, 2017

@Faithcaio Faithcaio force-pushed the feature/recipe branch from 8cefab6 to 642a973 Jun 17, 2017

@liach

liach approved these changes Jun 17, 2017

* @param world The world where the item would be crafted in
* @return The recipe or {@link Optional#empty()} if no recipe is formed
*/
Optional<CraftingRecipe> getRecipe(World world);

This comment has been minimized.

@Zidane

Zidane Jun 17, 2017

Member

Why does World matter?

This comment has been minimized.

@Faithcaio

Faithcaio Jun 17, 2017

Contributor

Vanilla matches Recipes with an InventoryCrafting and a World.
In vanilla it is only used to get MapData.

Maybe we could provide some kind of CraftingContext instead?

* A CraftingGridInventory represents the inventory of something that can craft
* items. This is excluding the Result slot.
*/
public interface CraftingGridInventory extends GridInventory {

This comment has been minimized.

@Zidane

Zidane Jun 17, 2017

Member

Why is result excluded?

This comment has been minimized.

@Faithcaio

Faithcaio Jun 17, 2017

Contributor

This is the equivalent to InventoryCrafting.

CraftingInventory is a view on InventoryCrafting and the ResultSlot

@Faithcaio Faithcaio referenced this pull request Jun 18, 2017

Open

Recipe API Enhancements #1585

0 of 7 tasks complete
@Faithcaio

This comment has been minimized.

Copy link
Contributor

Faithcaio commented Jun 18, 2017

Any more concerns? If not I'll merge it today.

*
* @return The list of items to display the Ingredient in a recipe.
*/
List<ItemStack> displayedItems();

This comment has been minimized.

@Minecrell

Minecrell Jun 18, 2017

Member

ItemStack is mutable. Shouldn't this return a list of ItemStackSnapshots?

@Faithcaio Faithcaio force-pushed the feature/recipe branch from 83584dd to 97baaa6 Jun 18, 2017

New Recipe API
CraftingRecipe with Ingredients
SmeltingRecipes

@Faithcaio Faithcaio force-pushed the feature/recipe branch from 97baaa6 to b2dff8d Jun 18, 2017

@Faithcaio

This comment has been minimized.

Copy link
Contributor

Faithcaio commented Jun 18, 2017

cd19310

@Faithcaio Faithcaio closed this Jun 18, 2017

@Faithcaio Faithcaio deleted the feature/recipe branch Jun 18, 2017

@parlough parlough referenced this pull request Nov 11, 2017

Closed

Dynamic Recipes #599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment