Fix VersionMatcher compatibility with other 1.20.x versions

[spnda]

Fixes to the implementation from #326, as I originally suggested in #294.

This fixes compatibility with relocated server JARs before 1.20.5 and provides a safer and shorter implementation than what we have currently. Also works with 1.20.6.

Furthermore, we don't even need all of this extra checking technically. The Bukkit.getBukkitVersion() function has existed since 2011, and we could just rely on that 100% of the time if we want to create a massive map or we rename all of the version wrappers to their respective Minecraft version name. But I'll leave that open for now.

[0dinD]

What do you mean by "relocated server JARs before 1.20.5"? Relocated server JARs contain the "R-version" package, so those should work on Minecraft < 1.20.5 on both Spigot and Paper. Unless I'm mistaken.

Maybe you mean the experimental non-relocated Paper builds, such as this one? I suppose it wouldn't hurt to add compatibility for it, but it's an experimental build that is now obsolete, so I don't see why anyone would be using it.

[spnda]

Yeah, that experimental one. I was running that and the VersionMatcher couldn't match it.

The point I was trying to make, though, was that we should be avoiding trying to parse the individual version components when we could just use a Map for this, which is safer and more reliable. Also simpler to update for new versions.

[0dinD]

I see, yeah I guess it wouldn't hurt to add compatibility for it.

The point I was trying to make, though, was that we should be avoiding trying to parse the individual version components when we could just use a Map for this, which is safer and more reliable. Also simpler to update for new versions.

I do agree, using a Map to create a lookup table for the adapters seems like a better idea. I think the lookup table will be easier to maintain in the long run.

[0dinD]

The Bukkit.getBukkitVersion() function has existed since 2011, and we could just rely on that 100% of the time if we want to create a massive map or we rename all of the version wrappers to their respective Minecraft version name. But I'll leave that open for now.

It's not completely necessary, and requires quite a large amount of refactoring, but I do think it would be worth it (in a future PR) to rename the version wrappers to be based on the Minecraft version name (so we can have consistent wrapper names, and avoid the "special"/fake R-versions such as 1_14_4_R1, 1_17_1_R1 and 1_19_1_R1). In that case, we would need the "massive map" either way though since multiple Minecraft versions will need to share the same version wrapper. But I don't think that the "massive map" would be a problem. It would be easy to understand what it does since it's just a simple lookup table.

FYI this refactoring was already done in #324 but the PR was not merged because no one had time to test it and #326 achieved the same goal (1.20.6 support) but was easier to test. #324 did not use a lookup table though, instead it used an even more complex version parser. I was not super happy about that implementation since it was more complex than the lookup table, and came with some potential future problems. With that said, I think #324 could be used as a base for the version wrapper name refactoring, it should be pretty easy to change the version matcher to use a lookup table.

[mastercake10]

The reason why I decided to manually check the version is to make it (potentially) compatible with future minor versions without the need to update it. So this line if (major == 20 && minor >= 5) { ... } matches 1.20.5 and 1.20.6. Your implementation wouldn't match, lets say, 1.20.7. But I think this can easily be solved.

[0dinD]

That is a good point which I had not realized.

It would be possible to add future compatibility to the lookup table solution by using the version wrapper for the highest version in the lookup table as a fallback when the current version is not found in the table. Then again, just because the version is not found in the table, doesn't necessarily mean it's a future version. It could also be an old version of Bukkit, like 1.6.4 or something (not a very likely situation though). The version parsing could be added as a sanity check, I suppose, but honestly I don't know if it's really that necessary. Unconditionally loading the wrapper with the highest version as a fallback for all unknown versions seems fine to me. I don't see how we could ever guarantee that it's compatible (regardless of whether we use the version parsing approach), but some error handling could always be added.

[spnda]

The reason why I decided to manually check the version is to make it (potentially) compatible with future minor versions without the need to update it. So this line if (major == 20 && minor >= 5) { ... } matches 1.20.5 and 1.20.6. Your implementation wouldn't match, lets say, 1.20.7. But I think this can easily be solved.

I don't feel like this is a good thing at all? Imagine 1.20.7 releases but has breaking changes. The VersionMatcher wouldn't catch that since that check doesn't differentiate and therefore would assume that the version is compatible, but it actually isn't. For version detection it's best to stay conservative and not freely allow versions where you have no idea if they are compatible. Nearly every major library or application works this way.

[0dinD]

Yeah I mean the whole idea is that it would be a "best effort" kind of solution, without any guarantee of compatibility. I think there are both pros and cons with this approach. The benefit is that downstream plugins can push out updates faster in some cases because they don't need to wait for an AnvilGUI update, but yes, in some cases it could also lead to crashes or bugs if an incompatible adapter is loaded (until an AnvilGUI update is released).

Either way, I'm fine with dropping the future compatibility thing as a default. Maybe it could be added as an option to AnvilGUI? That way, the downstream plugin authors could each decide whether they want AnvilGUI to try loading the latest wrapper on unknown versions. This could be added in a future PR if someone is interested in that functionality.

[mastercake10]

The reason why I decided to manually check the version is to make it (potentially) compatible with future minor versions without the need to update it. So this line if (major == 20 && minor >= 5) { ... } matches 1.20.5 and 1.20.6. Your implementation wouldn't match, lets say, 1.20.7. But I think this can easily be solved.

I don't feel like this is a good thing at all? Imagine 1.20.7 releases but has breaking changes. The VersionMatcher wouldn't catch that since that check doesn't differentiate and therefore would assume that the version is compatible, but it actually isn't. For version detection it's best to stay conservative and not freely allow versions where you have no idea if they are compatible. Nearly every major library or application works this way.

Odds are high that we don't need to write a new wrapper class between minor versions, so why not at least try to load the wrapper class? Either way the plugin wouldn't start if there were breaking changes, so I don't see the downside here. Maybe for transparency we should print out to the console that the mc version isn't compatible when it tries to load a wrapper for a version that hasn't been added to the map yet.

I feel like ~50% of commits in the future will be "update version map". Plugin authors would need to update AnvilGUI every time a new mc version comes out. This wasn't the case before since the "R-Version" changed like every two mc releases?

[WesJD]

I like the fallback idea, and letting the user know in the logs that the fallback was used. Also, I think we should give developers the ability to disable the fallback if they don't want it.

[spnda]

I've added support for 1.21 on this branch and added that as a fallback. Not sure how we want to go about the fallback enable/disable and if I can just print to the console if its used, so I didn't implement that. Perhaps we should just merge this now since it fixes the plugin's compatibility with Paper 1.21 servers, and think about the rest later?

[0dinD]

Yeah my vote is that we merge this and add an option to disable the fallback in a future PR. This PR will fix #335 so that AnvilGUI works on Paper 1.21 servers (right now, it only works on Spigot), which seems like the more urgent issue.

@WesJD Does that seem reasonable?

[WesJD]

Sorry for the delay, yeah this looks good to me. Could you update the patch version and I'll merge.

[spnda]

@WesJD Done, thank you.