Add createSmithingTransformRecipe method on RecipeGenerator #4701
Conversation
| * limitations under the License. | ||
| */ | ||
|
|
||
| package net.fabricmc.fabric.impl.datagen.smithing; |
There was a problem hiding this comment.
pretty sure this should be an api class, as otherwise, all the methods added are implementation methods
| * limitations under the License. | ||
| */ | ||
|
|
||
| package net.fabricmc.fabric.impl.datagen.smithing; |
| import net.minecraft.recipe.Ingredient; | ||
| import net.minecraft.recipe.book.RecipeCategory; | ||
|
|
||
| public interface SmithingTransformRecipeCreator { |
There was a problem hiding this comment.
in fabric api, we typically forgo the practice of naming injected interfaces after their purpose. usually it's something like FabricXX to allow the injected interface to remain once the functionality changes. I would guess this should be named FabricRecipeGenerator
There was a problem hiding this comment.
thank you; i moved the interfaces to the api package and renamed them to match conventions
|
|
||
| import net.minecraft.component.ComponentChanges; | ||
|
|
||
| public interface SmithingTransformParameters { |
There was a problem hiding this comment.
probably should be FabricSmithingTransformRecipeJsonBuilder
|
|
||
| @Shadow | ||
| public static String getItemPath(ItemConvertible item) { | ||
| return null; |
There was a problem hiding this comment.
Nit: usually the convention is for these to throw?
There was a problem hiding this comment.
i forgot to clear out the Shadows after switching my implementation from an offerRecipe-type method to a createRecipe one, sorry
| import net.minecraft.recipe.Ingredient; | ||
| import net.minecraft.recipe.book.RecipeCategory; | ||
|
|
||
| public interface FabricRecipeGenerator { |
There was a problem hiding this comment.
This could use a short doc comment (the methods in this itf don't really need it since they're duplicates of the vanilla ones)
|
|
||
| import net.minecraft.component.ComponentChanges; | ||
|
|
||
| public interface FabricSmithingTransformRecipeJsonBuilder { |
| import net.fabricmc.fabric.api.datagen.v1.smithing.FabricRecipeGenerator; | ||
|
|
||
| @Mixin(RecipeGenerator.class) | ||
| public abstract class RecipeGeneratorMixin implements FabricRecipeGenerator { |
There was a problem hiding this comment.
| public abstract class RecipeGeneratorMixin implements FabricRecipeGenerator { | |
| abstract class RecipeGeneratorMixin implements FabricRecipeGenerator { |
| import net.fabricmc.fabric.api.datagen.v1.smithing.FabricSmithingTransformRecipeJsonBuilder; | ||
|
|
||
| @Mixin(SmithingTransformRecipeJsonBuilder.class) | ||
| public class SmithingTransformRecipeJsonBuilderMixin implements FabricSmithingTransformRecipeJsonBuilder { |
There was a problem hiding this comment.
| public class SmithingTransformRecipeJsonBuilderMixin implements FabricSmithingTransformRecipeJsonBuilder { | |
| class SmithingTransformRecipeJsonBuilderMixin implements FabricSmithingTransformRecipeJsonBuilder { |
| import net.minecraft.recipe.book.RecipeCategory; | ||
|
|
||
| public interface FabricRecipeGenerator { | ||
| default SmithingTransformRecipeJsonBuilder createSmithingTransformRecipe(Ingredient template, Ingredient input, Ingredient addition, RecipeCategory category, Item result, int count, @Nullable ComponentChanges componentChanges) { |
There was a problem hiding this comment.
I wonder if it'd be better to just have one method with an ItemStack result parameter instead of all the overloads 🤔
This is probably fine too, but a bit messy with the long param lists.
There was a problem hiding this comment.
implemented; i still kept some overloads for ingredient/item but switched to itemstack for the result (aside from the base method, since its probably better to leave specifying them individually as an option)
| "has_the_recipe": { | ||
| "trigger": "minecraft:recipe_unlocked", | ||
| "conditions": { | ||
| "recipe": "minecraft:stick_smithing" |
There was a problem hiding this comment.
Nit: the recipe key uses the wrong namespace since it ends up being wrong in here as well (use the RegistryKey<Recipe> overload instead of the String overload of offerTo).
I think the string overload should be fixed on FAPI level to give the correct ns, but it's out of scope for this PR and quite possibly annoying to implement with mixins 😄
|
seems like this is read for review again :) |
there's no vanilla method for generating a Smithing Table recipe that modifies the components of an item, so this PR adds an interface onto the class that adds this functionality