Skip to content
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

Brewing recipes #161

Open
wants to merge 28 commits into
base: 1.19.4
Choose a base branch
from
Open

Brewing recipes #161

wants to merge 28 commits into from

Conversation

Platymemo
Copy link
Contributor

@Platymemo Platymemo commented Aug 5, 2022

Allows the creation of brewing recipes in a similar fashion to other recipe types.

The three kinds of brewing recipes are quilt:potion_brewing, quilt:custom_potion_brewing, and quilt:potion_item_brewing. potion brewing and custom potion brewing are functionally similar, with the addition that custom potion brewing allows multiple status effects, much like the turtle master potion but without having to register a potion first. potion item brewing is how you transform a regular potion into a splash potion, etc.

This PR also de-hardcodes the potion slots in the brewing stand, having it check a tag, quilt:potions instead.

The base AbstractBrewingRecipe and supporting infrastructure are general enough that they should support a subclass providing recipes on arbitrary item stacks.

Additionally, this PR opens up a brewing stand fuel registry as an item content registry to allow modders and users to easily add new fuels or modify existing ones.

@Patbox
Copy link
Contributor

Patbox commented Aug 5, 2022

This will break with vanilla clients

@oliviathevampire
Copy link

This will break with vanilla clients

How does this break vanilla? And is it any better ways?

@Patbox
Copy link
Contributor

Patbox commented Aug 5, 2022

If a datapack (or otherwise server side mod) uses that feature, vanilla clients won't be able to join server, as recipe packet will contain recipe type it can't read. QSL and it's datapackable additions should be compatible with vanilla clients

@Platymemo
Copy link
Contributor Author

Platymemo commented Aug 5, 2022

As the recipe is only used serverside, if we can strip the recipes from being sent to a vanilla client this would work. But... that sounds very hacky.

I'm not familiar at all with Minecraft or Quilt's networking so I don't know if it's possible to detect vanilla clients and strip the recipes if so.

We could just strip them entirely from being sent to clients, but I wouldn't want to do that for recipe integrations in mods like EMI.

@Patbox
Copy link
Contributor

Patbox commented Aug 6, 2022

@Platymemo Platymemo marked this pull request as ready for review August 6, 2022 11:23
@Platymemo Platymemo requested a review from a team August 6, 2022 11:23
@Platymemo Platymemo requested a review from a team as a code owner August 6, 2022 11:23
Copy link
Member

@LambdAurora LambdAurora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that the builders extend an impl class, that's not possible.

@EnnuiL EnnuiL added enhancement New feature or request library: item Related to the item library. library: data Related to the data library. t: new api This adds a new API. labels Aug 6, 2022
Copy link
Contributor

@Leo40Git Leo40Git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far, just some Javadoc nitpicks...

Platymemo and others added 3 commits August 9, 2022 01:41
Co-authored-by: ADudeCalledLeo <7997354+Leo40Git@users.noreply.github.com>
Co-authored-by: ADudeCalledLeo <7997354+Leo40Git@users.noreply.github.com>
@Leo40Git
Copy link
Contributor

Leo40Git commented Aug 9, 2022

312b54e is already a separate PR! (#163)
Please revert it, as it will cause conflicts when 163 is merged.

nevermind I'm dumb

Copy link
Member

@OroArmor OroArmor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some package level docs, especially for the different recipe types and their differences would be nice. The rest of the API looks great.

@EnnuiL EnnuiL added s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. and removed enhancement New feature or request labels Sep 10, 2022
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good! There is just one spot that I feel like it could be improved, however, i'm not sure if it's possible, i might wanna do my own tests

Copy link
Member

@TheGlitch76 TheGlitch76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with this part of Minecraft, so a lot of these questions come from ignorance. Sorry!

# Conflicts:
#	library/data/recipe/build.gradle
#	library/data/recipe/src/main/resources/quilt_recipe.mixins.json
#	library/item/item_content_registry/src/main/java/org/quiltmc/qsl/item/content/registry/impl/ItemContentRegistriesClientInitializer.java
#	library/item/item_content_registry/src/main/java/org/quiltmc/qsl/item/content/registry/impl/ItemContentRegistriesInitializer.java
#	library/item/item_content_registry/src/testmod/resources/data/quilt/attachments/minecraft/item/brewing_fuel_time.json
@Platymemo Platymemo changed the base branch from 1.19 to 1.19.4 April 1, 2023 03:08
@EnnuiL
Copy link
Contributor

EnnuiL commented Apr 12, 2023

It looks like it's time for a re-reviewing of this PR

Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh dear, i was expecting to bikeshed the API itself a lot, but nope! i only have style complaints (with only one concern about registry stuff);
I am pretty satisfied with the API itself, so it gets an Ennui's Stamp of Approval

Copy link
Member

@OroArmor OroArmor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should get this into FCP soon


import org.quiltmc.qsl.recipe.impl.RecipeImpl;

// TODO Conditionally remove only for vanilla clients
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really need to be considered ASAP because of mods like EMI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #298 is something to keep an eye into, since it might help with that

@OroArmor
Copy link
Member

We should get this into FCP soon

4 months later...

@TheGlitch76 TheGlitch76 added s: outdated This pull request is outdated. and removed s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. labels Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library: data Related to the data library. library: item Related to the item library. s: outdated This pull request is outdated. t: new api This adds a new API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants