-
Notifications
You must be signed in to change notification settings - Fork 560
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
Recipe Rewrite (API Only) #4256
base: master
Are you sure you want to change the base?
Recipe Rewrite (API Only) #4256
Conversation
Your Pull Request was automatically labelled as: "🔧 API" |
Slimefun preview buildA Slimefun preview build is available for testing! https://preview-builds.walshy.dev/download/Slimefun/4256/49449f71
|
I'm going to throw together a showcase addon sometime next week to facilitate testing since I don't want to migrate any Slimefun stuff in 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.
Some questions and comments my side but very excited for this, thank you!
@@ -0,0 +1,254 @@ | |||
# 2. Recipe rewrite |
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.
initial review is just on the adr - thank you for this by the way, it makes large changes like this much better :)
docs/adr/0002-recipe-rewrite.md
Outdated
On the first server tick, all recipes in the `plugins/Slimefun/recipes` folder | ||
are read and added to the `RecipeService`, removing all recipes with the | ||
same filename. This is why recipes should ideally be *defined* in JSON, | ||
to prevent unnecessary work. |
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.
err interesting...
any reason to not just make plugins register their recipes? avoids the whole 1 tick thing.
Also ideally we just don't ever depend on a filename, that's not great.
RecipeService.registerRecipes(Path.of(getPluginFolder(), "recipes", "recipes.json"));
(this is reading from addon folder which i also think is fine)
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 wanted all recipes to be in a single place for the convenience of server owners; like with Items.yml
With how custom recipes now work, they will have to be loaded on the first tick too since they take precedence over all other addons' recipes
|
||
### Stage 4 | ||
|
||
On server shutdown (or `/sf recipe save`), **all** recipes are saved to disk. |
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.
Does this include ones where addons register them without a file? If so, why?
I like owners being able to change but I'd also like to not force that to be the case.
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.
If an addon registers a new recipe in code it will have to provide a filename, otherwise we wouldn't know where to save it and server owners would have no way to override it. Also disallowing owners from overriding a recipe doesn't do much since they can fork the addon and change it there
Co-authored-by: Daniel Walsh <[email protected]>
Co-authored-by: Daniel Walsh <[email protected]>
Co-authored-by: Daniel Walsh <[email protected]>
…ck/Slimefun4 into api/recipe-rewrite-4
@WalshyDev I updated the ADR with most of your review For custom server recipes, that design was pretty confusing, so I replaced it with one where developers' recipes are in Regarding your comments about recipe filenames -- can you elaborate on why we can't rely on filenames for recipes? It makes overriding them with custom recipes easier. Afaik resource packs also use paths/filenames to override certain resources Lmk if I missed any parts |
Description
The long awaited recipe rewrite (supersedes #4093)
Proposed changes
In short, adds true shaped/shapeless crafting, tagged inputs, and json (de)serialization (custom recipes)
Full ADR here
Related Issues (if applicable)
N/A
Checklist
Nonnull
andNullable
annotations to my methods to indicate their behaviour for null values