-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Implement unregister functions in CraftingManager #5809
base: minor-next
Are you sure you want to change the base?
Implement unregister functions in CraftingManager #5809
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.
Code looks OK, although I haven't tested this myself.
Things to watch out for:
|
Does this PR require any further development action or is it just testing? |
no, I think the PR code is probably okay. |
Fixed #5286 |
$edited = true; | ||
} | ||
|
||
if($edited){ |
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.
$changed
would be more consistent with code convention
$edited = true; | ||
} | ||
|
||
if($edited){ |
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.
same again here
The code in the PR looks functional. The thing I'm dubious about is that this code expects plugin devs will have to get the exact instance of the recipe they want to unregister, in order to unregister it. I'm not sure this is the best way to go about it. May well result in some painful headaches if someone does something like this: $craftingManager->unregisterShapedRecipe(new ShapedRecipe(
[
"WWW",
"W W",
"WWW",
],
[
"W" => new ExactRecipeIngredient(VanillaBlocks::OAK_PLANKS()->asItem()),
],
[VanillaBlocks::CHEST()->asItem()]
)); This code looks fine, but it would not produce the intended result. Basically, I think we need a better way to compare recipes before implementing this. |
Introduction
Actually, if we want to delete or replace a recipe, we have to go through Reflection.
This PR provides a means of deleting a recipe to make it disappear, or replacing it by register it immediately afterwards.
There's no need to provide any functions other than this one, as the rest does the job:
Relevant issues
Changes
API changes
Add the following functions to the
CraftingManager
class:unregisterShapedRecipe
unregisterShapelessRecipe
unregisterPotionTypeRecipe
unregisterPotionContainerChangeRecipe
Add the following function to the
FurnaceRecipeManager
class:unregister
Backwards compatibility
I didn't want to do it directly, to get an opinion, but the observers should be renamed. It now only listens to the registration but also to the deletion.
Follow-up
Tests
Will remove all craft for Crafting table