-
Notifications
You must be signed in to change notification settings - Fork 105
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
Alloy Smelter recipe inheritance rewrite #812
Conversation
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 this, I'm happy with how it looks and works.
Just a couple of comments for you before we can merge!
...-machines/src/main/java/com/enderio/machines/common/blockentity/AlloySmelterBlockEntity.java
Outdated
Show resolved
Hide resolved
enderio-machines/src/main/java/com/enderio/machines/common/recipe/AlloySmeltingRecipe.java
Outdated
Show resolved
Hide resolved
7d54f28
to
3a1c0d8
Compare
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.
Looking good to me - happy to merge if you are :) Cheers!
Yup, go ahead. Thank you. |
Description
This pull request introduces a rewrite to the vanilla smelting recipe inheritance of the Alloy Smelter.
The current implementation leaves no room for customization. Modpack authors are not able to filter inherited recipes properly. The KubeJS EnderIO add-on made this possible but it's very hacky because the synchronization between server and client has to be handled via a dummy recipe type because of timing issues. Filtered recipes had to be synced to the client to hide them from JEI.
The new approach uses the
RecipeManager
directly. It wraps existingSmeltingRecipe
s asAlloySmeltingRecipe
s upon loading and injects them back into the manager. This way, the inherited recipes have the same recipe type as normal alloy smelting recipes and are handled through the default serializer. This has the benefit of automatic network synchronization. To properly reflect the origin of the recipe, the alloy smelting recipe type received a new flag which can be used to change behavior between alloying and smelting recipes. The recipe ID of inherited smelting recipes was also changed. It uses the EnderIO namespace and a path prefix which I decided to usesmelting/
for (feel free to change this).Because the recipes are properly synchronized, the JEI recipe category for the Alloy Smelter was modified and no longer shows all vanilla smelting recipes. Additionally, the integration endpoint to filter alloy smelting recipes is no longer required and was removed. The
RecipeManager
inject happens early enough so mods like KubeJS or CraftTweaker are now capable of modifying the inherited recipes or removing them completely. The recipe ID change allows other mods to easily target inherited recipes now.If you want to split the recipe generation logic from the mixin class for better structure, feel free to.
A few technical things:
To allow modifications to the recipe loading, a
RecipeManager
mixin was required. Currently, EnderIO only uses mixins in the base module. However, since this change is machine-related, I decided to make a new package within the machine module and load the required mixins from there. Feel free to change this if you want a different structure.A second mixin was required for the
AbstractCookingRecipe
. It's an accessor mixin that allows grabbing the hidden fields from the class. If you are questioning, why I don't grab the required elements via the methods in theSmeltingRecipe
class, you have a good point. Since the inject in theRecipeManager
holds an instance of theSmeltingRecipe
and theRecipeManager
also holds an instance of registry access, it should be possible to obtain the result of theSmeltingRecipe
. However, for some reason, the lambda inject needs to be static and this prevents using the local instance of the registry access. I thought this was a bug in the Minecraft Dev plugin but the bytecode shows that the lambda is indeed statically executed. This may be a bug in Mixin itself because it's definitely possible to have non-static lambda mixins but since the library is not well maintained, I didn't bother to file an issue report. I'd like to avoid a second mixin but I didn't find another option.Breaking Changes
The only breaking change I introduced is the removal of the integration endpoint to filter smelting recipes. But since my add-on was the only mod using this endpoint, I don't think this should be considered breaking.
Checklist