Skip to content

Conversation

SchnTgaiSpock
Copy link
Contributor

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

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@github-actions github-actions bot added the 🔧 API This Pull Request introduces API changes. label Nov 4, 2024
Copy link
Contributor

github-actions bot commented Nov 4, 2024

Your Pull Request was automatically labelled as: "🔧 API"
Thank you for contributing to this project! ❤️

Copy link
Contributor

github-actions bot commented Nov 4, 2024

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 49449f7

https://preview-builds.walshy.dev/download/Slimefun/4256/49449f71

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

@SchnTgaiSpock
Copy link
Contributor Author

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.

Copy link
Member

@WalshyDev WalshyDev left a 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
Copy link
Member

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 :)

Comment on lines 214 to 217
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.
Copy link
Member

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)

Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

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

SchnTgaiSpock and others added 6 commits November 10, 2024 15:55
Co-authored-by: Daniel Walsh <walshydev@gmail.com>
Co-authored-by: Daniel Walsh <walshydev@gmail.com>
Co-authored-by: Daniel Walsh <walshydev@gmail.com>
@SchnTgaiSpock
Copy link
Contributor Author

@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 plugins/Slimefun/recipes as usual, and custom server recipes are in plugins/Slimefun/recipes/custom, which takes precedent

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 API This Pull Request introduces API changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants