RFC: Add Player#sendBlockChanges(Map<Position, BlockState>)

[MeFisto94]

Is your feature request related to a problem?

I want to send virtual blocks (tile entities, here: Skulls/Player Heads) to the individual clients, without changing blocks on disk.
There is already the very promising experimental API Player#sendBlockChanges(Collection<BlockState>) and a Player#sendMultiBlockChange(Map<Position, BlockData>).

The problem however is that the latter is not sufficient for tile entities, only for simple blocks that require no special block state.
The former feels sufficient, however there is one major drawback: All Block States that can be constructed (virtually, Bukkit.createBlockState) refer to BlockPos.ZERO.

Describe the solution you'd like.

I'd like to see an additional overload of sendBlockChanges that takes a position that takes precedence over the location bound to the block state, but I'd also be open to other API suggestions, maybe there's already a resonable way of achieveing this?

Describe alternatives you've considered.

• Overriding the BlockPos on BlockState is not only "impossible" (final), but would also be massively confusing (as you probably can't just move the block state, you'd first need to relocate the block itself?)
• Trying to take a legit block state at that location and changing it only works if the block type was of the same material (or rather the right blockstate subclass) already
• Using something like ProtocolLib probably works, but I'd obviously prefer to use the paper api, especially if there's something very close available already

Other

I'm open for other solutions and I'd also be capable of providing the implementation of said API enhancement, provided it doesn't require deep tile entity knowledge that I don't have or anything.

[Owen1212055]

The underlying packet doesn’t support sending block entities with it. It’s ideal that you most likely send this separately.

But it could be argued having a combined one may be useful and could be bundled together

[MeFisto94]

Oh, so you're saying that the method that takes in a Collection<BlockState> is actually not supporting the block states and is only used as a convenience type for (Position, BlockData)? Then I'd argue that the method is even misleading and would deserve a javadoc hint at least.

I don't know how old that overload is already, but maybe it would even make sense to have a behavior change on that method, then? Because basically if I take a BlockState snapshot and would use that method to re-apply it, it would not really work, right?

[MeFisto94]

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: MeFisto94 <MeFisto94@users.noreply.github.com>
Date: Fri, 22 Mar 2024 01:11:48 +0100
Subject: [PATCH] Make Player#sendBlockChanges support actually updating the
 BlockEntity


diff --git a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java
index 206520f6f20b2e48b1eefdd4edb26510b88e4c92..c3776d5bb7fae624979ffc2f73bf266966acf81d 100644
--- a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java
+++ b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java
@@ -45,6 +45,7 @@ import net.minecraft.network.protocol.common.ClientboundResourcePackPopPacket;
 import net.minecraft.network.protocol.common.ClientboundResourcePackPushPacket;
 import net.minecraft.network.protocol.common.custom.CustomPacketPayload;
 import net.minecraft.network.protocol.game.ClientboundBlockDestructionPacket;
+import net.minecraft.network.protocol.game.ClientboundBlockEntityDataPacket;
 import net.minecraft.network.protocol.game.ClientboundBlockUpdatePacket;
 import net.minecraft.network.protocol.game.ClientboundClearTitlesPacket;
 import net.minecraft.network.protocol.game.ClientboundCustomChatCompletionsPacket;
@@ -91,6 +92,7 @@ import net.minecraft.world.food.FoodData;
 import net.minecraft.world.inventory.AbstractContainerMenu;
 import net.minecraft.world.level.GameType;
 import net.minecraft.world.level.block.Blocks;
+import net.minecraft.world.level.block.entity.BlockEntity;
 import net.minecraft.world.level.block.entity.SignBlockEntity;
 import net.minecraft.world.level.block.entity.SignText;
 import net.minecraft.world.level.border.BorderChangeListener;
@@ -914,6 +916,7 @@ public class CraftPlayer extends CraftHumanEntity implements Player {
         }
 
         Map<SectionPos, ChunkSectionChanges> changes = new HashMap<>();
+        List<ClientboundBlockEntityDataPacket> entityDataPackets = new ArrayList<>();
         for (BlockState state : blocks) {
             CraftBlockState cstate = (CraftBlockState) state;
             BlockPos blockPosition = cstate.getPosition();
@@ -926,6 +929,13 @@ public class CraftPlayer extends CraftHumanEntity implements Player {
 
             sectionChanges.positions().add(SectionPos.sectionRelativePos(blockPosition));
             sectionChanges.blockData().add(cstate.getHandle());
+
+            if (state instanceof CraftBlockEntityState<?> entityState) {
+                BlockEntity entity = BlockEntity.loadStatic(entityState.getPosition(), entityState.getHandle(), entityState.getSnapshotNBT());
+                if (entity != null) {
+                    entityDataPackets.add(ClientboundBlockEntityDataPacket.create(entity));
+                }
+            }
         }
 
         // Construct the packets using the data allocated above and send then to the players
@@ -934,6 +944,10 @@ public class CraftPlayer extends CraftHumanEntity implements Player {
             ClientboundSectionBlocksUpdatePacket packet = new ClientboundSectionBlocksUpdatePacket(entry.getKey(), chunkChanges.positions(), chunkChanges.blockData().toArray(net.minecraft.world.level.block.state.BlockState[]::new));
             this.getHandle().connection.send(packet);
         }
+
+        for (ClientboundBlockEntityDataPacket packet: entityDataPackets) {
+            this.getHandle().connection.send(packet);
+        }
     }
 
     @Override

Would something like this make sense? It misses the // paper start etc and I didn't submit it as a PR so we can clarify if e.g. we want to patch that upstream method or add another overload to the multiBlockChange, if it should be a dedicated patch or piggy backed into the other patch.