Avoid relying on package name to parse CraftBukkit version. #324

portlek · 2024-05-02T09:11:32Z

the pr might be a bit confusing. here are the things done:

revision names from wrapper class names removed.
searches the version wrapper starting from the server versions' patch version to 0.
move the special version classes to their own modules.
implemented a simple version parser using Bukkit#getVersion().

tested on 1.8.6, 1.8.8, 1.10.2, 1.17.1.

mastercake10 · 2024-05-03T12:52:30Z

What's the point of still having the R-Version in the module name while removing them in the class names? This is inconsistent.

Also, do we really need all these new classes in the API module? Afaik you copied them partially from the PaperLib, why can't they be shaded in? Its not ideal to have unattributed code from others in the repo. As we discussed in #294, we don't even need a lib for this. Getting the minor, major and patch version is really just parsing the version string.

portlek · 2024-05-03T14:37:21Z

What's the point of still having the R-Version in the module name while removing them in the class names? This is inconsistent.

Although I agree with what you are saying, I think it should stay the same because if we change the module names, every single artifactId in the maven repository will change and that might not be what @WesJD wants. It's up to him tho.

Also, do we really need all these new classes in the API module? Afaik you copied them partially from the PaperLib, why can't they be shaded in? It's not ideal to have unattributed code from others in the repo. As we discussed in #294, we don't even need a lib for this. Getting the minor, major and patch version is really just parsing the version string.

The reason why I didn't shade the paperlib in is because it's containing too many unnecessary features in it. I just copied the necessary part which parses the Minecraft version using Bukkit#getVersion() method, which is basically what you are saying here:

Getting the minor, major and patch version is really just parsing the version string.

Thanks for the feedback.

mastercake10 · 2024-05-03T15:06:43Z

every single artifactId in the maven repository will change

Why is this a problem? The modules get shaded in the final jar anyway, I don't think people use the Wrapper classes explicitly in their plugins. Even then they can still rename the module.

I think if we're going to ditch the R-Version, we should do it right, which means renaming the packages too.

The reason why I didn't shade the paperlib in is because it's containing too many unnecessary features in it. I just copied the necessary part which parses the Minecraft version using Bukkit#getVersion() method, which is basically what you are saying here:

Do you think we can use their code without attribution? I am not an expert when it comes to licenses, but it doesn't feel right. I think this can be done without using PaperLib at all. We don't even use the methods Environment#isPaper etc.

portlek · 2024-05-03T15:13:10Z

Why is this a problem? The modules get shaded in the final jar anyway, I don't think people use the Wrapper classes explicitly in their plugins. Even then they can still rename the module.

I think if we're going to ditch the R-Version, we should do it right, which means renaming the packages too.

idk I didn't say it's a problem, I just don't know what WesJD thinks about that. I'm willing to change the module names but it's not up to me.

Do you think we can use their code without attribution? I am not an expert when it comes to licenses, but it doesn't feel right. I think this can be done without using PaperLib at all.

It's an MIT license, which basically means that you do whatever you want with it. And plus, I've put the license file from the paperlib repository as well.

We don't even use the methods Environment#isPaper etc.

Yea, I agree, I can remove the isPaper/isSpigot parts and other unused methods as well.

portlek · 2024-05-03T15:22:06Z

remove almost every class and end up with a single version parser which makes things a lot simpler.

api/src/main/java/net/wesjd/anvilgui/version/VersionProvider.java

mastercake10 · 2024-05-03T15:26:20Z

Looking good.

It's an MIT license, which basically means that you do whatever you want with it. And plus, I've put the license file from the paperlib repository as well.

Yeah, I know. But it still needs attribution, which is basically what I was saying. The LICENSE file is empty btw. Mabye put it in the header of the file(s) so others know which parts are exactly taken from paper / licensed under MIT? I was thinking we could just avoid using PaperLib since its really not that hard to parse a simple string.

portlek · 2024-05-03T15:27:56Z

Yeah, I know. But it still needs attribution, which is basically what I was saying. The LICENSE file is empty btw. Mabye put it in the header of the file(s) so others know which parts are exactly taken from paper / licensed under MIT? I was thinking we could just avoid using PaperLib since its really not that hard to parse a simple string.

didn't notice it was empty. i've removed the license and everything related to paperlib so we should be good now.

