Better Combat [Fabric & Forge]

Better Combat [Fabric & Forge]

25M Downloads

Issues in single player and open to LAN due to resused bytebufs

DJtheRedstoner opened this issue ยท 5 comments

commented

Minecraft version - 1.20.2
Mod loader - Fabric
Mod version - 1.7.3
I am using the latest version available - Yes

Describe the bug
Various packet errors occur when using the mod in singleplayer or open to LAN. This is because the mod caches certain ByteBufs. Ordinarily this would be fine, but when in singleplayer packets are never serialized and the instances are directly passed from (integrated) server to client. This means the same ByteBuf instance is used, causing its readerIndex to be modified by the code that reads the packet. Then, next time this packet is sent/read an error occurs since the data has already been consumed. A fix is to slice the bytebuf before sending it using PacketByteBufs#slice. Adding this on these 2 lines resolved the issue for me.

sender.sendPacket(Packets.WeaponRegistrySync.ID, WeaponRegistry.getEncodedRegistry());
sender.sendPacket(Packets.ConfigSync.ID, configSerialized);

To Reproduce
Steps to reproduce the behavior:

  1. Create a singleplayer world with the mod
  2. Leave the world
  3. Attempt to rejoin

or

  1. Create a singlepalyer world with the mod
  2. Open to LAN
  3. Attempt to join from a second client

Expected behavior
The mod works in singleplayer and open to LAN.

Screenshots or video recordings
image

Additional context
This also breaks mods which depend on open to LAN for world sharing features, like Essential.

commented

I was curious about that too, but didn't investigate yet. Only took a few minutes to figure out what happened though:

Before 1.20.2, custom payloads where handled by CustomPayloadS2CPacket, who's constructor took an Identifier and a ByteBuf. In the write method it does buf.writeBytes(this.data.copy());. Since it makes a copy, it doesn't modify the readerIndex of the original ByteBuf. However, in 1.20.2 CustomPayloadS2CPacket was refactored to use a new CustomPayload interface. Fabric's implementation of this inteface does not copy or slice the ByteBuf when writing it. This isn't correct, I overlooked some things.

The real issue is this commit, which changes fabric to not slice ByteBufs that have been received.

I wonder if this could be considered a bug on fabric's end, I will ask. Following some discussion, this isn't behavior that should be relied on:
image

commented

Hello,
Thank you for the elaborate explanation, it helps a lot.

Although I am confused why this issue started appearing now. These serialized bytebuffers have been redistributed since the project was initially written.
Is there a change in MC 1.20.2 or a related Fabric API that would require me to change things?

commented

Thank you for the clarification.
This will be fixed as soon as I can work on BC, alongside with other refactors.
In the mean time I strongly recommend playing the 1.20.1 version instead, the selection of mods is far more generous, while losing very little functionality.

commented

Fixed by 1.8.0

commented

Note, Combat Roll mod also needs to be updated