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

Add startingBrewTime #11406

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

notTamion
Copy link
Contributor

This fixes the issue that when setting a different Total brew time in BrewingStartEvent the arrow in the brewing stand menu will either start of already full to a certain amount or not filling up for some time depending on whether you set the brew time to under or over 400

@notTamion notTamion requested a review from a team as a code owner September 15, 2024 18:10
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Any reason this cannot live in the block entities data access?

But also I am not too keen on this patch in general given it "fixes" a visual bug by hiding it behind a single value (the brew time)
IMO it would make more sense to store a separate field in the brewing stand to configure the total visual brew time.
E.g. right now a plugin calling setBrewingTime to something above 400 will "lock" the stand to that "startBrewTime" value.

@notTamion
Copy link
Contributor Author

I didn't put it on the data access as we would be sending that extra data to the client (i believe there are some size checks in there so that might break something).

Second point: Do you just want to make the value we calculate when sending the data to the client an extra variable? I don't see how that would be beneficial. Could you please elaborate on that?

Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Looking at this further, hijacking the slot setup is not gonna work for most implementations sadly, e.g. opening a custom inventory of that type currently just has hardcoded values for this. So I guess ctor overload it is or some fuckery with instanceOf in the ctor for ContainerData.

So to that degree I guess we need to force it being passed as an actual variable to a constructor/overload both.

Is starting really a good description for what this value does?
I mean, from a technical perspective, yea, but maybe something along the lines of "total" or "required"?

@notTamion
Copy link
Contributor Author

Not sure if this fixes it? Don't have time to test rn.

@lynxplay
Copy link
Contributor

Personal note keeping after talking it over with MM.
Create new BrewingSimpleContainerData subtype that extends SimpleContainerData but inits itself with value 2 set to 400.
Pass that in simple ctro and CraftContainer. Set both BrewingBLockEntityData and BrewingSimpleContainerData to count 3, keep the current "new containerData() { }` in setDataSlots to "translate" back to count 2 and do the conversion,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

2 participants