Fix Block#getType returning STONE, ordered synchronous setBlockData, block-state properties, and bridge hot-path costs - #25
Merged
Conversation
…ta map getBlockData called the native bridge, then discarded the response and returned Material.STONE for every non-air block, so Block#getType() could never report the actual block. Feed the returned block state string through Bukkit.createBlockData, which already parses namespaced keys, property suffixes and legacy names. Also make PatchBukkitBlock's metadata map lazy: World#getBlockAt creates a fresh wrapper per call, so the eager HashMap was ~80 bytes of garbage per block read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: TRiLON <d.m.alexandris@gmail.com>
newData ran Class.forName("net.minecraft.SharedConstants") and Class.forName("org.bukkit.craftbukkit.block.data.CraftBlockData") unconditionally on every call. Neither class can exist in this process (PatchBukkit ships no NMS/OBC), so both were guaranteed ClassNotFoundExceptions - measured at roughly 10 microseconds per call, ~95% of the cost of Block#getType() - on the hottest path in the API (getType -> createBlockData). Removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: TRiLON <d.m.alexandris@gmail.com>
…gistrations world.rs: - get_block_data now returns block state properties (facing, half, waterlogged, ...) via Block::properties(state_id).to_props() instead of the bare block name. - set_block_data applies the [k=v,...] suffix through Block::from_properties instead of silently placing default_state. - set_block_data was fire-and-forget (runtime.spawn): Block#setType returned before the write landed and two writes to one position could land in either order. Bukkit's contract is synchronous ordered mutation; now uses the same block_in_place + block_on pattern other callbacks use, with a comment documenting the re-entrancy constraint for anyone bridging block-physics events later. - applyPhysics honored: NOTIFY_ALL vs NOTIFY_LISTENERS. events.rs: (plugin, event type) dedup guard in the registration callback itself, complementing the Java-side registeredBridgeEvents set from 43e65ca - a duplicate Pumpkin handler costs a full serialize + cross-thread round trip per event fire and re-invokes listeners that already ran. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: TRiLON <d.m.alexandris@gmail.com>
Signed-off-by: TRiLON <d.m.alexandris@gmail.com>
Member
|
Ig LGTM, Thanks |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Correctness and hot-path fixes on the block read/write path and event registration — complementing the j4rs→JNI and dedup work that just landed in 43e65ca (this branch is rebased on top of it; three further issues I'd found — the per-byte event marshalling, the response double-unwrap, and the
block_in_placearound the atomic health read — were already fixed there, so they're not in this PR).1.
Block#getType()could never return the actual blockPatchBukkitRegionAccessor#getBlockDatacalled the bridge, then discarded the response and returnedSTONEfor every non-air block:Now feeds
response.getBlockState()throughBukkit.createBlockData(String), which already handles namespaced keys, property suffixes and legacy names.2. Block-state properties survive the bridge
get_block_datareturned onlyblock.name— stairs lostfacing, doors losthalf. Now includes properties viaBlock::properties(state_id).to_props()→minecraft:oak_stairs[facing=north,half=top]. Symmetrically,set_block_dataparsed the string by splitting on[and placingdefault_state; it now applies the[k=v,...]suffix throughBlock::from_properties(...).to_state_id(...).3.
Block#setTypereturned before the block was setset_block_datawas fire-and-forget (runtime.spawn): read-after-write raced, and two writes to one position could land in either order. Bukkit's contract is synchronous, ordered mutation. Now uses the sameblock_in_place+block_onpattern other callbacks already use, and honorsapplyPhysics(NOTIFY_ALLvsNOTIFY_LISTENERS). A comment documents the re-entrancy constraint for anyone wiring block-physics events to the JVM later.4. ~10 µs of guaranteed
ClassNotFoundExceptions per block readPatchBukkitBlockData.newDataranClass.forName("net.minecraft.SharedConstants")andClass.forName("org.bukkit.craftbukkit.block.data.CraftBlockData")unconditionally on every call. Neither class can exist in this process (no NMS/OBC on the classpath), so both were guaranteed exceptions. Measured on JDK 21/25: ~10 µs pergetType()call — ~95% of its total cost (the entire FFI round trip is ~450 ns). Removed.5. Rust-side registration dedup (defense in depth)
Complements 43e65ca's Java-side
registeredBridgeEvents: a(plugin, event type)guard in the Rust registration callback itself, so a duplicate Pumpkin handler can't be created even by callers that bypassPatchBukkitEventManager(directHandlerListregistration, plugin reloads). Each duplicate handler costs a full serialize + cross-thread round trip per event fire and re-invokes listeners that already ran.6. Misc
PatchBukkitBlockallocated a metadataHashMapper instance —getBlockAt()creates a fresh wrapper per call, so every block read produced ~80 B of garbage. The map is now lazy.Verification
Developed in a sandbox without Maven Central or a nightly toolchain, so verification is compile-level and unit-level rather than a live server run — CI should be the final arbiter:
nightlyrelease binary; unmodified master compiles identically under the same harness (A/B baseline). Re-verified after rebasing onto a6ca90a.rustccheck andrustfmt --checkpass on the touched files. Every new API verified against current Pumpkin sources:Block::properties/to_props/from_properties/to_state_id(pumpkin-datageneratedblock.rs),BlockFlags::NOTIFY_LISTENERS(pumpkin-world/src/world.rs). A full offlinecargo checkwasn't possible.[], whitespace, missing prefix).Judgment calls worth a maintainer's eye: the sync
set_block_stateis a semantics change (fire-and-forget → blocking, matching Bukkit's contract); andgetBlockDataon an unloaded chunk now reports what Pumpkin returns for it rather than always STONE.🤖 Generated with Claude Code
https://claude.ai/code/session_01Nk1NUKr7NNGxaU3jW5Nmyz