Fabric API

Fabric API

108M Downloads

21w37a Block Entities change and what it means for API.

LambdAurora opened this issue ยท 12 comments

commented

Hi, so basically this is to explore some of the changes in 21w37a to block entities and what it could lead to for the API.

Serialization changed quite a bit, the writeNbt method now returns void and is protected, other public methods should be used, one is basically a proxy, another one adds the id key with the BE type identifier, and another one adds in addition to the id key the x, y, and z fields.

But that's not the most important changes, the most important change is toUpdatePacket, the return type changed to Packet<?> and the BlockEntityUpdateS2CPacket changed a lot since now it takes a BlockEntityType parameter (got unhardcoded a lot).

All of these changes basically makes BlockEntityClientSerializable obselete.
I would recommend:

  • make BlockEntityClientSerializable deprecated, implementation-wise remove the "hack" to the packet.
  • in docs: tell mods to use the BlockEntityUpdateS2CPacket, it has a function to create an instance from the BE.
  • offer the sync method as a helper method outside of BlockEntityClientSerializable
commented

Seems like that is what has happened atm, 5553aff#diff-b4df20395c066a51c9febca3a0f3003f2c82933c54e7db7809e098eb76a908dd.

As for a sync helper method, the only code that needs to be called is:
((ServerWorld) world).getChunkManager().markForUpdate(pos);
not sure if it really needs a helper method but I'm not against one.

commented

Would be a use case for interface injection... ๐Ÿ˜„

commented

Shouldn't BlockEntityClientSerializable be removed entirely? We get rid of one module that way. ๐Ÿ˜„

commented

Haven't seen it that it was removed, nice.

I have to say that ((ServerWorld) world).getChunkManager().markForUpdate(pos); is definitely not intuitive.

commented

I feel like BECS can be deprecated/removed (I really would like to have stuff not deprecated for essentially forever) and sync can be somewhere else (networking api if we deprecate the whole module?).

commented

@bluejt There is no Fabric API release for 21w37a yet, but you can use Fabric loader 0.11.7 already. Please join the Discord or post on GitHub Discussions instead of posting unsolicited questions on issues.

commented

ๆ‰€ไปฅ21w37a็š„fabric่ฆๅœจๅ“ชไธ‹่ฝฝๅฎ‰่ฃ…ไบ†

commented

I removed this module as its no longer needed, and would have been a slight pain to update to keep it going anyway.

Adding back a helper somewhere would be useful, possbly worth waiting untill interface injection is a thing?

commented

For all who were as lost as I was when attempting to make a block entity sync to the client: I would like to share what I did that worked.

For a while, I was only able to make the data sync when the BlockState changed. (This was after implementing toUpdatePacket()) That changed when I added ((ServerWorld) world).getChunkManager().markForUpdate(pos); to markDirty(). This marks the block position as one that has a block entity to be synced with the client soon, usually within a tick.

However, this will NOT sync your block entity data when the chunk loads. For that, you must turn to toInitialChunkDataNbt() I mistakenly thought that this was data to be sent when the block is first initialized, as in when a new one is placed. (Don't ask me how I made that mistake. I haven't a clue.) This is data to be sent when the chunk is loaded. This happens either when the server is loading the chunk into memory from saved data, or when the client is requesting a chunk that is already loaded into memory.

As a bonus, (and as mentioned above), once you implement toInitialChunkDataNbt(), you can simplify toUpdatePacket() to the following:

    @Nullable
    @Override
    public Packet<ClientPlayPacketListener> toUpdatePacket() {
        return BlockEntityUpdateS2CPacket.create(this);
    }

Thus if you do updateListeners() or markForUpdate() when some changed data needs to be sent to the client, and implement both toUpdatePacket() and toInitialChunkDataNbt(), your block entity's data shouldn't desync from the client.

(Edited to remove reference to updateListeners(), as that is for when the VoxelShape changes and pathfinding entities need to be updated as to that fact. That's expensive, and in fact was the very reason BlockEntityClientSyncable existed in the first place.)

commented

So, should we inject a syncToClient method via interface injection or not?

commented

I don't think this issue is relevant anymore. The BlockEntityClientSerializable interface has been removed entirely, and block entities simply implement the following methods:

@Override
public BlockEntityUpdateS2CPacket toUpdatePacket() {
	return BlockEntityUpdateS2CPacket.create(this);
}

@Override
public NbtCompound toInitialChunkDataNbt() {
	NbtCompound nbt = new NbtCompound();
	this.writeClientNbt(nbt);
	return nbt;
}

private void writeClientNbt(NbtCompound nbt) {
	// Write NBT that should be written to the client here
}
commented

@haykam821 The main thing was whether to inject a sync() method. Personally I don't think it's even that useful.