Rework adventure handling

[booky10]

Reworks how packetevents handles adventure, should fix #1500

With this PR, packetevents now determines the currently installed version of adventure (if any) and downloads required dependencies with correct versions at runtime
This is not an optimal solution, though its the best solution in my opinion
I'd love to hear some feedback/other opinions on this

The options we currently have:

1. just stick with the current approach (shading without relocation) and work around all the weird issues this causes (not a good option)
2. relocate adventure (breaking change and is not an option for a library; there would be no way to "convert" between a packetevents Component class and the default adventure Component class, other than expensive serialization/deserialization)
3. drop support for platforms which either don't include adventure or include an outdated version of adventure (would be nice, though packetevents targets all versions starting with 1.8)
4. this PR (downloading adventure versions at runtime and injecting them into the platform classloader, won't work if offline; though e.g. paperclip also doesn't work if offline)

(this PR is also not fully done yet, we currently download adventure artifacts from maven central (against their ToS) and don't even cache them locally)

[TheFaser]

That's a good way to solve problem, but I think relocate is the better solution because it's easier to support. I don't think deserialize/serialize causes any critical latency, but I haven't tested it.

For example, I give my users a choice for this, and when they enable deserialize/serialize mode, they don't feel any message delay, even though my plugin sends messages through PacketEvents and it happens almost every tick. I haven't tested both cases, so I can't say for sure that this is a great way to do it, but I don't think it will be a bottleneck in PacketEvents.

Anyway, thank you for your work. You're doing a lot

[booky10]

For example, I give my users a choice for this, and when they enable deserialize/serialize mode, they don't feel any message delay, even though my plugin sends messages through PacketEvents and it happens almost every tick. I haven't tested both cases, so I can't say for sure that this is a great way to do it, but I don't think it will be a bottleneck in PacketEvents.

I get where you are coming from, but this still has the issue of breaking changes
And you would need to explicitely convert between net.kyori Component and a packetevents kyori Component on every method call which seems pretty excessive to me
I dont think this would be a good solution, especially for developer experience/ease of use

[TheFaser]

I get where you are coming from, but this still has the issue of breaking changes And you would need to explicitely convert between net.kyori Component and a packetevents kyori Component on every method call which seems pretty excessive to me I dont think this would be a good solution, especially for developer experience/ease of use

Developers will have to choose between using Kyori from PacketEvents or from Paper. In my personal experience with Paper, I very rarely need to use Kyori because most messages are sent through PacketEvents, which is more convenient.

Developers also have the option to relocate Kyori and PacketEvents libraries together to use their own version of Kyori.

I understand this is a breaking change, so it's probably best to discuss this for version 3.0.0. For now, your solution is good

[rafi67000]

Im vouching for either 1 or 4

First option sucks for packevents but works.
Second option would be very annoying for developers.
Third option is a HUGE NO because PE's greatness comes from multiplatform and multiversion support.
Dropping versions with incompatible/no adventure would be a disaster for plugin maintainers that depend on packetevents.
4th option is the best option in my opinion.

[iiAhmedYT]

im sorry to question this but how would this fix #1500 ? it's a compile-time induced error not really dependent on the adventure version in the server

basically if packetevents is compiled with 4.26.1 you can't call any Serializer#builder on 5.0.1

if its compiled with 5.0.1 you can't call Serializer#builder in older versions

[SamB440]

Would this approach fix the sponge problem? We shade adventure-nbt since sponge doesn't include it, but if two plugins shade it then everything breaks; so would this be checking if adventure-nbt exists and then download and install it if not?

[booky10]

im sorry to question this but how would this fix #1500 ?

@iiAhmedYT
without this PR, we currently shade a "patched" version of adventure's gson serializer which targets 4.26.1 and thus causes #1500

packetevents/patch/adventure-text-serializer-gson/src/main/java/net/kyori/adventure/text/serializer/gson/GsonComponentSerializer.java

Line 126 in f7479fd

interface Builder extends AbstractBuilder<GsonComponentSerializer>, Buildable.Builder<GsonComponentSerializer>, JSONComponentSerializer.Builder {

with this PR, we would no longer shade the adventure gson serializer ourselves but download it at runtime matching the adventure version currently installed on the platform (or defaulting to 4.26.1 if there is no adventure present)

feel free to test if this PR actually does fix the issue in your case, though it should work according to my testing

Would this approach fix the sponge problem? We shade adventure-nbt since sponge doesn't include it, but if two plugins shade it then everything breaks; so would this be checking if adventure-nbt exists and then download and install it if not?

@SamB440 yes, although this PR currently only does this for spigot
integrating this into the sponge module should be pretty simple though

[ytnoos]

Is removing Adventure from packetevents a valid alternative? Like completely stopping using it at all so packetevents doesn't need it anymore?

Or maybe distributing two different packetevents:

One with adventure bundled for legacy servers
The other without adventure, which relies on the one already present

[booky10]

Is removing Adventure from packetevents a valid alternative? Like completely stopping using it at all so packetevents doesn't need it anymore?

Would simplify things a lot, although you will still get breaking changes and less interopability with other plugins which may already use adventure

One with adventure bundled for legacy servers
The other without adventure, which relies on the one already present

This is basically what this PR does, but as one jar (and without hard failures)
Note that some parts of adventure aren't present on modern platforms - e.g. paper/sponge don't have adventure-nbt installed at all

[ytnoos]

Is removing Adventure from packetevents a valid alternative? Like completely stopping using it at all so packetevents doesn't need it anymore?

Would simplify things a lot, although you will still get breaking changes and less interopability with other plugins which may already use adventure

One with adventure bundled for legacy servers
The other without adventure, which relies on the one already present

This is basically what this PR does, but as one jar (and without hard failures)
Note that some parts of adventure aren't present on modern platforms - e.g. paper/sponge don't have adventure-nbt installed at all

Then I think that implementing the local cache so the download is made just at first boot is 100% necessary. Also packetevents should check on boot if the hash of the downloaded libraries is the same as the one needed, in case you update packetevents which requires a different adventure version compared to the cached one. (maybe you have already done it, I haven't looked at diffs)

Regarding dropping it, we are skipping a few things because "it would break everything", but one day I hope packetevents will be going to switch major version without caring of breaking things

[booky10]

Then I think that implementing the local cache so the download is made just at first boot is 100% necessary. Also packetevents should check on boot if the hash of the downloaded libraries is the same as the one needed, in case you update packetevents which requires a different adventure version compared to the cached one. (maybe you have already done it, I haven't looked at diffs)

yes, this should be done 100% - this PR is not finished yet, just wanted to gather feedback on this approach and what others think about this

Regarding dropping it, we are skipping a few things because "it would break everything", but one day I hope packetevents will be going to switch major version without caring of breaking things

v3 will contain breaking changes, though we aren't working on anything related to it yet