PaperMC / Paper

The most widely used, high performance Minecraft server that aims to fix gameplay and mechanics inconsistencies
https://papermc.io/
Other
10.04k stars 2.34k forks source link

CraftBukkit classes are incorrectly no longer under a versioned package #10618

Closed EricLangezaal closed 7 months ago

EricLangezaal commented 7 months ago

Expected behavior

In any version of Spigot, as well as every version of PaperMC up until 1.20.4, the CraftBukkit classes were located under the following package: org.bukkit.craftbukkit.v1_20_R4, where the last part changes, denoting the remapping version.

For a lot of plugins that use NMS this remapping is important. Sometimes when the version changes, the internals stay the same, meaning the same NMS code can be used. Therefore a plugin needs to be able to detect the remapping somewhere, using the version would unnecessarily break the plugin (since an older NMS implementation might still work if no remapping has happened). Therefore the de facto standard way of detecting this remapping is by doing:

String packageName = plugin.getServer().getClass().getPackage().getName();
String nmsVersion = packageName.substring(packageName.lastIndexOf('.') + 1);

Which has always worked on older version of Paper, and still works on Spigot. When testing with the new 1.20.5 versions of PaperMC though (specifically build 22), the craftbukkit classes are suddenly under org.bukkit.craftbukkit, therefore breaking this detection.

Observed/Actual behavior

Instead of returning the craftbukkit class under a versioned package like any other versions or Spigot, it returns an un-versioned package org.bukkit.craftbukkit

Steps/models to reproduce

Run the following code in any plugin:

String packageName = plugin.getServer().getClass().getPackage().getName();
Bukkit.getLogger(packageName);

On Spigot and any older version of PaperMC (i.e. 1.20.4) this correctly prints "org.bukkit.craftbukkit.v1_20_R4". On the new PaperMc builds, this however incorrectly prints "org.bukkit.craftbukkit".

Plugin and Datapack List

No datapacks. Not related to any plugins specifically.

Paper version

This server is running Paper version git-Paper-22 (MC: 1.20.5) (Implementing API version 1.20.5-R0.1-SNAPSHOT) (Git: 8f7ac62)

Other

No response

MachineBreaker commented 7 months ago

This is an intended change, read https://forums.papermc.io/threads/important-dev-psa-future-removal-of-cb-package-relocation.1106/ for more information

EricLangezaal commented 7 months ago

Ah shame Paper decided to again implement a breaking change, deviating it more from Spigot... Is there truly no way to detect the remapping version? Their argument is just not true, there has been quite a couple of times where an update didn't change remapping, allowing an NMS based plugin to work without requiring an update. (i.e. 1.16.4->5, 1.20 -> 1.20.1) By using the Minecraft version this would not work, requiring an unnecessary 'update' for every Minecraft version. I therefore really want to detect the remapping version, is there any way of doing so?

MachineBreaker commented 7 months ago

Ah shame Paper decided to again implement a breaking change, deviating it more from Spigot... Is there truly no way to detect the remapping version? Their argument is just not true, there has been quite a couple of times where an update didn't change remapping, allowing an NMS based plugin to work without requiring an update. (i.e. 1.16.4->5, 1.20 -> 1.20.1) By using the Minecraft version this would not work, requiring an unnecessary 'update' for every Minecraft version. I therefore really want to detect the remapping version, is there any way of doing so?

The post mentions the proper api to detect the minecraft version (i personally never tested it though) image

EricLangezaal commented 7 months ago

Yeah that's the issue though. The minecraft version and remapping version are different, which Paper just seems to ignore. Take 1.20 and 1.20.1 for example; obviously the minecraft version is different, but it only concerns as small bug fix. As such the internals were not remapped, and a plugin build against 1.20 NMS would work on 1.20.1 and vice-versa. If they based their detection on the Minecraft version however, the plugin would incorrectly have to say "unsupported version", even though it would work just fine. That's exactly why most people detect the remapping version instead, which Paper decided was a good idea to break...

yannicklamprecht commented 7 months ago

Yeah that's the issue though. The minecraft version and remapping version are different, which Paper just seems to ignore. Take 1.20 and 1.20.1 for example; obviously the minecraft version is different, but it only concerns as small bug fix. As such the internals were not remapped, and a plugin build against 1.20 NMS would work on 1.20.1 and vice-versa. If they based their detection on the Minecraft version however, the plugin would incorrectly have to say "unsupported version", even though it would work just fine. That's exactly why most people detect the remapping version instead, which Paper decided was a good idea to break...

Then just check both in your switch. That isn't even a hard operation.

        String version = ...;

        switch (version) {
            case "1.20", "1.20.1" -> new Nms120Adapter();
            case "1.20.3", "1.20.4" -> new Nms1204Adapter();
            case "1.20.5", "1.20.6" -> new Nms1206Adapter();
        }
EricLangezaal commented 7 months ago

