Implement packet filtering for bundled packets in 1.19.4 #2258
Conversation
There was a problem hiding this comment.
There are 3 things I noticed here:
- There is currently no way to efficiently cancel a specific bundle packet. I would personally call the bundle packet event first, and only call sub-bundle packets if it didn't get cancelled by any of the listeners. That way users listening to the bundle packet would receive a notification for bundle packets as well. ATM this is only possible by
- removing all packets from the bundle in the corresponding sub-packet listener
- canceling the bundle packet in one of the sub-bundle packets.
- I would really like to see you moving the PacketContainer collection read from the invokePacketSending method to a specialized method in StructureModifier (maybe there is one already, I did not check). This way users that want to send their own bundle actually have the tool to write the packets directly to the underlying object.
- Regarding the BundlePacket class mess: The packet should be registered on the server side somewhere, I guess just the mapping for the packet class is missing. The packet itself should be registered somewhere after the update (there should be a test which validates that, maybe that helps)
|
Thank you for your comments @derklaro. I totally agree with you and took care of the first two points. Regarding the 3rd one, the actual problem is that the While my current "solution" is definitely not ideal, I still do not see how to improve it without reworking the whole packet registration mechanism. Maybe that's something you could have a look at @derklaro @dmulloy2 as I am not really experienced with the ProtocolLib internals. Also, I suggest to rename PacketType.Play.Server.DELIMITER into PacketType.Play.Server.BUNDLE as that's the packet type ProtocolLib can interact with. The Delimiter packet is just inserted in a later stage of the Netty pipeline. |
|
Nice work, thank you! I was actually about to throw together something similar to this. Had been hoping we could move our injector to before the bundler but alas. As for the bundler class, I'd prefer to keep the PacketType enum 1:1 to EnumProtocol (makes updates scriptable) and just deal with the special case. Things of this nature are littered with special cases anyhow |
lukalt
changed the title
Since Minecraft 1.19.4, the protocol supports bundling consecutive packets to ensure the client processes them in one tick. However, Packet Events are not called for the individual packets in such a bundle in the current dev build of ProtocolLib. For example, no packet events are currently sent for the ENTITY_METADATA packet when an entity is first spawned as the packet is bundled with the ENTITY_SPAWN packet. However, if the entity metadata is changed later on, the event will be called.
This PR proposes to fix this by unpacking the bundled packets and invoking the packet filtering for each packet.
I also want to briefly explain how the bundling works. A bundle starts with a PACKET_DELIMITER (0x00,
net.minecraft.network.protocol.BundleDelimiterPacket) packet followed by all packets that should be bundled and finished with another PACKET_DELIMITER (0x00). Within the Netty pipeline, this sequence is transformed into one synthesized packet found innet.minecraft.network.protocol.game.ClientboundBundlePacket, which is essentially just a list of packets. At the stage at which ProtocolLib injects into the clientbound netty pipeline, this packet has not been unpacked yet. Thus, we need to handle theClientboundBundlePacket, which unfortunately is not registered in ProtocolLib. The fact that two different classes map to the same packet currently requires a dirty remapping in the packet structure modifier.Maybe someone can think of a better solution for this?
Fixes #2244