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 71 forks source link

Non NMS Packet classes being blacklisted. #583

Closed pronze closed 2 years ago

pronze commented 2 years ago

As the title states, Non NMS packet classes should not be blacklisted or blocked from the pipeline. This in fact, has nothing to do with OCM's functionality either. There are certain plugins that send "raw" packets, in the form of Netty's ByteBufs, entirely skipping the NMS encoder and are not passed around in the pipeline's channel handlers as NMS packets.

Our Library, ScreamingLib has a packet module which does the same. Normally, I'd have created a PR to include our packet module as a whitelist, but it's better to just entirely eliminate this check.

I-Al-Istannen commented 2 years ago

OCM needs to rewrite certain packets to be able to disable collisions and cancel a few packets. If plugins send packets as byte buffers, circumventing the normal minecraft packet handling, that might no longer be possible.

I do not really want users to report bugs against OCM in that case, as that is not our problem. Maybe we should print a bold warning if we detect a plugin is doing that and explicitly tell the user that some parts of OCMs functionality might not work as intended.

pronze commented 2 years ago

I can understand your concern, but it still makes zero sense to do that. Most plugins that send packets as raw byte buffers, are likely to be Clientbound non combat essential packets that entirely skip vanilla logic and should be the concern of the developers.

We also aren't transforming every packet in the pipeline, only the ones the we chose to construct and send manually. This measure has been taken due to 1.17.1 protocol classes being records, fields are now final. Our best option is to continue with ByteBuf's rather than Bytecode generation or manipulation to maintain a certain level of cross platform compatibility as well.

I'm also 100% with the idea of giving the users a fair warning.

I-Al-Istannen commented 2 years ago

Yea, the packet change was a bit of a headache to implement, but I did not expect plugins to just rewrite the protocol layer entirely. The check was added way before 1.17 when the only instances of non-packets being sent I saw were programmer errors.

One of the packets OCM needs to monitor and rewrite is tablist/team manipulation, which historically has been a field ripe with custom packet implementations. I am not convinced that skipping OCMs processing won't lead to bugs there.

If there are many bespoke protocol reimplementations out there or coming (which sounds like a headache) we don't really have a choice.

kernitus commented 2 years ago

Should be fixed with 7f39274