-
Notifications
You must be signed in to change notification settings - Fork 399
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
Cooking recipes working with count. #3512
base: 1.20.4
Are you sure you want to change the base?
Conversation
Tweaks cooking recipes to be able to deserialise "result" as an item string or an item json object with count.
}, | ||
"result": { | ||
"item": "minecraft:iron_ingot", | ||
"count": 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming it should be fabric:count
instead of count
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is using the Vanilla Codec for processing crafting recipe items with count. There is merit to renaming it in order to differentiate it from Vanilla datapack processing, but personally I prefer the consistency with vanilla.
I'm open to changing this however, but for now I am partial towards the proposal Shadows-of-Fire mentioned in the NBT thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I personally agree with your point of view, but fabric adds a suffix to all of its changes to vanilla formats (look at resource conditions and custom ingredients), so consistency should be assessed from fabric's point of view, not vanilla's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing me to these, I will take a look at them.
I agree that consistency should be prioritised over the mod as a whole, so I will look into building a separate codec to use with fabric:count
and build and test it over the next few days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about changing the new result format to fabric:result
?, Leaving out the normal result
.
So something like this:
{
"type": "minecraft:smelting",
"group": "iron_ingot",
"ingredient": {
"item": "minecraft:deepslate_iron_ore"
},
"fabric:result": {
"item": "minecraft:iron_ingot",
"count": 3
},
"experience": 0,
"cookingtime": 1
}
My reasoning is as follows: This change does not simply add to an additional option to an existing format, but instead adds an entirely new way of expressing results. For example, simply naming the (optional) count value fabric:count
does not imply that a result of the format
"result": {
"item": "minecraft:iron_ingot"
}
would fail to compile in Vanilla (which it would). Renaming to fabric:result
over result
would express that this is a fabric-specific datapack file, even in the case where an item count is not provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will this work with datagen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some form of additional functionality would be required for modders to be able to use this functionality with datagen. I do not intend to implement this within this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, to what extent is fabric:result
a realistic option? I would prefer the option that is easier from a datagen point of view, because a large number of recipes are created through datagen. I'm not saying you need to implement datagen support, I'm saying we can't make a choice without datagen
You change the codec, okay, but what about networking? |
Iirc networking uses the codecs for both serialisation and deserialisation also? I assume there may be some slowdown from packets switching from the String format to the JSON object format, but I don't imaging this is particularly significant. Crafting uses the same Codec. |
I decided to recheck the code and everything seems fine, so I'm sorry. :D My concern was that network packets might ignore the item NBT. And btw, no, recipe network packets doesn't use codecs |
Ah thanks, must me my faulty memory. I was under the impression that the pipeline was something like |
.../src/main/java/net/fabricmc/fabric/mixin/recipe/cooking/AbstractFurnaceBlockEntityMixin.java
Outdated
Show resolved
Hide resolved
...-v1/src/main/java/net/fabricmc/fabric/mixin/recipe/cooking/CookingRecipeSerializerMixin.java
Outdated
Show resolved
Hide resolved
`incrementByRecipe` changed from a `WrapOperation` to a `ModifyArg`. `addAlternativeCodec` changed from a `WrapOperation` to a `ModifyReceiver`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Datagen support is needed
- The tests should be improved, have a look at adding some game or unit tests to ensure that vanilla behaviour is presevered. (let me know if you need help with this)
Generally we would prefix it with fabric:
) | ||
) | ||
private static Codec<? extends ItemStack> addAlternativeCodec(Codec<? extends ItemStack> originalCodec, String fieldName) { | ||
return Codecs.alternatively( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a link for easy copy :DDDD neoforged/NeoForge#300
Alters the processing of cooking recipes so that they accept the following JSON formats for deserialising recipe results:
Vanilla
Additional (Added by this pull request)
Serialisation of recipe results is altered from the vanilla format to the additional object format.
There has been fruitful discussion on NBT recipe additions in the following thread:
Add result NBT and count to crafting and smelting recipes
This modification adds the Mojang crafting result Codec to the cooking result deserialisation Codec. Should a Codec that also accepts NBT be produced for crafting, it should be a simple matter to edit
CookingRecipeSerializerMixin
to use that instead. This should give you NBT smithing recipes for (nearly, see below) free.Suggested modifications should NBT recipes be enabled:
(Yarn names)
canAcceptRecipeOutput
inAbstractFurnaceBlockEntity.class
does not check NBT data when checking whether a recipe is valid. In order to avoid unintended gain/loss of NBT data for recipes, this function would need to check NBT of the outputs too.