Add support for anvil recipes #732

gigaherz · 2017-02-12T17:47:56Z

[11:52] (gigaherz): morning ppl
[11:52] (gigaherz): does JEI come with a template for anvil recipes?
[11:53] (@mezz): nope, it should though. you can PR if you are feeling motivated

So here it is.

I decided not to attempt generating recipe wrapper instances for the vanilla book enchanting and repair, due to how those are hardcoded:

For books, I'd need to test each possible item against each possible enchantment, and see if they are allowed. This is somewhat doable, but it seems ugly.
For repair, Minecraft calls Item#getIsRepairable(ItemStack,ItemStack), with the contents of the anvil slots, and returns true if the material matches the tool. There is no way to figure out the right combinations besides a a nested loop and matching all items against all items.

I'm open to suggestions.

…nvil enchanting or repair, yet.

CLAassistant · 2017-02-12T17:48:57Z

All committers have signed the CLA.

liach · 2017-02-12T17:50:45Z

src/main/java/mezz/jei/plugins/vanilla/anvil/AnvilRecipeCategory.java

+
+public class AnvilRecipeCategory extends BlankRecipeCategory<AnvilRecipeWrapper>
+{
+    @Nonnull


Just use @MethodsNonnullByDefault and @ParametersAreNonnullByDefault in package info file.

Right, I copied from the wrong example. ;P

liach · 2017-02-12T17:51:31Z

src/main/java/mezz/jei/plugins/vanilla/anvil/AnvilRecipeCategory.java

+import javax.annotation.Nonnull;
+
+public class AnvilRecipeCategory extends BlankRecipeCategory<AnvilRecipeWrapper>
+{


The code style...

Yep, I noticed that JEI uses the wrong brace location after I clicked to create the PR.

mezz · 2017-02-13T01:28:29Z

Looks good!
This will need a few things to move on to the next step:

We need a simple API to add recipes, similar to mezz.jei.api.IModRegistry#addDescription()
Manually add vanilla repair recipes using the API, an itemstack and its repair materials manually. Dynamic generation is too slow here unfortunately. Vanilla only has like 5 item classes that override getIsRepairable so it shouldn't be too bad.
See how JER adds enchantment recipes, maybe they found a solution? If it's very inefficient, we'll have to think more about it or just leave enchantments out for now.
Review from @HenryLoenwind since EnderIO has anvil recipes implemented already.

HenryLoenwind · 2017-02-13T04:59:31Z

Looks basically the same as our anvil recipes. I think I could adapt them to use this instead in about 5 minutes. 👍

way2muchnoise · 2017-02-13T15:21:12Z

JER just tests if an enchantment can be applied to an ItemStack (see entry point), I don't know if it is the most efficient way tho. (actual checking). I guess I could look into it again and see if I can find some thing.

HenryLoenwind · 2017-02-13T17:27:24Z

"codacy"? Ok, I'll remember that as "junk to avoid"...

gigaherz · 2017-02-13T17:46:31Z

I'm thinking of adding a 5th parameter to the anvil recipes, so that the recipe can show like, "required: 2+", for cases where the levels cost is variable. Opinions?

liach · 2017-02-13T18:25:00Z

@gigaherz I think that is not defined in a recipe; it is determined by inputs and are calculatable.

HenryLoenwind · 2017-02-13T18:33:01Z

Yes, I think subclassing is the better way here. Maybe split drawInfo(), e.g. in line 40, to make subclassing easier.

gigaherz · 2017-02-13T19:11:37Z

Does this look right for the repair recipes?

The damage numbers are semi-hardcoded (25%, 50%, 75% of max) right now. I don't know if there's a way to show different damage levels and their respective output dynamically.

…aked the formatting settings so that my code is even closer to the existing.

gigaherz · 2017-02-13T19:46:55Z

The repair recipes are now implemented. So far as I can tell, I added all the repairable items and their materials, but if there's something I missed, I'd like to know. ;P

As for enchanting, iterating through all items and testing each one against all enchants seems a bit overkill. It's not hard to implement, but I imagine it would take a rather long time on a big modpack? Are there any numbers from JER regarding that, @way2muchnoise?

gigaherz · 2017-02-13T20:13:09Z

I did a quick test, for vanilla recipes (786 ingredients, 30 enchantments with varying number of levels):

[21:08:42] [Client thread/INFO]: [STDOUT]: Registering repair recipes took 6581 microseconds
[21:08:42] [Client thread/INFO]: [STDOUT]: Registering repair recipes took 24656 microseconds

So it's not too bad. Event with 10x the items, and 2x the enchantments, it would still be less than half a second.

…nt recipes.

gigaherz · 2017-02-13T20:22:04Z

Right now, the enchantment recipes show one by one, including separate entries for each book level. Is there some way to show all book levels as different (rotating) alternatives of the same recipe?

mezz · 2017-02-13T23:15:20Z

@HenryLoenwind I tested out codacy to see if it would be usable for style-checking Forge, but I haven't had much time to configure it properly so it's really strict. It may still be useful.

@gigaherz looks good, that doesn't take as long as I thought.
For rotating recipes, just make an input list of (level 1, 2, 3) and output of applied (level 1, 2, 3) and they will rotate together. It's good enough for this purpose.
For the repair recipes, I think you should only show the recipe for (max damaged item -> somewhat repaired item). That way it's easy to see how much is repaired without having recipe spam.

…s rotation. Make repair show an almost-broken item on the left, so that the output represents the repair amount.

HenryLoenwind · 2017-02-14T06:49:33Z

Be careful with the enchantments, Ender IO has one that can be applied to any item (soulbound).

(semi OT) @mezz, I wasn't commenting on it's strictness, but on its dumbness. Rules that check the signature of a @Overrideing method do not make any sense in the first place. I take having such rules as a sign for "we don't care but want the biggest number of rules and findings to impress those clueless company buyers and execs so they buy our software which is not even half as good as the free/open source alternatives". I get to work with that kind of !%&$ software at work much too often.

gigaherz · 2017-02-14T19:27:05Z

@HenryLoenwind This has changed in 1.11, it seems. ALL means "all of the above" now, instead of literally "all the items". ~~There doesn't seem to be any actual way to have enchantments that can be applied to all items.~~
EDIT: I was wrong, there's still EnumHelper.addEnchantmentType which can help. XD

HenryLoenwind · 2017-02-14T19:42:41Z

~~So canApplyAtEnchantingTable() was moved from the enchantment to the item so we cannot override it anymore? What's the point in that other than annoying modders?~~

Edit: I looked at the call stack, it is still routed through the enchantment. So still full control for the enchantment by returning true from canApply().

Thx, for the heads-up. ~~Expect people to ASM their hooks in...~~

gigaherz · 2017-02-14T19:59:46Z

I added a custom enchantment, set to apply to all items.

[20:54:34] [Client thread/INFO] [jei]: Registered vanilla repair recipes in 1 ms
[20:54:34] [Client thread/INFO] [jei]: Registered enchantment recipes in 33 ms

in contrast, without that one enchant, I get

[20:57:16] [Client thread/INFO] [jei]: Registered vanilla repair recipes in 1 ms
[20:57:16] [Client thread/INFO] [jei]: Registered enchantment recipes in 17 ms

So it it takes about as long to register one "all items" enchantment, as it takes to register all the other ones. On the upside, it's in the milliseconds, so even with 10x the items, and 2x the enchants, that's still within a second. :)

HenryLoenwind · 2017-02-14T20:00:38Z

Try one that returns true from canApply() instead.

gigaherz · 2017-02-14T20:03:21Z

@HenryLoenwind Oh I was talking about EnumEnchantmentType.ALL and how it doesn't mean "ALL" anymore.
I mean, yes, if you override canApply, it works. ;P

HenryLoenwind · 2017-02-14T20:07:30Z

Yeah, I know. I skipped a step and looked up what to do to make an enchantment that is applicable to all items again. There are 2 ways: (a) canApply(ItemStack stack) { return true; } and (b)

EnumHelper.addEnchantmentType("REALLY_ALL", new Predicate<Item>() {
    @Override
    public boolean apply(@Nullable Item input) {
      return true;
    }```

mezz

Looks good. There are a few things I want changed before I test and pull it, but it's pretty much ready.

mezz · 2017-02-15T02:44:43Z

src/main/java/mezz/jei/api/IModRegistry.java

+	void addAnvilRecipe(ItemStack leftInput, ItemStack rightInput, ItemStack output, int levelsCost);
+	void addAnvilRecipe(ItemStack leftInput, ItemStack rightInput, ItemStack output);
+	void addAnvilRecipe(ItemStack leftInput, List<ItemStack> rightInput, List<ItemStack> output, int levelsCost);
+	void addAnvilRecipe(ItemStack leftInput, List<ItemStack> rightInput, List<ItemStack> output);


This should be one method only, not four. Just keep

void addAnvilRecipe(ItemStack leftInput, List<ItemStack> rightInput, List<ItemStack> output, int levelsCost);

I don't think hiding the level cost is valid, if there is no cost they can put 0.

I think that makes the use of the API unnecessarily ugly, but your choice.
However, for context, the option to not have a cost shown wasn't about the recipe not having a cost, it was about the recipe's cost not being known beforehand, such as for enchanted item repair, where the cost depends on how many times the item has been repaired, and how many enchants it has.

Is it possible to automatically determine the cost, maybe using a fake ContainerRepair?

mezz · 2017-02-15T02:52:17Z

src/main/java/mezz/jei/plugins/vanilla/anvil/AnvilRecipeMaker.java

+				Pair.of(new ItemStack(Items.WOODEN_SWORD), repairWood),
+				Pair.of(new ItemStack(Items.WOODEN_PICKAXE), repairWood),
+				Pair.of(new ItemStack(Items.WOODEN_AXE), repairWood),
+				Pair.of(new ItemStack(Items.WOODEN_SHOVEL), repairWood),


I think this would be cleaner as a Map:

Map<ItemStack, List<ItemStack>> items = new HashMap<>(); items.put(repairWood, Arrays.asList( new ItemStack(Items.WOODEN_SWORD), new ItemStack(Items.WOODEN_PICKAXE), ... ));

I disagree, but it's your code so I'll fix it to your preference.

mezz · 2017-02-15T02:54:23Z

src/main/java/mezz/jei/plugins/vanilla/VanillaPlugin.java

@@ -133,17 +141,21 @@ public void register(IModRegistry registry) {
 		recipeTransferRegistry.addRecipeTransferHandler(ContainerFurnace.class, VanillaRecipeCategoryUid.SMELTING, 0, 1, 3, 36);
 		recipeTransferRegistry.addRecipeTransferHandler(ContainerFurnace.class, VanillaRecipeCategoryUid.FUEL, 1, 1, 3, 36);
 		recipeTransferRegistry.addRecipeTransferHandler(ContainerBrewingStand.class, VanillaRecipeCategoryUid.BREWING, 0, 4, 5, 36);
+		recipeTransferRegistry.addRecipeTransferHandler(ContainerRepair.class, VanillaRecipeCategoryUid.ANVIL, 0, 2, 3, 4 * 9);


4 * 9 should be 36 like the other lines above it

mezz · 2017-02-15T02:55:31Z

src/main/java/mezz/jei/plugins/vanilla/anvil/AnvilRecipeWrapper.java

+		this(leftInput, Collections.singletonList(rightInput), Collections.singletonList(output), levelsCost);
+	}
+
+	public AnvilRecipeWrapper(ItemStack leftInput, List<ItemStack> rightInputs, List<ItemStack> outputs) {


Similar to the API, this class should only need one constructor.

mezz · 2017-02-15T02:55:57Z

src/main/java/mezz/jei/plugins/vanilla/anvil/AnvilRecipeWrapper.java

+		this(leftInput, rightInputs, outputs, -1);
+	}
+
+	@SuppressWarnings("unchecked")


@SuppressWarnings("unchecked") should not be needed

If it isn't, I don't know how. I tried both Lists.newArrayList and Arrays.asList. So far as I can tell, Java is stupid and doesn't know how to handle generics in arrays, so it panics when it sees the varargs parameter type as List<ItemStack>.

I don't think it's possible to use Arrays.asList with genetics this way, you're right.
Try creating a new arraylist and then adding the inputs to it after creation.

* Reduced number of overloads * Changed the list of pairs to a map of lists * Computed the result of a multiplication. * Unsilenced an unchecked operation.

gigaherz · 2017-02-15T09:30:16Z

I think this makes all the comments addressed.

mezz · 2017-02-15T09:31:03Z

Is it possible to automatically determine the level cost, maybe using a fake ContainerRepair?

HenryLoenwind · 2017-02-15T09:31:47Z

Yes, but that would be the cost for a fresh item, at best misleading ...

…

Sent from my iPhone

On 15 Feb 2017, at 10:26, James Mitchell ***@***.***> wrote: Warning: This message has had one or more attachments removed Warning: (msg-20509-39.html). Warning: Please read the "j-e-b-Attachment-Warning.txt" attachment(s) for more information. mezz commented on this pull request. > @@ -92,6 +92,19 @@ void addDescription(List<ItemStack> itemStacks, String... descriptionKeys); /** + * Adds an anvil recipe for the given inputs and output. + * @param leftInput The itemStack placed on the left slot. + * @param rightInput The itemStack placed on the right slot. + * @param output The resulting itemStack. + * @param levelsCost The cost, in experience levels, that will be required in order to pick up the item. + * Optional: If not specified, the cost text will not display on the recipe. + */ + void addAnvilRecipe(ItemStack leftInput, ItemStack rightInput, ItemStack output, int levelsCost); + void addAnvilRecipe(ItemStack leftInput, ItemStack rightInput, ItemStack output); + void addAnvilRecipe(ItemStack leftInput, List<ItemStack> rightInput, List<ItemStack> output, int levelsCost); + void addAnvilRecipe(ItemStack leftInput, List<ItemStack> rightInput, List<ItemStack> output); Is it possible to automatically determine the cost, maybe using a fake `ContainerRepair`? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #732 This is a message from the MailScanner E-Mail Virus Protection Service ---------------------------------------------------------------------- The original e-mail message contained potentially dangerous content, which has been removed for your safety. The content is dangerous as it is often used to spread viruses or to gain personal or confidential information from you, such as passwords or credit card numbers. Due to limitations placed on us by the Regulation of Investigatory Powers Act 2000, we were unable to keep a copy of the original attachment. The content filters found this: MailScanner: Found a script in HTML message -- Postmaster MailScanner thanks transtec Computers for their support

mezz · 2017-02-15T09:33:43Z

It's better than no information, and means that nobody has to specify the level with the API.

…ayed in the rotation.

Add support for anvil recipes. Does not add recipes for the vanilla a…

34d1da9

…nvil enchanting or repair, yet.

liach reviewed Feb 12, 2017

View reviewed changes

gigaherz added 3 commits February 12, 2017 18:51

Fixup brace location to better match the rest of the code.

8f7c002

Make use of the default-nonnull package annotations.

ee9b960

Improve the logic in the anvil category.

a03b560

Added API for adding anvil recipes. Added vanilla repair recipes. Twe…

48fd24a

…aked the formatting settings so that my code is even closer to the existing.

Fixed javadoc comment. Implement ("brute-forced") the anvil enchantme…

9f07666

…nt recipes.

Use the right Pair implementation.

75c026f

Consolidated enchantment recipes per level into one recipe with level…

3e3da07

…s rotation. Make repair show an almost-broken item on the left, so that the output represents the repair amount.

mezz requested changes Feb 15, 2017

View reviewed changes

gigaherz added 2 commits February 15, 2017 10:27

Address review comments:

35cc453

* Reduced number of overloads * Changed the list of pairs to a map of lists * Computed the result of a multiplication. * Unsilenced an unchecked operation.

Construct the list manually.

c68bd31

gigaherz added 2 commits February 15, 2017 11:53

Make the recipe cost calculated dynamically, based on the items displ…

94606ef

…ayed in the rotation.

Bump version and mark the APIs' accordingly.

ddc1e6f

mezz merged commit 9891cbc into mezz:1.11 Feb 15, 2017

gigaherz deleted the Anvil-Recipes branch February 15, 2017 11:28

Lothrazar mentioned this pull request Mar 11, 2017

[Mod Compatibility] JEI now supports anvil recipes, but recipes from this mod dont show up rarmenia/Anvil-Enchantments#8

Closed

Add support for anvil recipes #732

Add support for anvil recipes #732

Conversation

gigaherz commented Feb 12, 2017

CLAassistant commented Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mezz commented Feb 13, 2017 • edited Loading

HenryLoenwind commented Feb 13, 2017

way2muchnoise commented Feb 13, 2017 • edited Loading

HenryLoenwind commented Feb 13, 2017

gigaherz commented Feb 13, 2017

liach commented Feb 13, 2017

HenryLoenwind commented Feb 13, 2017

gigaherz commented Feb 13, 2017 • edited Loading

gigaherz commented Feb 13, 2017

gigaherz commented Feb 13, 2017 • edited Loading

gigaherz commented Feb 13, 2017

mezz commented Feb 13, 2017

HenryLoenwind commented Feb 14, 2017

gigaherz commented Feb 14, 2017 • edited Loading

HenryLoenwind commented Feb 14, 2017 • edited Loading

gigaherz commented Feb 14, 2017

HenryLoenwind commented Feb 14, 2017

gigaherz commented Feb 14, 2017 • edited Loading

HenryLoenwind commented Feb 14, 2017

mezz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mezz Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

gigaherz commented Feb 15, 2017

mezz commented Feb 15, 2017

HenryLoenwind commented Feb 15, 2017 via email

mezz commented Feb 15, 2017

CLAassistant commented Feb 12, 2017 •

edited

Loading

mezz commented Feb 13, 2017 •

edited

Loading

way2muchnoise commented Feb 13, 2017 •

edited

Loading

gigaherz commented Feb 13, 2017 •

edited

Loading

gigaherz commented Feb 13, 2017 •

edited

Loading

gigaherz commented Feb 14, 2017 •

edited

Loading

HenryLoenwind commented Feb 14, 2017 •

edited

Loading

gigaherz commented Feb 14, 2017 •

edited

Loading

mezz Feb 15, 2017 •

edited

Loading