Conversation
|
I'll add the stuff back for remapping |
There was a problem hiding this comment.
Pull request overview
This PR switches Bedrock block runtime IDs from sequential (palette-order) IDs to hashed block network IDs, aiming to reduce reliance on version-specific palettes and improve client-side caching behavior.
Changes:
- Enable hashed block network IDs in the Bedrock StartGame packet.
- Replace palette-order runtime ID handling with hashed
GeyserBedrockBlockruntime IDs and adjust block registry structures accordingly. - Update several call sites that assumed palette-order runtime IDs (e.g., item/block mappings and entity metadata).
Reviewed changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/resources/bedrock/block_palette.1_21_130.nbt | Updates a versioned palette resource (appears less central now that hashed IDs are enabled). |
| core/src/main/java/org/geysermc/geyser/translator/sound/block/ButtonSoundInteractionTranslator.java | Removes an unused import. |
| core/src/main/java/org/geysermc/geyser/translator/level/block/entity/StructureBlockBlockEntityTranslator.java | Minor formatting-only change. |
| core/src/main/java/org/geysermc/geyser/session/GeyserSession.java | Enables blockNetworkIdsHashed in StartGame. |
| core/src/main/java/org/geysermc/geyser/registry/type/ItemMappings.java | Compares block definitions by runtime ID rather than object identity. |
| core/src/main/java/org/geysermc/geyser/registry/type/GeyserBedrockBlock.java | Implements hashed runtime IDs (FNV-1a) from canonicalized block state NBT. |
| core/src/main/java/org/geysermc/geyser/registry/type/BlockMappings.java | Reworks runtime ID lookup structures; switches item frame detection to hashed runtime IDs. |
| core/src/main/java/org/geysermc/geyser/registry/populator/ItemRegistryPopulator.java | Uses global (non-versioned) block mappings; iterates new runtime map structure. |
| core/src/main/java/org/geysermc/geyser/registry/populator/CreativeItemRegistryPopulator.java | Uses global (non-versioned) block mappings. |
| core/src/main/java/org/geysermc/geyser/registry/populator/BlockRegistryPopulator.java | Builds hashed runtime ID map and adjusts population logic accordingly. |
| core/src/main/java/org/geysermc/geyser/registry/BlockRegistries.java | Changes block mappings registry from versioned to simple (single mapping). |
| core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java | Uses global block mappings instead of per-protocol block mappings. |
| core/src/main/java/org/geysermc/geyser/entity/type/living/monster/PiglinEntity.java | Switches metadata usage from BLOCK to VARIANT for animation state. |
| core/src/main/java/org/geysermc/geyser/entity/type/living/monster/BasePiglinEntity.java | Switches metadata usage from BLOCK to VARIANT for animation state. |
| core/src/main/java/org/geysermc/geyser/entity/type/ItemFrameEntity.java | Constructs item frame block definitions directly with hashed IDs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| return this.stateDefinitionMap.get(tag); | ||
| return new GeyserBedrockBlock(tag); |
There was a problem hiding this comment.
getDefinition(NbtMap tag) now always returns a new GeyserBedrockBlock instance and does not consult bedrockRuntimeMap. This makes the method behave differently from getDefinition(int) and also means isRegistered() will be false for values returned here. Consider computing the runtime ID from the tag and returning the canonical instance from bedrockRuntimeMap (or returning null if it isn't present) to keep registry semantics consistent and avoid repeated hashing/allocations.
| return new GeyserBedrockBlock(tag); | |
| // Construct a temporary block to determine the runtime ID, then | |
| // return the canonical instance from the registry if present. | |
| GeyserBedrockBlock tempBlock = new GeyserBedrockBlock(tag); | |
| return bedrockRuntimeMap.get(tempBlock.getRuntimeId()); |
| NbtMap bedrockTag = buildBedrockState(blockState, entry); | ||
|
|
||
| GeyserBedrockBlock vanillaBedrockDefinition = blockStateOrderedMap.get(bedrockTag); | ||
| GeyserBedrockBlock vanillaBedrockDefinition = new GeyserBedrockBlock(bedrockTag); | ||
|
|
||
| GeyserBedrockBlock bedrockDefinition; |
There was a problem hiding this comment.
With hashed block network IDs, bedrockDefinition.getRuntimeId() is no longer a dense 0..N index. In this method there's still logic later on that uses getRuntimeId() to index into blockStates when populating flowerPotBlocks, which will now be out-of-range/negative and can break registry population or store the wrong tag. That lookup should be replaced with a map-based lookup (e.g., using the computed tag itself or bedrockRuntimeMap.get(runtimeId)), not a list index.
| Int2ObjectMap<GeyserBedrockBlock> bedrockRuntimeMap = new Int2ObjectOpenHashMap<>(blockStates.size()); | ||
| for (NbtMap tag : blockStates) { | ||
| GeyserBedrockBlock block = new GeyserBedrockBlock(tag); | ||
| bedrockRuntimeMap.put(block.getRuntimeId(), block); |
There was a problem hiding this comment.
bedrockRuntimeMap.put(block.getRuntimeId(), block) will silently overwrite if two distinct block states hash to the same runtime ID. Since these IDs are now computed hashes (not sequential), it would be safer to detect collisions (e.g., check the previous value returned by put, and if present and the state differs, throw/log) to avoid hard-to-debug client desyncs.
| bedrockRuntimeMap.put(block.getRuntimeId(), block); | |
| GeyserBedrockBlock previous = bedrockRuntimeMap.put(block.getRuntimeId(), block); | |
| if (previous != null) { | |
| GeyserImpl.getInstance().getLogger().error( | |
| "Detected Bedrock runtime ID collision for ID {}. Previous block: {}, New block: {}", | |
| block.getRuntimeId(), previous, block | |
| ); | |
| throw new IllegalStateException("Bedrock runtime ID collision detected for ID " + block.getRuntimeId()); | |
| } |
This PR replaces the incremental network ids with hashed block network ids, useful for more compact client caching then persistent palettes, and theoretically not requiring to have a palette for each version and some reduced mapping.