Paper CommandAPI #517

DerEchtePilz · 2024-01-19T15:03:37Z

This PR refactors parts of the CommandAPI so the Adventure library and Bungee library aren't used in the same .jar's anymore and Paper API and Spigot API also aren't used together anymore.
In order to do that, commandapi-bukkit-core only serves as a tool to access NMS so it can be shared and doesn't have to be implemented again for Paper and Spigot.
The new usable platforms are commandapi-paper-(core/shade/plugin) and commandapi-spigot-(core/shade/plugin).

This still requires a lot of work. I am fairly confident that these changes should already make a fairly functional CommandAPI for Paper and Spigot, however, there still is a lot of work left.

This should also invalidate #474 as this refactor puts them into different modules

TODO's:

+ fixed exception appearing in server log, this isn't done with VarHandles but I think because this is a hotfix, it is fine for now. I will handle all this properly on dev/commandapi-paper and #517

willkroboth

Just an initial look at some things.

The hierarchy of Bukkit and Spigot/Paper NMS feels weird to me, though I can't quite say what to do about it. For example, these methods in CommandAPIPaper need to call the static getBukkit method to get the generics to match up. Implementations of NMS call up CommandAPIBukkit#wrapCommandSender, which passes it over to BukkitPlatform#wrapCommandSender. Nothing major, but I just wonder if there is a cleaner way. Maybe I'll do some poking around on my own and see if I find anything.

commandapi-core/src/main/java/dev/jorel/commandapi/CommandAPIPlatform.java

...otlin/commandapi-kotlin-bukkit/src/main/kotlin/dev/jorel/commandapi/kotlindsl/ExecutorDSL.kt

...ndapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java

...ms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/Platform.java

DerEchtePilz · 2024-07-24T18:45:41Z

About your comment, yeah definitely weird at some places.
The main goal was to share as much code as possible while changing inheritances around.
For example, the methods you linked to in the first link are all defined in CommandAPIPlatform but as CommandAPIBukkit is not a CommandAPIPlatform anymore, I left the methods there so CommandAPIPaper and CommandAPISpigot can call these methods.
The wrapCommandSender method is another case of this weird "needs to be implemented platform specific but also needs to be accessible from CommandAPIBukkit" state since the Bukkit NMS classes call this method.
Definitely let me know if you come up with a way to handle this better/different, but this definitely accomplishes the goal of making the Bukkit modules an interface for the core NMS features and implementations.