mastercake10 · 2024-05-03T15:38:21Z

didn't notice it was empty. i've removed the license and everything related to paperlib so we should be good now.

Parts of the code are still from PaperLib, so it needs the attribution + license.

portlek · 2024-05-03T15:51:42Z

we don't need to add them, but I still add it to make you feel ok. it's not a required thing to do if it's a mit license. you just don't need to do that.

0dinD · 2024-05-04T02:19:51Z

In regards to the renaming of the packages and Maven artifacts: I think it's fine, since most (if not all) users of AnvilGUI probably just depend on the main AnvilGUI artifact, not the individual adapter modules. So the rename will not affect them.

If we make the decision to keep the old "R-version" module/package names, I think we should also keep the old class names, for consistency. Then we would need to have a hybrid approach where we use the new naming scheme for 1.20.5+ and fall back to the old "R-version" naming scheme for older modules. But for what it's worth, if I had to choose, I would ditch the "R-versions" entirely and rename the packages and Maven artifacts.

In regards to the version parsing, I am quite convinced that we could just use Bukkit.getServer().getBukkitVersion().split("-")[0] instead of trying to parse Bukkit.getVersion(), as discussed in #294 (comment). The reason why PaperLib does that is because it gives a few more bits of information, such as information about whether the Minecraft version is a pre-release or release candidate. But I don't see how this information is important to us. And if we use Bukkit.getServer().getBukkitVersion().split("-")[0], we can avoid copying from PaperLib at all and simplify the VersionProvider class.

But maybe there is some advantage to parsing Bukkit.getVersion() that I have missed?

0dinD · 2024-05-04T02:42:44Z

api/src/main/java/net/wesjd/anvilgui/version/VersionMatcher.java

+ // Start searching for suitable VersionWrapper
+ // starting from the current patch version going downward
+ VersionWrapper suitableWrapper = null;
+ for (int i = patch; i >= 0; --i) {


This would stop searching at the lowest patch version. Which works fine so far, because each new minor version thus far has required a new AnvilGUI module, due to Spigot's "R-version" packages.

But if Spigot stops using "R-version" packages in the future (which would not surprise me), we could end up in a situation where we have a Wrapper for, let's say, Minecraft 1.25, which also works for Minecraft 1.26. Then we would not be able to find the 1.25 Wrapper class, because the search stops at 1.26.

Honestly this is purely hypothetical at this point and doesn't really matter (we can fix it later if the problem arises), but I just wanted to highlight it for discussion.

The reason I bring this up is that I am wondering if it would be simpler (and also more future-proof) to just define an ordered list of Wrapper names (1_8, 1_8_3, 1_8_8, ..., 1_20_5) and then have the VersionMatcher try to load the highest supported version from the list. Just start from the back of the list and continue until a supported version is found (one that is less than or equal to the current version), then load that module. This is just an idea that came to my mind, let me know what you think.

not quite sure what you are talking about. the lowest patch version would be zero, and if it's zero, I don't use the patch version, and instead I just use major_minor pattern. I really don't understand what can cause an issue here from what you are saying. because we have 1.10 and 1.11 versions as well. so what's the difference between 1.10–1.11 and 1.25–1.26? since we're going to separate 1.25 and 1.26 versions as WrapperV1_25 and WrapperV1_26 like how we did it in 1.10–1.11 versions.
the main reason why I don't like defining every single version in a map is because it'll require changing the map values on every single release. so I don't like putting new things that can go wrong while reviewing. making things as automatic as possible would be much more cleaner imo.

not quite sure what you are talking about. the lowest patch version would be zero, and if it's zero, I don't use the patch version, and instead I just use major_minor pattern.

Yes, but that's not the problem I referring to. Let me try to illustrate the problem:

Let's say that we have the following list of wrapper versions in the future:

1_8

1_8_3

1_8_8

...

1_25

1_25_1

1_25_2

1_28

1_28_3

And let's say that the current Minecraft version is 1.26.3. Which Wrapper should be loaded? Well, from looking at the list, we can see that 1_28_3 and 1_28 are out of the question since they require a higher Minecraft version than the current one. But 1_25_2 is the next Wrapper in the list, and this one is lower than the current version, so this is the correct one (we can interpret from the list that the 1_25_2 Wrapper supports all Minecraft versions starting from 1.25.2 up to 1.28.

The problem with your current solution is that (unless I'm mistaken) it would start searching for 1_26_3, then for 1_26_2, 1_26_1 and finally for 1_26. After that, it would give up the search and throw new IllegalStateException("No compatible version wrapper found for server version v" + major + "." + minor + "." + patch);. But that's not correct, because there is a compatible version wrapper (1_25_2).

because we have 1.10 and 1.11 versions as well. so what's the difference between 1.10–1.11 and 1.25–1.26? since we're going to separate 1.25 and 1.26 versions as WrapperV1_25 and WrapperV1_26 like how we did it in 1.10–1.11 versions.

That is not the same situation. There is a WrapperV1_10 and a WrapperV1_11, so your current solution will work for these versions because the search never has to continue down to a lower minor version. As illustrated above, the hypothetical case I described above is different in that there is a WrapperV1_25_2, which is also compatible with 1.26.x versions, but there are no WrapperV1_26_X version wrappers, so your current algorithm will not find a wrapper if the current version is 1.26.x even though there is a compatible one (WrapperV1_25_2).

Again, I realize that this case is currently hypothetical, but in my opinion, there is no reason why it couldn't happen in the future if Spigot stops using "R-versions", which would improve version compatibility across minor version releases where nothing about the Anvil classes changes.

the main reason why I don't like defining every single version in a map is because it'll require changing the map values on every single release. so I don't like putting new things that can go wrong while reviewing. making things as automatic as possible would be much more cleaner imo.

I do agree and I see the benefit of your solution, IF the current assumption that each minor release requires a new wrapper class holds. My point is that I don't think that assumption will hold forever, and the solution with the version wrapper list is simpler and easier to maintain. Yes, we would need to add a number to the list each time we add a new wrapper, but it's the same for the Maven modules. In fact, we could generate the list based on the list of Maven modules in the pom.xml file if we really wanted to (not sure how easy this is to do in Maven, but it can easily be done in Gradle so I assume there is some way). But manually adding a new version to the list is not really that much of a pain, I don't think, so not sure if it's worth automating if that turns out to be complicated in Maven.

Again, your solution will definitely work as long as the assumption holds, and I don't oppose it. We can always change VersionMatcher in a future release if need be. I just wanted to bring this up for discussion, since I think having the list would lead to a simpler and more future-proof VersionMatcher (unless I am mistaken).

And let's say that the current Minecraft version is 1.26.3. Which Wrapper should be loaded? Well, from looking at the list, we can see that 1_28_3 and 1_28 are out of the question since they require a higher Minecraft version than the current one. But 1_25_2 is the next Wrapper in the list, and this one is lower than the current version, so this is the correct one (we can interpret from the list that the 1_25_2 Wrapper supports all Minecraft versions starting from 1.25.2 up to 1.28.

The problem with your current solution is that (unless I'm mistaken) it would start searching for 1_26_3, then for 1_26_2, 1_26_1 and finally for 1_26. After that, it would give up the search and throw new IllegalStateException("No compatible version wrapper found for server version v" + major + "." + minor + "." + patch);. But that's not correct, because there is a compatible version wrapper (1_25_2).

that's not a problem tho, we have to provide wrapper versions for each minecraft version. if there is no 1_26 wrapper which means we don't support 1.26 version yet. this is already how the library works right now. why we jump from 1_25 to 1_28? that doesn't make sense and never gonna happen either way.

this is already how the library works right now. why we jump from 1_25 to 1_28? that doesn't make sense and never gonna happen either way.

That can happen in the future, if Spigot stops using the "R-versions" and just uses Mojang mappings (same as Paper), which wouldn't surprise me. When that happens, it is totally possible for a 1.25.2 wrapper to be compatible with Minecraft 1.26, 1.27 etc. This can already be done for Paper, as mentioned by @mastercake10 in #294:

I see one huge advantage in method 2), which is that the new wrapper class can be re-used for all upcoming paper versions, since it doesn't rely on imports with cb version numbers in it. Also mojang mappings doesn't change over releases.

Once again, I do agree that this situation currently cannot happen, so I am also fine with keeping your solution until this hypothetical (but fairly likely, in my opinion) situation happens.

Right, that's why we don't even need to drop the R at this point.

i think we can do that but this is not the PR for it. maybe after refactoring the build system to gradle.

so the problem is not the mappings or R or anything, just DIY?

I don't think there is a problem for now, its just based on assumptions for the future.

It comes down to personal preference, for me this whole process of finding the right Wrapper class for a given MC version is not really required, especially for < MC 1.20.5, where you can still look up the R-Version.

i think we can do that but this is not the PR for it. maybe after refactoring the build system to gradle.

I didn't mean the module names, you already dropped the R in the class names. I have already a fork for gradle support as discussed here, but it doesn't drop the R (see the comment why).

i think we should think about all of these whenever we need, let's just focus on merging this pr. sent request.

There is so much discussion going on here, we simply don't know what Spigot's next moves are, so why not just take the simple approach and see what the future brings?

i think we should think about all of these whenever we need,

Once again, I brought this stuff with VersionMatcher up only for discussion, I agree that this is not something we have to fix right now.

This is why we discussed just checking if the package name contains an R-Version, and if it does not, load the module through a lookup table in.

It comes down to personal preference, for me this whole process of finding the right Wrapper class for a given MC version is not really required, especially for < MC 1.20.5, where you can still look up the R-Version.

This is the second point that I was trying to make, which is that the current VersionMatcher implementation (apart from possibly breaking in the future if we don't have a wrapper for each minor version because Spigot moves in the direction of Paper and uses Mojang mappings without "R-versions") seems unnecessarily complex when we can just simply have a lookup table or list.

Once again, I don't oppose the current VersionMatcher implementation, it will definitely work. Ultimately the decision is not up to me either way, I am not a maintainer here, just throwing my ideas around.

Good point @0dinD, but I think we should wait to fix this issue until the problem arises.

portlek · 2024-05-04T11:31:54Z

In regards to the version parsing, I am quite convinced that we could just use Bukkit.getServer().getBukkitVersion().split("-")[0] instead of trying to parse Bukkit.getVersion(), as discussed in #294 (comment). The reason why PaperLib does that is because it gives a few more bits of information, such as information about whether the Minecraft version is a pre-release or release candidate. But I don't see how this information is important to us. And if we use Bukkit.getServer().getBukkitVersion().split("-")[0], we can avoid copying from PaperLib at all and simplify the VersionProvider class.

But maybe there is some advantage to parsing Bukkit.getVersion() that I have missed?

Bukkit#getVersion() is much more simple to work with in terms of the knowledge about the way Bukkit works. The main reason why I think that is because Bukkit#getBukkitVersion() defined by Bukkit, but Bukkit#getVersion() method is using the thing that represents the Mojang's implementation. So if MinecraftServer#getServerVersion() implementation changes, which is highly unlikely, then we will know that immediately and change the version pattern in the version provider. But let's say if some random fork decided to change CraftServer#getBukkitVersion(), we cannot catch that change easily since we cannot know every single spigot/paper fork on the planet.

0dinD · 2024-05-04T12:06:33Z

Bukkit#getVersion() is much more simple to work with in terms of the knowledge about the way Bukkit works. The main reason why I think that is because Bukkit#getBukkitVersion() defined by Bukkit, but Bukkit#getVersion() method is using the thing that represents the Mojang's implementation. So if MinecraftServer#getServerVersion() implementation changes, which is highly unlikely, then we will know that immediately and change the version pattern in the version provider. But let's say if some random fork decided to change CraftServer#getBukkitVersion(), we cannot catch that change easily since we cannot know every single spigot/paper fork on the planet.

Okay, that is a fair point. Although I honestly find it hard to believe that a fork would change the getBukkitVersion implementation that drastically. At that point, what's to say that they don't also change getVersion to return something in a different format as well? Like, neither of those methods actually make any guarantees about the return string format, as far as I know. But then again, I would agree that getBukkitVersion is more likely to change than getVersion, so honestly I'm fine with your solution.

0dinD · 2024-05-04T17:10:51Z

Personally I would have liked to see either sticking entirely with the old "R-version" names for both the modules and the class names (which would lead to a very simple PR with minimal changes) or the complete opposite where we remove all traces of "R-versions", even from the module names. Currently, we have an inconsistent middle ground.

But for what it's worth, I still approve of the current solution, because it will work, and at the end of the day that's what really matters.

WesJD

Overall, good work here. I really appreciate your help.

Since you're renaming the classes to remove the concept of an "R-version", then the module names should have the "R-version" removed from them too.

I think that this PR could have been much simpler (i.e. without touching all of the past versions), but it's not going to hurt anything to make this change. It just means more mental overhead to read the diff.

WesJD · 2024-05-04T19:41:50Z

api/src/main/java/net/wesjd/anvilgui/version/VersionMatcher.java

+ // Start searching for suitable VersionWrapper
+ // starting from the current patch version going downward
+ VersionWrapper suitableWrapper = null;
+ for (int i = patch; i >= 0; --i) {


Good point @0dinD, but I think we should wait to fix this issue until the problem arises.

api/src/main/java/net/wesjd/anvilgui/version/VersionProvider.java

mastercake10

Looking good now, but yeah dropping the R-Version on mc < 1.20.5 is not essentially needed, but shouldn't hurt either.

1_10_2/pom.xml

1_16_1/pom.xml

api/src/main/java/net/wesjd/anvilgui/version/VersionProvider.java

1_10/pom.xml

1_16/pom.xml

1_14/pom.xml

1_17/pom.xml

1_19/pom.xml

0dinD · 2024-05-05T11:33:37Z

Looks like these dependencies have not been updated to use the new module names:

https://github.com/forkstuffs/AnvilGUI/blob/90fcd18557b98ac2a1a45efca509f2a4296a1bdd/api/pom.xml#L28-L195

0dinD

I have not had time to test this (run it on a server), but the code looks good to me now. Good work!

WesJD · 2024-05-05T22:29:23Z

@portlek Can you confirm which versions you tested the latest commit on?

portlek · 2024-05-05T23:38:49Z

@portlek Can you confirm which versions you tested the latest commit on?

i didn't test the latest commits. whenever i get time, i will test on different versions.

WesJD · 2024-05-11T23:01:33Z

@mastercake10 @0dinD @portlek could someone test the latest commit so we can get this out?

mastercake10 · 2024-05-12T00:02:07Z

@mastercake10 @0dinD @portlek could someone test the latest commit so we can get this out?

I think this should be fine, I've tested it on a few MC versions. Shouldn't break anything.

But I have my gradle fork that already has support for 1.20.5/6. If we decide to use Gradle instead of maven later on, all the changes of this PR would be useless since I haven't renamed the NMS modules.

The reason why I haven't created this PR is that I am not sure if the PaperWrapper will stay the same for the next few minecraft versions, its just speculations at this point.

This is why we proposed not to rename the modules in #294 and take a simple approach instead (something like just to check if the craft bukkit version contains an R, if not, get the minecraft version and find the appropriate "R-Module" through a look up table).

I don't want to void all the work @portlek put into this PR, I really appreciate it. Maybe we should find a middle ground and create a new PR without changing that much to make it work for 1.20.5/6? We can still merge this or my gradle fork when necessary later on.

WesJD · 2024-05-12T00:07:57Z

I'm good for you to take the lead and make a decision there. If you think it's better to do a simpler PR, just whip one up. I just want to make sure we are on track to support the new versions, as I've been getting a lot of requests for it.

BySwiizen · 2024-05-12T11:43:15Z

I'm good for you to take the lead and make a decision there. If you think it's better to do a simpler PR, just whip one up. I just want to make sure we are on track to support the new versions, as I've been getting a lot of requests for it.

I think it would be more favorable firstly to make a simple commit which adds the new versions and secondly, to create a branch with all the more technical modifications to be made.

WesJD · 2024-05-13T01:44:03Z

I'm closing this since we just merged #326 . Let's make these technical changes in another PR if still desired.

portlek added 9 commits May 2, 2024 11:44

start working on new versions.

b273699

spotless.

bf4111e

find suitable VersionWrapper.

eaf0ee4

Update VersionMatcher.java

14823e3

Update VersionMatcher.java

efc7cc2

Update VersionMatcher.java

6770e87

Update VersionMatcher.java

a57f05e

Update VersionMatcher.java

e431a02

Update VersionMatcher.java

4e0fc8a

remove unnecessary classes and methods.

0ed9c0c

Update VersionProvider.java

6a45bc6

portlek commented May 3, 2024

View reviewed changes

api/src/main/java/net/wesjd/anvilgui/version/VersionProvider.java Outdated Show resolved Hide resolved

Add license and attribution.

8ffbf2f

0dinD reviewed May 4, 2024

View reviewed changes

portlek requested review from mastercake10 and 0dinD May 4, 2024 15:18

WesJD requested changes May 4, 2024

View reviewed changes

throw exception when parsing integers.

6cb890c

portlek requested a review from WesJD May 4, 2024 19:56

portlek added 2 commits May 4, 2024 23:23

move modules.

b231211

update artifact ids as well.

90b34f9

mastercake10 approved these changes May 4, 2024

View reviewed changes

0dinD suggested changes May 5, 2024

View reviewed changes

1_10_2/pom.xml Outdated Show resolved Hide resolved

1_16_1/pom.xml Outdated Show resolved Hide resolved

0dinD reviewed May 5, 2024

View reviewed changes

api/src/main/java/net/wesjd/anvilgui/version/VersionProvider.java Outdated Show resolved Hide resolved

portlek added 3 commits May 5, 2024 12:11

Update VersionProvider.java

2e76a4b

rename.

81475c1

rename.

90fcd18

portlek requested review from 0dinD and mastercake10 May 5, 2024 09:12

0dinD suggested changes May 5, 2024

View reviewed changes

1_10/pom.xml Outdated Show resolved Hide resolved

1_16/pom.xml Outdated Show resolved Hide resolved

1_14/pom.xml Outdated Show resolved Hide resolved

1_17/pom.xml Outdated Show resolved Hide resolved

1_19/pom.xml Outdated Show resolved Hide resolved

fix.

af216a7

portlek requested a review from 0dinD May 5, 2024 11:52

0dinD approved these changes May 5, 2024

View reviewed changes

WesJD closed this May 13, 2024

0dinD mentioned this pull request May 30, 2024

Fix VersionMatcher compatibility with other 1.20.x versions #331

Open

Avoid relying on package name to parse CraftBukkit version. #324

Avoid relying on package name to parse CraftBukkit version. #324

Conversation

portlek commented May 2, 2024 • edited

mastercake10 commented May 3, 2024

portlek commented May 3, 2024 • edited

mastercake10 commented May 3, 2024

portlek commented May 3, 2024

portlek commented May 3, 2024

mastercake10 commented May 3, 2024

portlek commented May 3, 2024

mastercake10 commented May 3, 2024

portlek commented May 3, 2024 • edited

0dinD commented May 4, 2024 • edited

0dinD May 4, 2024

Choose a reason for hiding this comment

portlek May 4, 2024

Choose a reason for hiding this comment

0dinD May 4, 2024 • edited

Choose a reason for hiding this comment

portlek May 4, 2024 • edited

Choose a reason for hiding this comment

0dinD May 4, 2024 • edited

Choose a reason for hiding this comment

portlek May 4, 2024

Choose a reason for hiding this comment

mastercake10 May 4, 2024

Choose a reason for hiding this comment

portlek May 4, 2024

Choose a reason for hiding this comment

0dinD May 4, 2024 • edited

Choose a reason for hiding this comment

WesJD May 4, 2024

Choose a reason for hiding this comment

portlek commented May 4, 2024 • edited

0dinD commented May 4, 2024

0dinD commented May 4, 2024

WesJD left a comment

Choose a reason for hiding this comment

WesJD May 4, 2024

Choose a reason for hiding this comment

mastercake10 left a comment

Choose a reason for hiding this comment

0dinD commented May 5, 2024

0dinD left a comment

Choose a reason for hiding this comment

WesJD commented May 5, 2024

portlek commented May 5, 2024 • edited

WesJD commented May 11, 2024

mastercake10 commented May 12, 2024

WesJD commented May 12, 2024

BySwiizen commented May 12, 2024

WesJD commented May 13, 2024

portlek commented May 2, 2024 •

edited

portlek commented May 3, 2024 •

edited

portlek commented May 3, 2024 •

edited

0dinD commented May 4, 2024 •

edited

0dinD May 4, 2024 •

edited

portlek May 4, 2024 •

edited

0dinD May 4, 2024 •

edited

0dinD May 4, 2024 •

edited

portlek commented May 4, 2024 •

edited

portlek commented May 5, 2024 •

edited