Skip to content

Implement dynamic remapping for InventoryView methods #229

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.

Sign up for GitHub

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

Merged
ammodev merged 1 commit into from
Mar 2, 2026
+109 −1
Merged

Implement dynamic remapping for InventoryView methods #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ org.jetbrains.dokka.experimental.gradle.pluginMode=V2Enabled
javaVersion=25
mcVersion=1.21.11
group=dev.slne.surf
version=1.21.11-2.59.3
version=1.21.11-2.59.4
relocationPrefix=dev.slne.surf.surfapi.libs
snapshot=false
4 changes: 4 additions & 0 deletions ...rc/main/kotlin/dev/slne/surf/surfapi/bukkit/server/inventory/framework/InventoryLoader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import dev.slne.surf.surfapi.bukkit.server.plugin
import me.devnatan.inventoryframework.ViewFrame

object InventoryLoader {
init {
InventoryViewRemapper.remap()
Comment on lines 5 to +8
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling InventoryViewRemapper.remap() in the object initializer means any exception during remapping will prevent InventoryLoader from initializing (and then break InventoryLoader.load() / enable() calls). Even if remap() is made more defensive, it’s safer to ensure failures here are contained (e.g., handle/log exceptions inside remap() or around this call) so inventory framework startup doesn’t hard-fail.

Suggested change
object InventoryLoader {
init {
InventoryViewRemapper.remap()
import java.util.logging.Level
object InventoryLoader {
init {
try {
InventoryViewRemapper.remap()
} catch (t: Throwable) {
plugin.logger.log(
Level.SEVERE,
"Failed to remap inventory views during InventoryLoader initialization. " +
"Inventory framework will continue to load, but some inventories may not behave as expected.",
t
)
}

Copilot uses AI. Check for mistakes.
}

lateinit var viewFrame: ViewFrame

fun load() {
Expand Down
104 changes: 104 additions & 0 deletions ...n/kotlin/dev/slne/surf/surfapi/bukkit/server/inventory/framework/InventoryViewRemapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
package dev.slne.surf.surfapi.bukkit.server.inventory.framework

import net.bytebuddy.ByteBuddy
import net.bytebuddy.asm.AsmVisitorWrapper
import net.bytebuddy.description.field.FieldDescription
import net.bytebuddy.description.field.FieldList
import net.bytebuddy.description.method.MethodList
import net.bytebuddy.description.type.TypeDescription
import net.bytebuddy.dynamic.ClassFileLocator
import net.bytebuddy.dynamic.loading.ClassLoadingStrategy
import net.bytebuddy.implementation.Implementation
import net.bytebuddy.jar.asm.ClassVisitor
import net.bytebuddy.jar.asm.MethodVisitor
import net.bytebuddy.jar.asm.Opcodes
import net.bytebuddy.pool.TypePool
import net.bytebuddy.utility.OpenedClassReader
import org.bukkit.inventory.InventoryView

object InventoryViewRemapper {
private const val INVENTORY_VIEW_INTERNAL = "org/bukkit/inventory/InventoryView"
private const val INVENTORY_UPDATE_INTERNAL =
"dev/slne/surf/surfapi/libs/devnatan/inventoryframework/runtime/thirdparty/InventoryUpdate"
Comment on lines +18 to +22
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INVENTORY_UPDATE_INTERNAL hard-codes the relocated internal class name, which is tightly coupled to the Shadow relocation configuration (relocationPrefix + relocate("me.devnatan.inventoryframework", ...)). This is easy to break if the relocation prefix or library package structure changes. Consider deriving this name from an actual type reference (if available) or centralizing the relocated prefix/string in one place so build config and runtime remapping can’t drift.

Suggested change
object InventoryViewRemapper {
private const val INVENTORY_VIEW_INTERNAL = "org/bukkit/inventory/InventoryView"
private const val INVENTORY_UPDATE_INTERNAL =
"dev/slne/surf/surfapi/libs/devnatan/inventoryframework/runtime/thirdparty/InventoryUpdate"
import dev.slne.surf.surfapi.libs.devnatan.inventoryframework.runtime.thirdparty.InventoryUpdate
object InventoryViewRemapper {
private val INVENTORY_VIEW_INTERNAL: String = InventoryView::class.java.name.replace('.', '/')
private val INVENTORY_UPDATE_INTERNAL: String =
InventoryUpdate::class.java.name.replace('.', '/')

Copilot uses AI. Check for mistakes.

fun isRemapNecessary(): Boolean {
return try {
InventoryView::class.java.isInterface
} catch (_: Throwable) {
false
}
}

fun remap() {
if (!isRemapNecessary()) return

val locator = ClassFileLocator.ForClassLoader.of(javaClass.classLoader)
val typePool = TypePool.Default.of(locator)
val typeDescription = typePool.describe(INVENTORY_UPDATE_INTERNAL.replace("/", ".")).resolve()

ByteBuddy()
.redefine<Any>(typeDescription, locator)
.visit(InventoryViewClassVisitorWrapper())
.make()
.load(javaClass.classLoader, ClassLoadingStrategy.Default.INJECTION)
}
Comment on lines +32 to +44
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remap() can throw (e.g., TypePool#resolve() if the class name changes / isn't present, or load(...) if injection fails). Because this runs during plugin startup, any uncaught exception here will fail inventory framework initialization (or even plugin load). Consider wrapping the resolve/redefine/load sequence in a try/catch and logging a warning/severe message so the plugin can continue (ideally with a clear indication that InventoryView compatibility patching failed).

Copilot uses AI. Check for mistakes.

private class InventoryViewClassVisitorWrapper : AsmVisitorWrapper {
override fun mergeWriter(flags: Int): Int = flags
override fun mergeReader(flags: Int): Int = flags

override fun wrap(
instrumentedType: TypeDescription,
classVisitor: ClassVisitor,
implementationContext: Implementation.Context,
typePool: TypePool,
fields: FieldList<FieldDescription.InDefinedShape?>,
methods: MethodList<*>,
writerFlags: Int,
readerFlags: Int
): ClassVisitor {
return InventoryViewClassVisitor(classVisitor)
}
}

private class InventoryViewClassVisitor(
visitor: ClassVisitor
) : ClassVisitor(OpenedClassReader.ASM_API, visitor) {

override fun visitMethod(
access: Int,
name: String,
descriptor: String,
signature: String?,
exceptions: Array<String>?
): MethodVisitor {
val mv = super.visitMethod(access, name, descriptor, signature, exceptions)
return InventoryViewMethodVisitor(mv)
}
}

private class InventoryViewMethodVisitor(
visitor: MethodVisitor
) : MethodVisitor(OpenedClassReader.ASM_API, visitor) {

override fun visitMethodInsn(
opcode: Int,
owner: String,
name: String,
descriptor: String,
isInterface: Boolean
) {
if (opcode == Opcodes.INVOKEVIRTUAL && owner == INVENTORY_VIEW_INTERNAL) {
super.visitMethodInsn(
Opcodes.INVOKEINTERFACE,
owner,
name,
descriptor,
true
)
} else {
super.visitMethodInsn(opcode, owner, name, descriptor, isInterface)
}
}
}
}