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 Vault block API #12068

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

Add Vault block API #12068

wants to merge 13 commits into from

Conversation

Warriorrrr
Copy link
Member

@Warriorrrr Warriorrrr commented Feb 4, 2025

Supersedes #11159

Adds new API for vault blocks

Changes made compared to the previous PR:

  • Added some javadocs.
  • Removed itemsToEject methods, these items are only populated when a player is opening a vault, but these can already be changed with the block dispense loot event, and also can't think of any use case for changing items in the middle of ejecting.
  • Removed rewarded players methods with player instances.
  • Added API for state update time.
  • Made the VaultBlockEntity#serverData field public to be able to directly update the server data, without having to access the actual tile entity.

Co-authored-by: Boy <[email protected]>
@Warriorrrr Warriorrrr requested a review from a team as a code owner February 4, 2025 22:31
@Grabsky
Copy link

Grabsky commented Feb 5, 2025

May not be the scope of this PR, but perhaps these could also be addressed here?

Or would this rather be a separate PR?

@Warriorrrr
Copy link
Member Author

May not be the scope of this PR, but perhaps these could also be addressed here?

* [VaultUnlockEvent #11687](https://github.com/PaperMC/Paper/discussions/11687)

* [VaultStateChangeEvent #11679](https://github.com/PaperMC/Paper/discussions/11679)

Or would this rather be a separate PR?

My current scope is just the vault block, I'll change the PR name

@Warriorrrr Warriorrrr changed the title Add Vault API Add Vault block API Feb 5, 2025
* @apiNote Only the most recent 128 player UUIDs will be stored by vault blocks.
*/
@Unmodifiable
Set<UUID> getRewardedPlayers();
Copy link
Member Author

Choose a reason for hiding this comment

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

Would a Collection or SequencedCollection return type make more sense for this method, instead of guaranteeing a Set?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the question boils down if ever in the future it could make sense listing a player twice, doesn't it?
My take would be, that it doesn't make sense to use a general Collection, since the vault basically just keeps track of whom to not reward any time soon again, not a complete history.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually pretty unsure on this statement.
A set would be useful for insertion schematics, but the returned thing is unmodifiable/immutable anyway.
Consumers cannot do anything about the contents in the first place but calling contains which, that works on a collection just as well as on a set.
I don't see much benefit from this being a set, tho I'll chat it over with others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Awaiting final testing
Development

Successfully merging this pull request may close these issues.

6 participants