kernitus / BukkitOldCombatMechanics

Spigot plugin to configure combat mechanics from 1.9 onwards
https://www.spigotmc.org/resources/19510/
Mozilla Public License 2.0
168 stars 72 forks source link

Allow any non-NMS packet through the pipeline #607

Closed MWHunter closed 2 years ago

MWHunter commented 2 years ago

This fixes PacketEvents 2.0 compatibility.

It is perfectly legal to send a byte buffer through the netty pipeline. PacketEvents 2.0 is designed to use byte buffers over NMS objects for performance and compatiblity reasons. It's twice as fast to not use reflection to read packets. Additionally, there's few incompatibilities with server jars changing reflection. The code is also cleaner, and compatible with Bungeecord, Velocity, and Bukkit. While packetevents 2.0 could re-encode the packet. ViaVersion handles byte buffers fine, and vanilla's encoder will ignore packets that aren't NMS objects. Pushing it through the pipeline.

kernitus commented 2 years ago

@I-Al-Istannen did we not deliberately choose to filter packets that way?

I-Al-Istannen commented 2 years ago

This was discussed in https://github.com/kernitus/BukkitOldCombatMechanics/issues/583 and I am okay-ish with the change. When plugins do this, OCM can no longer reliably intercept packets and any OCM functionality depending on it might be silently broken.

I would like users to at least see a warning once, if OCM detects such a potential incompatibility. I don't want to end up debugging an issue that only exists become some plugin rewrote the protocol layer and bypassed OCM entirely.

kernitus commented 2 years ago

I see how that would make it very hard to debug if some other plugin is messing with packets. However with this change there wouldn't be any warning whatsoever? Maybe we should allow the packets through but log them with debug so at least there is some trace to work on if something goes wrong.

I-Al-Istannen commented 2 years ago

Yes, I'd either log a message on DEBUG or use some global state (a boolean perhaps) to log a message on a higher level once, if that is detected. I am not sure how much overhead debug logging has if it is disabled but it could be extremely spammy: Packets are potentially send often.

kernitus commented 2 years ago

Yes, I suppose the best approach is to noisily complain once, then let the packets through.

kernitus commented 2 years ago

Implemented with 7f39274