Add Vault Block API #11159
Add Vault Block API #11159
Conversation
|
I would also add methods accepting player UUIDs, so you can manage rewarded players when they are offline/you don't have a Player instance. It is done similarly for example for Wolf owners etc... |
patches/api/0483-Vault-API.patch
Outdated
| * @return the 'vault_state' value | ||
| */ | ||
| @NotNull | ||
| - State getTrialSpawnerState(); |
There was a problem hiding this comment.
This might break plugins. Maybe keep those old methods but deprecate them
There was a problem hiding this comment.
In my opinion this is a fine api break, considering this is clearly a wrong name and the methods have not been around for long and since the naming of these methods might cause confusion.
There was a problem hiding this comment.
While I don't hate the API break here, we cannot break ABI for spigot plugins as those will need to use said methods.
As such, this PR would have to address the conversion in commodore byte code rewriting.
There was a problem hiding this comment.
That part can be remove when pr is rebased. see https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/commits/8a9ecf29405887b9e58244cd5d87e8ca0a5970d4#src%2Fmain%2Fjava%2Forg%2Fbukkit%2Fblock%2Fdata%2Ftype%2FVault.java
patches/server/1045-Vault-API.patch
Outdated
| + return this.rewardedPlayers.contains(uniqueId); | ||
| + } | ||
| + | ||
| + public void addToRewardedPlayers(UUID uniqueId) { |
There was a problem hiding this comment.
A lot of AT can be removed if you use your new methods instead and just get the uuid from the player and to reduce the diff a bit you could just change the original method and make a new overload with player, same applies for the api.
patches/server/1045-Vault-API.patch
Outdated
| + } | ||
| + | ||
| + @Override | ||
| + public void setItemsToEject(final List<ItemStack> itemsToEject) { |
There was a problem hiding this comment.
This doesn't check empty item, so i can easily create a corrupted block entity
There was a problem hiding this comment.
Empty items seems to be checked internally and handled so null and AIR items should work fine ithink
Unless you want me to explicitly mark ItemStack as notnull here
There was a problem hiding this comment.
The codec doesn't support those empty item.
For reference here is the stacktrace when you save the world without interacting with the vault (or using the data command): https://pastes.dev/faeBdNZWcj
|
You should also check with requirePlaced/isPlaced to take in account unplaced blockstate for all methods that works on the block entity directly (and require a level to be set) and not the snapshot |
patches/api/0483-Vault-API.patch
Outdated
| * @return the 'vault_state' value | ||
| */ | ||
| @NotNull | ||
| - State getTrialSpawnerState(); |
There was a problem hiding this comment.
That part can be remove when pr is rebased. see https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/commits/8a9ecf29405887b9e58244cd5d87e8ca0a5970d4#src%2Fmain%2Fjava%2Forg%2Fbukkit%2Fblock%2Fdata%2Ftype%2FVault.java
|
merged onto master for 1.21.4 🙏 |
Adds bunch of basic API to get/set properties of vault blocks
Not finished but opening to get some feedback on structure and whatnot
The API Break with renaming of Vault-api methods get/setTrialSpawnerState -> get/setVaultState needs bytecode modification
If a contributor could sort that before merging this PR
All accessing of VaultServerData is done through TileEntity instead of a snapshot, due to snapshot always having a ServerLevel null
Therefore VaultServerData would always return null
Unsure if its a bug in my implementation or another issue