Add BukkitAdapter.adapt method that returns a Bukkit BlockState
Intelli opened this issue ยท 6 comments
Is your feature request related to a problem? Please describe.
When working with the WorldEdit API, it's not possible to convert data into to a Bukkit BlockState without making a NMS call. This results in significant performance overhead when relying on the available methods in the Bukkit API to generate a BlockState.
Describe the solution you'd like
A BukkitAdapter.adapt method that returns a Bukkit BlockState
Craftbukkit has available methods available such as the following:
BlockState blockState = new CraftBlockState(block);
Describe alternatives you've considered
When working with the custom wrapped WorldEdit objects that are returned when working with the WorldEdit API, most necessary information can be inferred from just the Material and BlockData information.
However, BlockState usage is deeply engrained throughout the Bukkit API, as well as my own code, and refactoring to the point where BlockStates could be ignored isn't something that make sense just for the purpose of boosting WorldEdit performance (when utilizing the WE API).
Unfortunately it's not easy for us to make a BlockState
, as it requires world and position information as well. We'd have to add a parameter to the adapt
method to create it, e.g. adapt(BlockStateHolder, (worldedit)Location)
, and it would only work properly in the presence of a BukkitAdapter, otherwise it would likely have to throw an exception.
I personally don't know the tradeoffs, @wizjany should probably weigh in, but it seems extremely awkward for us to add, and it probably wouldn't be any better than your NMS call, which you claim to have "significant performance overhead".
BlockState actually doesn't require valid world or position information (although it can certainly hold it).
You can check how Craftbukkit handles it here:
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/block/CraftBlockState.java
That is:
public CraftBlockState(Material material) {
world = null;
data = CraftMagicNumbers.getBlock(material).getBlockData();
chunk = null;
position = BlockPosition.ZERO;
}
As well, CoreProtect doesn't utilize any NMS in the code, so I can't utilize these methods myself.
What she said basically. Even in the case of virtual (isPlaced == false) BlockStates, there's no way to convert an IBlockData into a CraftBlockState cleanly. Our reflection wouldn't be any less overhead than yours.
edit: (notably, world/chunk/pos are all final, so we'd need to construct via Material
ctor and then reflectively change data
)
edit2: actually looks like there's a setter on Data so we're back to square one.
edit3: i'd also note that that route doesn't give access to the TE snapshots anyway? what's the need for these? BlockData already has property stuff if that's what you rely on. if you need TE stuff...that's not available via CraftBlockState either?
I second the question on where you see this being used. You say "BlockState usage is deeply engrained throughout the Bukkit API", but looking at usages of BlockState
, it mostly appears in events fired by Bukkit, which I suppose you could be wanting to emulate, but it's certainly a much rarer usage than BlockData
.
Most of the data here is only accessible through the Bukkit API via a BlockState:
https://hub.spigotmc.org/javadocs/spigot/org/bukkit/block/package-summary.html
e.g. We use a BlockState to read the command from a command block. That information isn't available from BlockData.
Having all the valid meta information set for a converted BlockState via the WorldEdit API would be unimportant. The use case would be for passing it through our many layers of (async) data processing.