Feat/more bridges #210
Feat/more bridges #210
Conversation
…command argument handling
…mmandArgumentTypesBridge
There was a problem hiding this comment.
Pull request overview
This PR expands the Bukkit command/NMS bridging layer to support NBT compound tags (Adventure NBT) as command arguments and enables entity creation from NBT, while also centralizing build/toolchain configuration and updating build/dependency wiring.
Changes:
- Added an Adventure
CompoundBinaryTagCommandAPI argument type plus NMS bridge(s) to parse/convert NBT. - Introduced new Bukkit NMS bridges (command argument types, entity creation) and a plugin-test summon command using NBT.
- Centralized Java toolchain + ShadowJar relocation behavior in buildSrc and adjusted module build scripts/dependencies accordingly.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| surf-api-velocity/surf-api-velocity-server/build.gradle.kts | Removes per-module Adventure NBT relocation (now centralized). |
| surf-api-standalone/build.gradle.kts | Removes per-module ShadowJar relocation block for Adventure NBT. |
| surf-api-gradle-plugin/surf-api-processor/build.gradle.kts | Applies new Java toolchain convention plugin. |
| surf-api-gradle-plugin/build.gradle.kts | Applies new Java toolchain convention plugin. |
| surf-api-core/surf-api-core-server/src/main/java/.../SurfInvocationHandlerJava.java | Improves proxy invocation (notably method-handle invocation shape + method lookup fallback). |
| surf-api-core/surf-api-core-server/build.gradle.kts | Adds Byte Buddy dependency (currently exposed as api). |
| surf-api-bukkit/.../VanillaArgumentProviderProxy.kt | Adds reflection proxy for Paper’s VanillaArgumentProvider. |
| surf-api-bukkit/.../VanillaArgumentProviderImplProxy.kt | Adds reflection proxy for Paper’s VanillaArgumentProviderImpl.wrap. |
| surf-api-bukkit/.../Reflection.kt | Registers new reflection proxies. |
| surf-api-bukkit/.../nms-extensions.kt | Adds EntityType.toNmsHolder() helper. |
| surf-api-bukkit/.../AdventureNBT.kt | Adds Adventure NBT ⇄ NMS NBT conversion utilities. |
| surf-api-bukkit/.../SurfBukkitNmsEntityBridgeImpl.kt | Implements entity creation from NBT via NMS SummonCommand. |
| surf-api-bukkit/.../SurfBukkitNmsCommandArgumentTypesBridgeImpl.kt | Implements NMS argument type access + Byte Buddy adapter for Paper converter interface. |
| surf-api-bukkit/surf-api-bukkit-server/build.gradle.kts | Removes per-module Adventure NBT relocation (now centralized). |
| surf-api-bukkit-plugin-test/.../SummonCommandTest.kt | Adds test command to summon entities with NBT. |
| surf-api-bukkit-plugin-test/.../SurfApiTestCommand.java | Registers the new summon test subcommand. |
| surf-api-bukkit-plugin-test/build.gradle.kts | Enables downloading CommandAPI from Hangar again. |
| surf-api-bukkit-api/.../bukkit-util.kt | Adds chunk coordinate extensions for Position. |
| surf-api-bukkit-api/.../SurfBukkitNmsEntityBridge.kt | Adds public NMS entity bridge API. |
| surf-api-bukkit-api/.../SurfBukkitNmsCommandArgumentTypesBridge.kt | Adds public NMS command argument types bridge API. |
| surf-api-bukkit-api/.../AdventureCompoundBinaryTagArgument.kt | Adds public CommandAPI argument for Adventure CompoundBinaryTag. |
| surf-api-bukkit-api/api/surf-api-bukkit-api.api | Updates ABI dump for newly exposed public APIs. |
| gradle/wrapper/gradle-wrapper.properties | Updates Gradle wrapper distribution URL to 9.4.0-milestone-5. |
| gradle/libs.versions.toml | Adds Byte Buddy version + library entry. |
| gradle.properties | Bumps project version. |
| buildSrc/src/main/kotlin/java-toolchain-convention.gradle.kts | Adds new Java toolchain convention plugin (currently contains build-breaking issues). |
| buildSrc/src/main/kotlin/core-convention.gradle.kts | Applies toolchain convention + centralizes Adventure NBT relocation. |
| @@ -0,0 +1,18 @@ | |||
| import gradle.kotlin.dsl.accessors._bcd9a993373509de50154c5485fe667f.java | |||
There was a problem hiding this comment.
The precompiled script plugin imports an internal generated accessor (gradle.kotlin.dsl.accessors._….java). This class name is Gradle-version-specific and can break the build when the accessor package changes; the plugin should not depend on it. Remove this import and rely on the normal java { ... } DSL after applying the java plugin.
| import gradle.kotlin.dsl.accessors._bcd9a993373509de50154c5485fe667f.java |
|
|
||
| java { | ||
| toolchain { | ||
| languageVersion.set(JavaLanguageVersion.of(javaVersion)) |
There was a problem hiding this comment.
JavaLanguageVersion.of(javaVersion) is passed a String, but Gradle’s toolchain API expects an integer language version. This will fail at script compilation time unless you convert to Int (or use JavaLanguageVersion.of(javaVersion.toInt())).
| languageVersion.set(JavaLanguageVersion.of(javaVersion)) | |
| languageVersion.set(JavaLanguageVersion.of(javaVersion.toInt())) |
| api(project(":surf-api-core:surf-api-core-api")) | ||
| api(project(":surf-api-shared:surf-api-shared-internal")) | ||
| compileOnly(libs.packetevents.netty.common) | ||
| api(libs.bytebuddy) |
There was a problem hiding this comment.
SurfBukkitNmsCommandArgumentTypesBridgeImpl uses Byte Buddy, but this module doesn’t declare a direct dependency on it; instead surf-api-core-server now exposes Byte Buddy as an api dependency. Since core-server doesn’t use Byte Buddy, this should be moved to the Bukkit server module (and likely be implementation), to avoid leaking Byte Buddy to all core-server consumers.
| api(libs.bytebuddy) |
| val nmsTag = when (adventure) { | ||
| is IntBinaryTag -> IntTag.valueOf(adventure.value()) | ||
| is ByteBinaryTag -> ByteTag.valueOf(adventure.value()) | ||
| is FloatBinaryTag -> FloatTag.valueOf(adventure.value()) | ||
| is LongBinaryTag -> LongTag.valueOf(adventure.value()) | ||
| is DoubleBinaryTag -> DoubleTag.valueOf(adventure.value()) | ||
| is ShortBinaryTag -> ShortTag.valueOf(adventure.value()) | ||
| is StringBinaryTag -> StringTag.valueOf(adventure.value()) | ||
| is ByteArrayBinaryTag -> ByteArrayTag(adventure.value()) | ||
| is IntArrayBinaryTag -> IntArrayTag(adventure.value()) | ||
| is LongArrayBinaryTag -> LongArrayTag(adventure.value()) | ||
| is EndBinaryTag -> EndTag.INSTANCE | ||
| is ListBinaryTag -> if (adventure.isEmpty) { | ||
| ListTag() | ||
| } else { | ||
| val list = ListTag() | ||
| for (entry in adventure) { | ||
| val nms = toNms(entry) | ||
| list.add(nms) | ||
| } | ||
| list | ||
| } | ||
|
|
||
| is CompoundBinaryTag -> { | ||
| val tag = CompoundTag() | ||
| for ((key, entry) in adventure) { | ||
| val nms = toNms(entry) | ||
| tag.put(key, nms) | ||
| } | ||
| tag | ||
| } | ||
|
|
||
| else -> throw IllegalArgumentException("Unsupported tag type: ${adventure::class}") | ||
| } | ||
|
|
||
| accounter.popDepth() | ||
|
|
||
| return nmsTag |
There was a problem hiding this comment.
toNms(BinaryTag, accounter) calls accounter.pushDepth() but popDepth() is only reached on the success path. If conversion throws (e.g., unsupported tag), depth is never popped. Use try/finally so popDepth() always executes.
| val nmsTag = when (adventure) { | |
| is IntBinaryTag -> IntTag.valueOf(adventure.value()) | |
| is ByteBinaryTag -> ByteTag.valueOf(adventure.value()) | |
| is FloatBinaryTag -> FloatTag.valueOf(adventure.value()) | |
| is LongBinaryTag -> LongTag.valueOf(adventure.value()) | |
| is DoubleBinaryTag -> DoubleTag.valueOf(adventure.value()) | |
| is ShortBinaryTag -> ShortTag.valueOf(adventure.value()) | |
| is StringBinaryTag -> StringTag.valueOf(adventure.value()) | |
| is ByteArrayBinaryTag -> ByteArrayTag(adventure.value()) | |
| is IntArrayBinaryTag -> IntArrayTag(adventure.value()) | |
| is LongArrayBinaryTag -> LongArrayTag(adventure.value()) | |
| is EndBinaryTag -> EndTag.INSTANCE | |
| is ListBinaryTag -> if (adventure.isEmpty) { | |
| ListTag() | |
| } else { | |
| val list = ListTag() | |
| for (entry in adventure) { | |
| val nms = toNms(entry) | |
| list.add(nms) | |
| } | |
| list | |
| } | |
| is CompoundBinaryTag -> { | |
| val tag = CompoundTag() | |
| for ((key, entry) in adventure) { | |
| val nms = toNms(entry) | |
| tag.put(key, nms) | |
| } | |
| tag | |
| } | |
| else -> throw IllegalArgumentException("Unsupported tag type: ${adventure::class}") | |
| } | |
| accounter.popDepth() | |
| return nmsTag | |
| try { | |
| val nmsTag = when (adventure) { | |
| is IntBinaryTag -> IntTag.valueOf(adventure.value()) | |
| is ByteBinaryTag -> ByteTag.valueOf(adventure.value()) | |
| is FloatBinaryTag -> FloatTag.valueOf(adventure.value()) | |
| is LongBinaryTag -> LongTag.valueOf(adventure.value()) | |
| is DoubleBinaryTag -> DoubleTag.valueOf(adventure.value()) | |
| is ShortBinaryTag -> ShortTag.valueOf(adventure.value()) | |
| is StringBinaryTag -> StringTag.valueOf(adventure.value()) | |
| is ByteArrayBinaryTag -> ByteArrayTag(adventure.value()) | |
| is IntArrayBinaryTag -> IntArrayTag(adventure.value()) | |
| is LongArrayBinaryTag -> LongArrayTag(adventure.value()) | |
| is EndBinaryTag -> EndTag.INSTANCE | |
| is ListBinaryTag -> if (adventure.isEmpty) { | |
| ListTag() | |
| } else { | |
| val list = ListTag() | |
| for (entry in adventure) { | |
| val nms = toNms(entry) | |
| list.add(nms) | |
| } | |
| list | |
| } | |
| is CompoundBinaryTag -> { | |
| val tag = CompoundTag() | |
| for ((key, entry) in adventure) { | |
| val nms = toNms(entry) | |
| tag.put(key, nms) | |
| } | |
| tag | |
| } | |
| else -> throw IllegalArgumentException("Unsupported tag type: ${adventure::class}") | |
| } | |
| return nmsTag | |
| } finally { | |
| accounter.popDepth() | |
| } |
| is ListBinaryTag -> if (adventure.isEmpty) { | ||
| ListTag() | ||
| } else { | ||
| val list = ListTag() | ||
| for (entry in adventure) { | ||
| val nms = toNms(entry) | ||
| list.add(nms) | ||
| } | ||
| list | ||
| } |
There was a problem hiding this comment.
Recursive conversions here call toNms(entry) without passing the provided accounter, so nested tags ignore the caller’s accounting/depth tracking. Thread the same accounter into recursive calls (e.g., toNms(entry, accounter)).
| class InterceptorHolder<B : Any, C : Any>( | ||
| private val converter: OpenedResultConverter<B, C> | ||
| ) { | ||
| @RuntimeType | ||
| @Throws(Exception::class) | ||
| fun convert(@RuntimeType input: B): C { | ||
| return converter.convert(input) | ||
| } | ||
| } |
There was a problem hiding this comment.
InterceptorHolder is currently unused (and its Byte Buddy annotations aren’t applied anywhere). Removing it (or wiring it into the generation approach) would reduce dead code and confusion about the intended interception mechanism.
| val type: EntityType by args | ||
| val nbt: CompoundBinaryTag by args | ||
|
|
||
| entityBridge.createEntityByNbt(sender.world, type, sender.location, nbt) |
There was a problem hiding this comment.
This command passes sender.location (a Bukkit Location) into createEntityByNbt, which expects a FinePosition. This won’t compile (and would be an unsafe cast at runtime). Convert the location to a FinePosition (or change the bridge API to accept Location/Position).
| entityBridge.createEntityByNbt(sender.world, type, sender.location, nbt) | |
| entityBridge.createEntityByNbt(sender.world, type, sender.location.toFinePosition(), nbt) |
| object OpenedResultConverterImpl { | ||
| private val converterCache = ConcurrentHashMap<String, Any>() | ||
|
|
||
| @Suppress("UNCHECKED_CAST") | ||
| fun <B : Any, C : Any> of(converter: OpenedResultConverter<B, C>): Any { | ||
| val cacheKey = "${converter.javaClass.name}_${System.identityHashCode(converter)}" | ||
|
|
||
| return converterCache.computeIfAbsent(cacheKey) { |
There was a problem hiding this comment.
This converterCache is a static ConcurrentHashMap of generated converter instances, and each generated instance/handler strongly captures the original converter. That can leak converters and (more importantly) plugin classloaders across reloads. Prefer weak keys/values (e.g., WeakHashMap with WeakReference values) or avoid caching unless profiling shows it’s necessary.
| val handler = InvocationHandler { _, method, args -> | ||
| when (method.name) { | ||
| "convert" -> converter.convert(args[0] as B) | ||
| else -> throw UnsupportedOperationException("Unknown method: ${method.name}") | ||
| } | ||
| } |
There was a problem hiding this comment.
The generated proxy intercepts all methods and the handler throws for anything except convert. If toString/equals/hashCode are called (often happens implicitly), this will throw UnsupportedOperationException. Handle Object methods explicitly, or only intercept the convert method.
| .subclass(Any::class.java) | ||
| .implement(resultConverterInterface) | ||
| .name("io.papermc.paper.command.brigadier.argument.GeneratedResultConverter\$${System.nanoTime()}") | ||
| .method(ElementMatchers.any()) |
There was a problem hiding this comment.
ElementMatchers.any() will route every method (including Object methods) through the throwing InvocationHandler. Even if you add handling, it’s safer to match only the intended convert method to reduce unexpected interception surface.
| .method(ElementMatchers.any()) | |
| .method(ElementMatchers.named("convert")) |
1 participant
This pull request introduces new features and improvements related to handling NBT (Named Binary Tag) data in commands, adds new NMS (Net Minecraft Server) bridge interfaces, and updates build tooling and dependencies. The most significant changes include the addition of a new command argument type for NBT compounds, new NMS bridge interfaces for command arguments and entity creation, and a new test command for entity summoning with NBT data. There are also updates to Gradle build scripts, including a new Java toolchain convention and dependency updates.
New NBT Command Argument and Utilities
AdventureCompoundBinaryTagArgumentand related extension functions to allow commands to accept and parse NBT compound tags as arguments (AdventureCompoundBinaryTagArgument.kt).surf-api-bukkit-api.api).NMS Bridge Interfaces
SurfBukkitNmsCommandArgumentTypesBridgeinterface and singleton for accessing NMS-level command argument types, including methods for handling NBT compound tags (SurfBukkitNmsCommandArgumentTypesBridge.kt). [1] [2]SurfBukkitNmsEntityBridgeinterface and singleton to support creating entities from NBT data (SurfBukkitNmsEntityBridge.kt). [1] [2]Test Command Enhancements
SummonCommandTestto test entity summoning with NBT data, and registered it in the main test command (SummonCommandTest.kt,SurfApiTestCommand.java). [1] [2] [3]Build System and Dependency Updates
java-toolchain-conventionGradle plugin for consistent Java version/toolchain configuration across modules (java-toolchain-convention.gradle.kts,core-convention.gradle.kts). [1] [2] [3]gradle-wrapper.properties,gradle.properties). [1] [2]libs.versions.toml). [1] [2]Utility Improvements
Positionobjects in Bukkit utilities for easier chunk math (bukkit-util.kt,surf-api-bukkit-api.api). [1] [2] [3]These changes improve the flexibility and capability of the command API, especially for advanced use cases involving NBT data and entity manipulation, and modernize the build setup for better maintainability.