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 Throw EntityChangeBlockEvent for BrushableBlockEntity#brush #12133

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Y2Kwastaken
Copy link
Contributor

@Y2Kwastaken Y2Kwastaken commented Feb 17, 2025

This Pull Request adds calls for EntityChangeBlockEvent for BrushableBlockEntities. This allows for the increased tracking described in #12132.

This warrants further testing due to the fact that the point where the loot table is unpacked at could cause issues, however, I was unable to notice anything significant during my testing. Input for this aspect is appreciated.

@Y2Kwastaken Y2Kwastaken requested a review from a team as a code owner February 17, 2025 02:37
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.

The event calls are too late right now.
Right now, the block would drop its content even if people cancelled the change event.
It would also unpack its loottable, which, that modifies the block.

We need to move the unpacking down, dropContent calls it so, I think we can just move it right before the block set when a completion state changed (needs testing).
But yea, cancelling these events should maintain the block in the same state (not drop items/unpack loot tables).

@Y2Kwastaken
Copy link
Contributor Author

It would also unpack its loottable, which, that modifies the block.

Ran into this now the loot should only be unpacked after the first permitted brush Cancelling the event should not persist the state.
image

We need to move the unpacking down, dropContent calls

I moved these down as well and it seems to be working as intended to me.

I did some more playing around with the event and wasn't able to change the state after cancelling as intended using the vanilla brushing behavior. The patch needs a bit of cleaning up, but I'll leave it as is for now to see if this is what we will go with before I do that.

@lynxplay
Copy link
Contributor

Mhmmm, I did not even think about the brushCount, good catch.
This makes this entire block a lot more difficult, but yea, I think this is the way to go forward (shuffling).
The client should be perfectly fine and we still unpack the loottable on the first brush (the first brush count moves from 0 to 1 in completion stage)

Event is still not fully cancelling the "last" and final brush tho, the game event is still called and the brushCount is not reset either.

@lynxplay lynxplay force-pushed the fixes/12132 branch 2 times, most recently from 328a870 to c614a20 Compare February 17, 2025 23:59
@lynxplay
Copy link
Contributor

769de10 might be a separate solution, not gonna push over yet because I am too tired.
Will review tomorrow.

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

Successfully merging this pull request may close these issues.

2 participants