Less code evident by reduced jar size (`commandapi-paper-plugin` 746.0 kB -> 728.3 kB, `commandapi-spigot-plugin` 813.5 kB -> 789.6 kB, sidenote: why is Spigot so much bigger lol) Tested chat arguments on Spigot and Paper 1.16.5-1.21 to ensure validity CommandAPISpigot and CommandAPIPaper are now interfaces. CommandAPIBukkit/Spigot/Paper all extend/implement BukkitPlatform to establish methods they can use from each other. The Bukkit nms hierarchy is abstract classes. Spigot/Paper nms modules merge the Bukkit NMS class with the Spigot/Paper interface. Preprocessor comments added to clarify some aspects of the structure. pom.xml files combed through to ensure clear and consistent structure and comments. --- Reworked nms pom.xml files so depending on a module propagates its classifier to transitive nms dependencies. E.g. when you depend on `dev.jorel:commandapi-bukkit-1.21:jar:mojang-mapped`, you get `dev.jorel:commandapi-bukkit-nms-common:jar:mojang-mapped`, but when you depend on `dev.jorel:commandapi-bukkit-1.21:jar:` you get `dev.jorel:commandapi-bukkit-nms-common:jar:`. This makes it so transitive dependencies work as expected, and you don't need to redeclare them to get the right classifier. E.g. before, when you depended on when you depend on `dev.jorel:commandapi-bukkit-1.21:jar:`, you would get `dev.jorel:commandapi-bukkit-nms-common:jar:mojang-mapped` in your final jar unless you explicitly added a dependency for `dev.jorel:commandapi-bukkit-nms-common:jar:` that took priority. This was done by getting maven to generate the nms jars like so: - Compile mojang-mapped source files into jar - Shade mojang-mapped dependencies into new jar with mojang-mapped classifier - Remap source files still in default jar to spigot mappings - Shade spigot-mapped dependencies into default jar For reference, the old build process (which is still used if a module does not have any nms dependencies) goes like so: - Compile mojang-mapped source files into jar - Save into new jar with mojang-mapped classifier - Remap source files in default jar to spigot mappings This might have been easier to set up using gradle, but I got it working :P. --- The `CommandAPIBukkit#failWithAdventureComponent` methods were moved to CommandAPIPaper. This was causing a problem, because SpigotNMS implementations that extended CommandAPIBukkit didn't have access to the Component classes since their modules did not include a dependency on paper-api or adventure. `CommandAPIBukkit#failWithBaseComponenet` was moved to CommandAPISpigot to match (which is kinda nice b/c it doesn't say "BaseComponent is deprecated" by paper-api). This means it is no longer possible for a Spigot plugin that shades adventure to access that method (which is perhaps consistent with the chat arguments, but maybe limiting). Alternate solutions that would allow a Spigot plugin to access `failWithAdventureComponent` include adding a paper-api/adventure dependency to the spigot-nms modules or moving the `failWithComponent` methods to a different shared class in bukkit-core. --- TODO: - Ensure preprocessor and brigadier dependencies are not being leaked Further investigation: - Can the load message (e.g. `Loaded platform SpigotNMS_1_16_R3 > NMS_1_16_R3 > CommandAPIBukkit`) be tweaked to include the Spigot/Paper interfaces? - Can the chat preview handler code be merged more? - Should/can Spigot always be spigot-mapped, pre 1.20.5 Paper always be spigot-mapped, and 1.20.5+ Paper always be mojang-mapped? - Should/can there be a plugin and shade dependency that supports Spigot and Paper, like before? - Other things mentioned in added code todos This is kinda code review for #517

willkroboth · 2024-08-24T19:43:28Z

commandapi-annotations/pom.xml

- <executions>
- <execution>
- <goals>
- <goal>testCompile</goal>
- </goals>
- <configuration>
- <annotationProcessors>
- <annotationProcessor>dev.jorel.commandapi.annotations.Annotations</annotationProcessor>
- </annotationProcessors>
- </configuration>
- </execution>
- </executions>


If this build plugin no longer has any executions, does it need to be declared anymore? Could this removal be expanded to also remove the whole build section?

willkroboth · 2024-08-24T19:51:02Z

...mandapi-kotlin-bukkit/src/main/kotlin/dev/jorel/commandapi/kotlindsl/CommandAPICommandDSL.kt

@@ -52,13 +52,15 @@ inline fun CommandAPICommand.location2DArgument(nodeName: String, locationType:
 inline fun CommandAPICommand.rotationArgument(nodeName: String, optional: Boolean = false, block: Argument<*>.() -> Unit = {}): CommandAPICommand = withArguments(RotationArgument(nodeName).setOptional(optional).apply(block))
 inline fun CommandAPICommand.axisArgument(nodeName: String, optional: Boolean = false, block: Argument<*>.() -> Unit = {}): CommandAPICommand = withArguments(AxisArgument(nodeName).setOptional(optional).apply(block))

+/* TODO: Create additional modules for component related arguments


It looks like this TODO (and others with the same comment) were resolved, so this commented-out code can probably be removed now.

willkroboth · 2024-08-24T20:00:29Z

...mandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/BukkitPlatform.java

+
+ @Override
+ default void onLoad(CommandAPIConfig<?> config) {
+ onLoad((CommandAPIBukkitConfig<?>) config);


Should there be a config instanceof CommandAPIBukkitConfig check here before doing the cast, like there is in other classes where a general config object is being loaded by a specific platform? Alternatively, CommandAPISpigot/Paper are already checking the class. So, this method could be removed, and Spigot/Paper would just start from a CommandAPIConfig.

willkroboth · 2024-08-24T20:06:55Z

...ndapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java

- @Unimplemented(because = REQUIRES_MINECRAFT_SERVER)
- public abstract SuggestionProvider<Source> getSuggestionProvider(SuggestionProviders suggestionProvider);
-
- @Override


It looks like many of the methods in CommandAPIBukkit incorrectly had their @Override annotation removed.

Probably a remnant from when CommandAPIBukkit wasn't a CommandAPIPlatform

willkroboth · 2024-08-24T20:10:12Z

...bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkitConfig.java

- @Override
- public CommandAPIBukkitConfig instance() {
- return this;
+ public T skipReloadDatapacks(boolean skip) {


This method seems to have lost its javadocs when it was moved down from further up in the class.

willkroboth · 2024-08-24T20:11:52Z

...bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkitConfig.java

+
+ @Override
+ public abstract T instance();


I don't think this method declaration does anything. It can be removed.

Suggested change

@Override

public abstract T instance();

...mmandapi-spigot/commandapi-spigot-core/src/main/java/dev/jorel/commandapi/nms/SpigotNMS.java

willkroboth · 2024-08-24T21:36:49Z

...-nms/commandapi-paper-nms-common/src/main/java/dev/jorel/commandapi/nms/PaperNMS_Common.java

+
+ @Override
+ public NMS<?> bukkitNMS() {
+ return null;
+ }


This seems odd. This probably shouldn't return null. This method declaration should probably be removed so it can just be inherited normally in the subclasses.

Suggested change

@Override

public NMS<?> bukkitNMS() {

return null;

}

willkroboth · 2024-08-24T21:38:28Z

...ndapi-spigot/commandapi-spigot-core/src/main/java/dev/jorel/commandapi/CommandAPISpigot.java

+
+ @SuppressWarnings("unchecked")
+ public static <Source> CommandAPISpigot<Source> getSpigot() {
+ return (CommandAPISpigot<Source>) spigot;


Should this have a hey, you're calling this too early check like there is in CommandAPIBukkit?

willkroboth · 2024-08-24T21:41:34Z

...ndapi-spigot/commandapi-spigot-core/src/main/java/dev/jorel/commandapi/CommandAPISpigot.java

+ CommandAPI.logError("CommandAPIBukkit was loaded with non-Bukkit config!");
+ CommandAPI.logError("Attempts to access Bukkit-specific config variables will fail!");
+ }
+ onLoad();


It would be clearer this calls the method in CommandAPIBukkit if it used the super keyword. At the moment, I was like, "Wait, where's the no-parameter onLoad method in CommandAPISpigot?" when I saw this.

Suggested change

onLoad();

super.onLoad();

willkroboth · 2024-08-24T21:50:36Z

...ms/commandapi-spigot-nms-common/src/main/java/dev/jorel/commandapi/nms/SpigotNMS_Common.java

+ @Override
+ public abstract BaseComponent[] getChat(CommandContext<CommandSourceStack> cmdCtx, String key) throws CommandSyntaxException;


getChat (and getChatComponent) could be implemented here, as long as they are @Overriden in 1.20.5+, since Serializer#toJson needs the COMMAND_BUILD_CONTEXT then.

NMS_Common on dev/dev and dev/paper/restructure-experiment currently does this.

DerEchtePilz mentioned this pull request Jan 19, 2024

Rename Adventure arguments #474

Closed

2 tasks

DerEchtePilz force-pushed the dev/commandapi-paper branch 2 times, most recently from 46558a5 to 9babcd2 Compare February 9, 2024 20:48

willkroboth mentioned this pull request Feb 10, 2024

Ensure CME does not happen when (un)registering commands on Paper during server runtime #501

Draft

DerEchtePilz force-pushed the dev/commandapi-paper branch from 9babcd2 to 330f113 Compare February 21, 2024 12:37

DerEchtePilz mentioned this pull request Feb 23, 2024

Velocity Documentation #521

Merged

DerEchtePilz force-pushed the dev/commandapi-paper branch from 9102ea7 to bfbe182 Compare February 28, 2024 16:01

DerEchtePilz force-pushed the dev/commandapi-paper branch from bfbe182 to c5d49e1 Compare March 30, 2024 09:24

DerEchtePilz linked an issue Mar 30, 2024 that may be closed by this pull request

CommandAPI paper plugin #414

Open

DerEchtePilz force-pushed the dev/commandapi-paper branch from 4dba32a to 9f28b25 Compare March 30, 2024 13:12

DerEchtePilz force-pushed the dev/commandapi-paper branch 2 times, most recently from b088d05 to a93af70 Compare April 11, 2024 15:10

DerEchtePilz linked an issue Apr 22, 2024 that may be closed by this pull request

The CommandAPI should use MinecraftServer#getCommands instead of MinecraftServer.vanillaCommandDispatcher #406

Open

DerEchtePilz force-pushed the dev/commandapi-paper branch 5 times, most recently from 18e8c28 to f80d789 Compare May 7, 2024 18:49

DerEchtePilz force-pushed the dev/commandapi-paper branch from 36ca865 to acda2d1 Compare May 9, 2024 20:37

DerEchtePilz mentioned this pull request May 12, 2024

Hotfix CommandAPI throwing errors because of Paper's Brigadier command API #555

Merged

DerEchtePilz linked an issue May 13, 2024 that may be closed by this pull request

java.lang.NoSuchFieldError: Class net.minecraft.server.MinecraftServer does not have member field 'net.minecraft.commands.Commands vanillaCommandDispatcher' after upgrading to Paper-1.20.6-b68 #554

Closed

DerEchtePilz force-pushed the dev/commandapi-paper branch 7 times, most recently from f9a99ef to c357912 Compare May 14, 2024 13:17

DerEchtePilz force-pushed the dev/commandapi-paper branch from a81dd6b to 66db468 Compare May 29, 2024 15:44

DerEchtePilz force-pushed the dev/commandapi-paper branch 3 times, most recently from b152dbe to 4625a32 Compare July 24, 2024 15:49

DerEchtePilz marked this pull request as ready for review July 24, 2024 16:37

DerEchtePilz requested review from JorelAli and willkroboth July 24, 2024 16:37

willkroboth requested changes Jul 24, 2024

View reviewed changes

DerEchtePilz force-pushed the dev/commandapi-paper branch 4 times, most recently from fce7eba to b049854 Compare July 26, 2024 14:09

DerEchtePilz force-pushed the dev/commandapi-paper branch 6 times, most recently from ffa8138 to c85f8a7 Compare August 9, 2024 06:27

DerEchtePilz force-pushed the dev/commandapi-paper branch from c85f8a7 to 6b8070a Compare August 11, 2024 17:52

DerEchtePilz added 5 commits August 15, 2024 08:29

Implement a Paper CommandAPI version

0a4631d

Update documentation references and some other minor changes

6c0015d

Restructured Paper/Spigot NMS hierarchy

d072e1b

Add changelog entry

cf0d977

Add initial draft for the upgrading guide

904dc4b

DerEchtePilz force-pushed the dev/commandapi-paper branch from 6b8070a to 904dc4b Compare August 15, 2024 06:36

Create CommandBuildContext only once in later versions

7d2a622

willkroboth reviewed Aug 24, 2024

View reviewed changes

willkroboth requested changes Aug 24, 2024

View reviewed changes

willkroboth mentioned this pull request Aug 25, 2024

Rework plugin config system #596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paper CommandAPI #517

Paper CommandAPI #517

DerEchtePilz commented Jan 19, 2024 •

edited

Loading

willkroboth left a comment

DerEchtePilz commented Jul 24, 2024

willkroboth Aug 24, 2024

willkroboth Aug 24, 2024

willkroboth Aug 24, 2024

willkroboth Aug 24, 2024

DerEchtePilz Aug 25, 2024

willkroboth Aug 24, 2024

willkroboth Aug 24, 2024

willkroboth Aug 24, 2024

willkroboth Aug 24, 2024

willkroboth Aug 24, 2024

willkroboth Aug 24, 2024

		@Override
		public abstract BaseComponent[] getChat(CommandContext<CommandSourceStack> cmdCtx, String key) throws CommandSyntaxException;

Paper CommandAPI #517

Are you sure you want to change the base?

Paper CommandAPI #517

Conversation

DerEchtePilz commented Jan 19, 2024 • edited Loading

willkroboth left a comment

Choose a reason for hiding this comment

DerEchtePilz commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerEchtePilz commented Jan 19, 2024 •

edited

Loading