Adding CustomModelChoice to bukkit recipe system

[derverdox]

CustomModelChoice is used to create recipes that not only compare for materials but also CustomModelData.
Using the ExactChoice alternative is not a good idea since items can have different lores or attributes.
This is a great way to implement recipes with the new possibilities of Minecraft custom items.

[powercasgamer]

Don't use var

[powercasgamer]

Don't use var

[lynxplay]

This feels rather arbitrary IMO.
Preferably we just match specific nbt tags.

Custom model data feels somewhat randomly selected from the plethora of ways plugin developers can identify custom items. Given the fact that custom model data is not even the preferred way of identifying custom items (the PDC is) I am really not sure if I like a slapped on server side recipe choice that makes use of the custom model data value.

The fact that mojang does not support nbt matching recipes has been a long standing issue and I 100% can see the technical usage for this, however I think if we end up trying to slap a weaker version of ExactRecipe choice at it, then we should be looking into a more generally applicable implementation rather than grabbing a display value such as customModelData.

[derverdox]

This feels rather arbitrary IMO. Preferably we just match specific nbt tags.

Custom model data feels somewhat randomly selected from the plethora of ways plugin developers can identify custom items. Given the fact that custom model data is not even the preferred way of identifying custom items (the PDC is) I am really not sure if I like a slapped on server side recipe choice that makes use of the custom model data value.

The fact that mojang does not support nbt matching recipes has been a long standing issue and I 100% can see the technical usage for this, however I think if we end up trying to slap a weaker version of ExactRecipe choice at it, then we should be looking into a more generally applicable implementation rather than grabbing a display value such as customModelData.

I see your point. However, the idea of it is that we actively ignore the rest of the information in the pdc. I agree that we might need a more generalized approach but there are some things ExactChoice can't do for us.

Consider the following scenario:
I used to build a concept where every item had a description—based on where it came from it had a different description. They also had custom textures. So only comparing them by materials would not make sense. Instead, I would like to compare them using their custom model data and their material.

Let's say you have 20 different apples from different regions. They are all the same "item" but differ in their description based on the biome they were collected from.
I think it's not a good idea to create 20 different ExactChoices for every description. Besides, it would not be possible to change the description of the custom item dynamically because there would be no exact choice for that.
So comparing for customModelData as well makes sense in my head or am I missing something?

[lynxplay]

What do you mean the *rest" of the PDC ? CustomModelData isn't in the pdc. I 100% get what you are going for here. I am not saying it would be useless.

I am more arguing that this change picks an arbitrary way to identify a custom item and promotes it to the API with a recipe choice when plenty of other ways exist.

It's in a way fixing a symptom of a way deeper issue, Mojang not permitting recipe ingredients via compound tag.

If I am identifying my custom items via a pdc tag, e.g. I have a saord that changes custom model data due to some in-game event but remains the same item because it's just a visual change, this PR fails to give me the ability to havw it as a recipe choice.

If I (for whatever reason) use the display name, same concept applies.
This approach is imo just flawed and now generic enough for what it is trying to achieve.

[derverdox]

What do you mean the *rest" of the PDC ? CustomModelData isn't in the pdc. I 100% get what you are going for here. I am not saying it would be useless.

I am more arguing that this change picks an arbitrary way to identify a custom item and promotes it to the API with a recipe choice when plenty of other ways exist.

It's in a way fixing a symptom of a way deeper issue, Mojang not permitting recipe ingredients via compound tag.

If I am identifying my custom items via a pdc tag, e.g. I have a saord that changes custom model data due to some in-game event but remains the same item because it's just a visual change, this PR fails to give me the ability to have it as a recipe choice.

If I (for whatever reason) use the display name, same concept applies. This approach is imo just flawed and generic enough for what it is trying to achieve.

I get your point.
The fact that it solves problems vanilla recipes have with custom model data is a pleasant consequence.
A more generalized approach would be to have something like an ExactChoice that compares only to what is there.

Like a "predicate choice" where your input is a template ItemStack, the Choice compares every existing data in the nbt while ignoring what is not there.
If the template item has customModelData it compares for that. If it has a lore it compares with the lore.
If it has a custom display name it compares to that.
The difference with exactchoice is that it requires a replica. The "predicate choice" would only compare for data in the item stack nbt and ignore everything the template does not have.

You could even give an extra boolean flag to the ExactChoice, whether it should compare for non-existing data aswell. (Like it does right now)

[lynxplay]

Yea pretty much. A "here is this compound tag of nbt, only compare the inputs values if the provided tag even has that value" recipe choice would be nice but rn I am unsure if we have nice enough API to model that kind of item nbt

I presume whenever the properties API makes it in, that might be nicer.

[Machine-Maker]

Do you know if this works with shapeless recipes? My initial guess is that it does not. The initial implementation of ExactChoice also did not work with shapeless recipes which upstream just decided to ignore putting in the javadoc that it didn't work with them. I don't really want to go back to that adding more RecipeChoice that doesn't work in all recipes.

[derverdox]

Do you know if this works with shapeless recipes? My initial guess is that it does not. The initial implementation of ExactChoice also did not work with shapeless recipes which upstream just decided to ignore putting in the javadoc that it didn't work with them. I don't really want to go back to that adding more RecipeChoice that doesn't work in all recipes.

Ive been using this implementation for quite a while now but I am willing to do a full testing session to make clear it works on every recipe! :)

[derverdox]

Do you know if this works with shapeless recipes? My initial guess is that it does not. The initial implementation of ExactChoice also did not work with shapeless recipes which upstream just decided to ignore putting in the javadoc that it didn't work with them. I don't really want to go back to that adding more RecipeChoice that doesn't work in all recipes.

Is there a specific reason why we are still using StackingContents and RecipePicker instead of building a whole new system? I was thinking about PredicateChoice. MaterialChoice and ExactChoice would extend PredicateChoice. However I don't know if we can get same performance achieved by Bitset dfs etc...

Or is something in the works already?

[electronicboy]

The nature of ExactChoice being flawed is more an issue with how bukkit represents itemstacks and would generally stand a chance of being fixed once we get the property API
Replacing entire vanilla systems generally needs a strong reason when plugins have been delivering this within their own code for years now, especially given the expectations that we'd be expected to maintain it, while mojang is working on stuff which would generally make the custom model data irrelevant for a good chunk of its usecases

[derverdox]

The nature of ExactChoice being flawed is more an issue with how bukkit represents itemstacks and would generally stand a chance of being fixed once we get the property API Replacing entire vanilla systems generally needs a strong reason when plugins have been delivering this within their own code for years now, especially given the expectations that we'd be expected to maintain it, while mojang is working on stuff which would generally make the custom model data irrelevant for a good chunk of its usecases

I agree. However, I think we need a more generalized approach to implementing custom recipes. Like PredicateChoice

[derverdox]

I created a second pull request regarding PredicateChoice API. Let me know your thoughts #9996