Skip to content

add VaultStateChangeEvent#1621

Closed
Grabsky wants to merge 2 commits into
PurpurMC:ver/1.21.4from
Grabsky:feat/VaultStateChangeEvent
Closed

add VaultStateChangeEvent#1621
Grabsky wants to merge 2 commits into
PurpurMC:ver/1.21.4from
Grabsky:feat/VaultStateChangeEvent

Conversation

@Grabsky

@Grabsky Grabsky commented Dec 2, 2024

Copy link
Copy Markdown

Simplified version of PaperMC/Paper#11679. May extend this API further in the future but it is already good enough for use-case described in the discussion thread.

I don't know if I put the implementation in the right place because currently - I'm afraid - the event is also called when Vault.State is changed via API and doing so inside the event can create an indefinite loop if handled incorrectly. I'm not sure what should I do about this.

@Onako2

Onako2 commented Dec 2, 2024

Copy link
Copy Markdown
Contributor

Is there a reason behind not creating a PR at Paper?
Just asking because Paper is more wide-spread and would profit more people

@Grabsky

Grabsky commented Dec 2, 2024

Copy link
Copy Markdown
Author

@Onako2: Is there a reason behind not creating a PR at Paper?
Just asking because Paper is more wide-spread and would profit more people

Linked discussion have not received any feedback ever since it's creation and because Paper is generally bigger, I think this PR might have taken much longer than I'd like to. I agree however that more people could benefit from it but I'm currently unable to implement the full suggestion. When I'm a bit more familiar with how patches and such work, I might PR the full version to the Paper instead (assuming nobody else would get to it before me!). Just wanted to start with Purpur, as it seems smaller and more open for small changes like this one.

@Grabsky Grabsky changed the base branch from ver/1.21.3 to ver/1.21.4 December 6, 2024 18:11
@Grabsky Grabsky force-pushed the feat/VaultStateChangeEvent branch from 1d1fc24 to 0e06eaf Compare December 13, 2024 09:07
@Grabsky Grabsky force-pushed the feat/VaultStateChangeEvent branch from 0e06eaf to 1680e5d Compare December 23, 2024 10:48
@granny

granny commented Jan 14, 2025

Copy link
Copy Markdown
Member

Hey! Sorry for the late response. This PR needs to be updated to account for the hard fork changes, which means you'd have to re-learn how to apply your change, to a degree. It's definitely a lot easier now, though.

I'd recommend pushing this PR to Paper. The maintainers are currently doing their best to stay on top of PRs, so this would be the perfect time to try and get this merged in by them.

@Grabsky

Grabsky commented Jan 17, 2025

Copy link
Copy Markdown
Author

I don't know when I will be able to work on that, so if anyone wants to take this PR to Paper, feel free to do so.
This would be very appreciated. 😃

Because of hard fork changes, only one per-file patch will be needed and event class can be added directly to the source.
Class to patch: net.minecraft.world.level.block.entity.vault.VaultBlockEntity


@Grabsky: I don't know if I put the implementation in the right place because currently - I'm afraid - the event is also called when Vault.State is changed via API and doing so inside the event can create an indefinite loop if handled incorrectly.

Ever since I wrote this, I looked at the implementation of the mentioned method and IIRC it is setting the property manually and not calling method modified by this PR. Correct me if I'm wrong.

@granny granny closed this Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants