-
Notifications
You must be signed in to change notification settings - Fork 478
Implement an opt-in packet splitter. #4841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 1.21.8
Are you sure you want to change the base?
Conversation
...orking-api-v1/src/main/java/net/fabricmc/fabric/impl/networking/PayloadTypeRegistryImpl.java
Outdated
Show resolved
Hide resolved
...-api-v1/src/main/java/net/fabricmc/fabric/impl/networking/splitter/FabricPacketSplitter.java
Outdated
Show resolved
Hide resolved
|
||
import net.fabricmc.fabric.impl.networking.FabricPacketsImpl; | ||
|
||
public record FabricSplitDataPacketPayload(int splitId, int part, ByteBuf byteBuf) implements FabricSplitPacketPayload { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of redundant data here. A simple split start packet with an integer for the number of payloads to follow is enough. No need for splitId
and part
anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly to make it easier to find what went wrong if packets get reordered for whatever reason. Which while might not happen in vanilla, could be triggered by other mods / proxies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proxies should not be reordering, dropping arbitrarily, or otherwise altering packets that they don't recognize. Registry sync has sent order-sensitive packet split for a long time and as far as I know there has never been an issue.
...orking-api-v1/src/main/java/net/fabricmc/fabric/impl/networking/PayloadTypeRegistryImpl.java
Outdated
Show resolved
Hide resolved
When porting to 1.21.9, the registry sync module should be changed to use this too. (We can't change it for 1.21.8 because of the protocol change, but on the 1.21.9 update vanilla is changing the protocol anyway so it's fine). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has a lot of scope for unit tests, see DirectRegistryPacketHandlerTest
for where we test the registry sync packet splitting.
Implements a opt-in packet splitting logic, that can be used for any packet (through default implementation only handles CustomPayloadS2C and only works for S2C packets).
It's not enabled by for all packets, to prevent unwanted format breakage.
Functionally it's implemented in similar way to how vanilla handles Bundle packets, through work for both play and configuration stages. Outside the packet pipeline, the game / mods will only deal with their own packets. Aka entire splitting and merging is handled within packet pipeline.
Quick test (forcing all payload packets to go through it, with chunk size of 20 bytes) seemed to work just fine, but more extensive testing would be a good idea probably.
Currently splittable packets always go through packet splitting process, through ideally I want to get rid of that.
(Also keeping it as draft until I gather some feedback and more testing)