Yeah that's the issue though. The minecraft version and remapping version are different, which Paper just seems to ignore. Take 1.20 and 1.20.1 for example; obviously the minecraft version is different, but it only concerns as small bug fix. As such the internals were not remapped, and a plugin build against 1.20 NMS would work on 1.20.1 and vice-versa. If they based their detection on the Minecraft version however, the plugin would incorrectly have to say "unsupported version", even though it would work just fine. That's exactly why most people detect the remapping version instead, which Paper decided was a good idea to break...

Then just check both in your switch. That isn't even a hard operation.

        String version = ...;

        switch (version) {
            case "1.20", "1.20.1" -> new Nms120Adapter();
            case "1.20.3", "1.20.4" -> new Nms1204Adapter();
            case "1.20.5", "1.20.6" -> new Nms1206Adapter();
        }

That doesn't really help though. My issue is that that would require a manual update for a plugin, where really none would be required. On Spigot (and older Paper), my plugin would automatically work if no remapping had occurred, without my users having to wait for an update, which is way nicer for users of course. If a remapping did occur, my plugin would detect that it didn't have an implementation, and disable accordingly, instead of throwing an uninformative (for users) error.

With the accepted 'solution', this is no longer possible. For every new Minecraft version, I would have to update my plugin, simply to add the String "1.20.1" to it, even though it would have actually worked just fine on some versions. This manual step just feels unnecessary and silly to me, since it wasn't required before.

yannicklamprecht commented 7 months ago

@EricLangezaal so you basically need a method to get the legacyRelocationVersion() which should be included somewhere in the paper jar but isn't exposed via API.

EricLangezaal commented 7 months ago

Yeah Ideally I need something which is consistent on Spigot and Paper (such that I don't need to create an exception for Paper's differences again), that allows me to find the remapping version. I posted on that thread though and it doesn't seem like they provide that anywhere/are willing to make Paper consistent with Spigot in that sense.

Sytm commented 7 months ago

I think the easiest solution here is to just default to the latest version adapter with something like this:

        switch (version) {
            case "1.20", "1.20.1" -> new Nms120Adapter();
            case "1.20.3", "1.20.4" -> new Nms1204Adapter();
            case "1.20.5", "1.20.6" -> new Nms1206Adapter();
            default -> new Nms1206Adapter();
        }

This basically results in the same behavior for users who want to use the latest version

EricLangezaal commented 7 months ago

I think the easiest solution here is to just default to the latest version adapter with something like this:

        switch (version) {
            case "1.20", "1.20.1" -> new Nms120Adapter();
            case "1.20.3", "1.20.4" -> new Nms1204Adapter();
            case "1.20.5", "1.20.6" -> new Nms1206Adapter();
            default -> new Nms1206Adapter();
        }

This basically results in the same behavior for users who want to use the latest version

Not really though, this will just cause a crash on most versions. In that case I'd rather show a more meaningful message by being able to detect it upfront. Only if the remapping version hasn't changed there might be a chance that an old implementation would still work.

Sytm commented 7 months ago

Not really though, this will just cause a crash on most versions.

Although it wouldn't?? Only on the latest if it isn't supported. Supported versions will be handled by the specific cases and a version that is too old could also be detected like checking that the version is at least 1.13 or greater.

And it doesn't get any more stable than this when using NMS, because maybe something changes in the internals without a mapping name change

EricLangezaal commented 7 months ago

Not really though, this will just cause a crash on most versions.

Although it wouldn't?? Only on the latest if it isn't supported. Supported versions will be handled by the specific cases and a version that is too old could also be detected like checking that the version is at least 1.13 or greater.

And it doesn't get any more stable than this when using NMS, because maybe something changes in the internals without a mapping name change

I understand that. Let me explain what I meant more clearly: In 80-90% of the MC updates, I need to update the plugin too. So defaulting to newest isn't a good idea, that will crash far more often then it will help. If the remapped version hasn't changed however, there is a good (in my experience 75%) chance that it will work. So I therefore wanted to detect the remapping number, since that is an indicator that little/no NMS has changed.

Closing this as Paper is fixed on using their package naming, despite it being less informative.

Machine-Maker commented 7 months ago

If the remapped version hasn't changed however, there is a good (in my experience 75%) chance that it will work.

I don't think this is a good metric to use if you are basing it off of probabilities. Internals can change at any time even within a minecraft version, that's why they are called internals, and not API. Paper can make changes to internal mechanisms within a version's lifespan. The best way to know that no changes have been made to logic that is unmodified by paper, is the actual minecraft version. That has a 100% chance of changing whenever anything mojang writes changes, and a 0% chance of changing when mojang doesn't change anything.

Leguan16 commented 7 months ago

I know this is already closed but just to let you know that with soft spoon approaching slowly, paper will more and more deviate from spigot. It's just a matter of time at this point :)