Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split 1.17 module into 1.17-common, 1.17, and 1.17.1 #304

Merged
merged 2 commits into from
Jul 22, 2022
Merged

Split 1.17 module into 1.17-common, 1.17, and 1.17.1 #304

merged 2 commits into from
Jul 22, 2022

Conversation

willkroboth
Copy link
Collaborator

This PR takes the old commandapi-1.17 module and splits it into three modules: commandapi-1.17-common, commandapi-1.17, and commandapi-1.17.1.
This is necessary because the field MinecraftServer.resources (accessed in reloadDataPacks()) is mapped to two different names (aB and aC) for the two versions 1.17 and 1.17.1. This caused the following exception on a 1.17 Spigot Server:

[15:14:47] [Server thread/INFO]: [CommandAPI] Reloading datapacks...
[15:14:47] [Server thread/WARN]: [CommandAPI] Task #2 for CommandAPI v8.4.1 generated an exception
java.lang.NoSuchFieldError: aB
    at dev.jorel.commandapi.nms.NMS_1_17_R1.reloadDataPacks(NMS_1_17_R1.java:647) ~[?:?]
    at dev.jorel.commandapi.CommandAPI.lambda$onEnable$0(CommandAPI.java:186) ~[?:?]
    at org.bukkit.craftbukkit.v1_17_R1.scheduler.CraftTask.run(CraftTask.java:81) ~[spigot-1.17.jar:3170-Spigot-a483d2c-ec116f6]
    at org.bukkit.craftbukkit.v1_17_R1.scheduler.CraftScheduler.mainThreadHeartbeat(CraftScheduler.java:400) ~[spigot-1.17.jar:3170-Spigot-a483d2c-ec116f6]
    at net.minecraft.server.MinecraftServer.b(MinecraftServer.java:1252) ~[spigot-1.17.jar:3170-Spigot-a483d2c-ec116f6]
    at net.minecraft.server.dedicated.DedicatedServer.b(DedicatedServer.java:436) ~[spigot-1.17.jar:3170-Spigot-a483d2c-ec116f6]
    at net.minecraft.server.MinecraftServer.a(MinecraftServer.java:1200) ~[spigot-1.17.jar:3170-Spigot-a483d2c-ec116f6]
    at net.minecraft.server.MinecraftServer.x(MinecraftServer.java:1027) ~[spigot-1.17.jar:3170-Spigot-a483d2c-ec116f6]
    at net.minecraft.server.MinecraftServer.lambda$0(MinecraftServer.java:307) ~[spigot-1.17.jar:3170-Spigot-a483d2c-ec116f6]
    at java.lang.Thread.run(Thread.java:831) [?:?]

It seems that this is the only difference relevant to the CommandAPI between 1.17 and 1.17.1, though if other differences are found, these classes can definitely be expanded. To cut down redundant code, commandapi-1.17-common contains almost all the NMS methods except for reloadDataPacks(), which is implemented identically in commandapi-1.17 and commandapi-1.17.1, but mapped differently by specialsource.

To allow commandapi-1.17 and commandapi-1.17.1 access commandapi-1.17-common both before and after it is remapped, they depend on both the mojang mapped and unmapped versions of Spigot 1.17.1-R0.1-SNAPSHOT.

I tried to match the code style and project layout, but I'm not sure if I got the annotations correct on the NMS classes. Feel free to change anything that's not quite right!

@JorelAli
Copy link
Owner

Thanks for this - I was not aware Maven would let you compile against two versions of the same dependency.

The NMS class requires a method compatibleVersions() to be implemented, this will need to be implemented in each submodule (i.e. in 1.17 and 1.17.1):

@Override
public String[] compatibleVersions() {
    return new String[] { "1.17", "1.17.1" };
}

@JorelAli
Copy link
Owner

JorelAli commented Jul 22, 2022

Each NMS module should have a test folder with a SafeReflect.java file. This is used for compile-time reflection checks for code that has reflection. The reloadDatapacks() method uses reflection to modify CustomFunctionManager's command dispatcher.

Although we can already assert this won't fail, I'd still like the 1.17 module and 1.17.1 module to independently have this reflection check (as opposed to just commandapi-1.17-common), because the compile-time reflection checks use the spigot libraries used for compile time (i.e. it'll perform reflection checks against spigot 1.17 for commandapi-1.17 and perform reflection checks against spigot 1.17.1 for commandapi-1.17.1).

@JorelAli JorelAli merged commit e66192b into JorelAli:dev/dev Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants