Sonic2423 / NeoForwarding

Enables modern player information forwarding in NeoForge servers for use with Velocity
GNU General Public License v3.0
4 stars 2 forks source link

Avoid side effects on stream codecs related to custom queries #5

Closed ustc-zzzz closed 1 month ago

ustc-zzzz commented 1 month ago

The encoder of a StreamCodec should not produce any side effect on the object to encode, which allows encoding multiple times. For example:

MyPacket packet = ...

FriendlyByteBuf buf = new FriendlyByteBuf(Unpooled.buffer());
MyPacket.STREAM_CODEC.encode(buf, packet);
MyPacket.STREAM_CODEC.encode(buf, packet);
MyPacket.STREAM_CODEC.encode(buf, packet);
... // encode packet multiple times

The internal data of a packet should not change after multiple encodings while ClientboundCustomQueryPackets and ServerboundCustomQueryAnswerPackets in the current version of NeoForwarding do not fit the situation: the related write methods can only be called once because the internal reader indexes of FriendlyByteBufs as members of PlayerDataForwarding.VelocityPlayerInfoPayloads and PlayerDataForwarding.QueryAnswerPayloads will change.

This pull request fixes this issue by replacing write methods with overloads by which side effects will not be produced.

Sonic2423 commented 1 month ago

I noticed the payload size doubling after testing. buf.writeBytes writes to the calling buffer object and its signature requires a source buffer, a start index for the source buffer and a length of how much to read from the source. With the pull request write would try to read as many bytes as the target buf already contains and not respecting the size of the source buffer.

I guess the intended fix would have been buf.readBytes(this.buffer, this.buffer.readerIndex(), this.buffer.readableBytes());?

Yes, buf.readBytes(this.buffer) increases the reader index of this.buffer so with commit 856c6cadce5d0905b391c4a627bea9d7be57ce6a i have fixed possible side effects a little differently to make the payloads intentions more clear.

Anyway, thanks for bringing this to my attention.