More efficient multi block updates#6307
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets the bulk Java ClientboundSectionBlocksUpdatePacket translation path to reduce packet fan-out by batching block changes into a single Bedrock UpdateSubChunkBlocksPacket, with some associated chunk-cache/prediction handling refactors.
Changes:
- Reworks section multi-block update translation to build/send one
UpdateSubChunkBlocksPacketinstead of many per-block packets. - Adds chunk-section access helper in
ChunkCacheand adjusts how chunk-cache block updates are applied. - Refactors prediction removal logic in
WorldCache(addsremovePrediction).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| core/src/main/java/org/geysermc/geyser/translator/protocol/java/level/JavaSectionBlocksUpdateTranslator.java | Batches section updates into a single Bedrock subchunk update packet and changes cache/prediction handling. |
| core/src/main/java/org/geysermc/geyser/translator/protocol/java/JavaRecipeBookAddTranslator.java | Comments out the translator annotation (disables translator registration). |
| core/src/main/java/org/geysermc/geyser/session/cache/WorldCache.java | Simplifies prediction removal and introduces removePrediction. |
| core/src/main/java/org/geysermc/geyser/session/cache/ChunkCache.java | Adds getChunkSection helper and changes updateBlock to use it (affecting palette allocation behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
valaphee
force-pushed
the
multi-block-change
branch
from
e7be80c to
8d8978f
Compare
valaphee
force-pushed
the
multi-block-change
branch
from
8d8978f to
6aecb95
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
valaphee
force-pushed
the
multi-block-change
branch
from
f9ceb77 to
6f3154a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
also found this CloudburstMC/Protocol#333 but the sub chunk position has not really an effect |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BlockState blockState = BlockState.of(entry.getBlock()); | ||
| if (blockState.is(Blocks.AIR)) { | ||
| ItemFrameEntity itemFrameEntity = ItemFrameEntity.getItemFrameEntity(session, entry.getPosition()); | ||
| if (itemFrameEntity != null) { // Item frame is still present and no block overrides that; refresh it | ||
| itemFrameEntity.updateBlock(true); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // Some block may have special handling, keep it that way | ||
| if (!(blockState.block().getClass().equals(Block.class))) { | ||
| blockState.block().updateBlock(session, blockState, entry.getPosition()); | ||
| continue; | ||
| } | ||
|
|
||
| // Skull is gone | ||
| session.getSkullCache().removeSkull(entry.getPosition()); |
There was a problem hiding this comment.
I understand why you didn't use ChunkUtils#updateBlockClientSide as this would send UpdateBlockPacket for each changed block. But if we ever add something extra to Block#updateBlock (other than checkForEmptySkull) then we'd need to think about adding it here too.
This would probably require a larger refactor, so I think it's fine for now.
Not sure what other people think.
There was a problem hiding this comment.
Yea its only called if its specialized, for anything else we would have to check both locations, could add comments
There was a problem hiding this comment.
I'd be fine with a comment in Block#updateBlock like: any changes you do here, add them to this translator too.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Some block may have special handling, keep it that way | ||
| if (!(blockState.block().getClass().equals(Block.class))) { | ||
| blockState.block().updateBlock(session, blockState, entry.getPosition()); | ||
| continue; |
There was a problem hiding this comment.
The specialization check !(blockState.block().getClass().equals(Block.class)) treats all Block subclasses as requiring per-block updateBlock(...) handling, even when the subclass does not override updateBlock (e.g., BannerBlock just adds dyeColor() but inherits Block.updateBlock). That means those blocks will still fan out into individual UpdateBlockPackets and reduce the effectiveness of batching. Consider switching to an explicit marker (interface/boolean method) for blocks that truly need special update behavior, and batch everything else.
This PR aims to improve multi block updates, by not fanning out into up to 4096 block update packets and instead using a single UpdateSubChunkBlocksPacket.
Solves #6192