search

PaperMC / Paper

The most widely used, high performance Minecraft server that aims to fix gameplay and mechanics inconsistencies
https://papermc.io/
Other
9.7k stars 2.27k forks source link

Faulty implementation of Server#getRegistry(Class) #11421

Open Jannyboy11 opened 2 hours ago

Jannyboy11 commented 2 hours ago

Expected behavior

Per Bukkit Javadocs, the Server#getRegistry(Class) method returns null when a registry is not available for the given Keyed class.

Observed/Actual behavior

Paper asserts that a registry must exists and throws a NullPointerException:

[10:40:39 ERROR]: Exception when Jannyboy11 attempted to tab complete Jannyboy11
org.bukkit.command.CommandException: Unhandled exception during tab completion for command '/igive Jannyboy11 ' in plugin InvSeePlusPlus_Give v0.29.6-SNAPSHOT
        at org.bukkit.command.PluginCommand.tabComplete(PluginCommand.java:150) ~[paper-mojangapi-1.21.1-R0.1-SNAPSHOT.jar:?]
        at org.bukkit.command.Command.tabComplete(Command.java:101) ~[paper-mojangapi-1.21.1-R0.1-SNAPSHOT.jar:?]
        at io.papermc.paper.command.brigadier.bukkit.BukkitCommandNode$BukkitBrigSuggestionProvider.getSuggestions(BukkitCommandNode.java:118) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at com.mojang.brigadier.tree.ArgumentCommandNode.listSuggestions(ArgumentCommandNode.java:71) ~[brigadier-1.3.10.jar:1.21.1-DEV-925c3b9]
        at com.mojang.brigadier.CommandDispatcher.getCompletionSuggestions(CommandDispatcher.java:551) ~[paper-1.21.1.jar:?]
        at com.mojang.brigadier.CommandDispatcher.getCompletionSuggestions(CommandDispatcher.java:531) ~[paper-1.21.1.jar:?]
        at net.minecraft.server.network.ServerGamePacketListenerImpl.sendServerSuggestions(ServerGamePacketListenerImpl.java:870) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.network.ServerGamePacketListenerImpl.lambda$handleCustomCommandSuggestions0$2(ServerGamePacketListenerImpl.java:831) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.TickTask.run(TickTask.java:18) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.util.thread.BlockableEventLoop.doRunTask(BlockableEventLoop.java:151) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.util.thread.ReentrantBlockableEventLoop.doRunTask(ReentrantBlockableEventLoop.java:24) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.MinecraftServer.doRunTask(MinecraftServer.java:1537) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.MinecraftServer.doRunTask(MinecraftServer.java:201) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.util.thread.BlockableEventLoop.pollTask(BlockableEventLoop.java:125) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.MinecraftServer.pollTaskInternal(MinecraftServer.java:1514) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.MinecraftServer.pollTask(MinecraftServer.java:1507) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.util.thread.BlockableEventLoop.managedBlock(BlockableEventLoop.java:135) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.MinecraftServer.managedBlock(MinecraftServer.java:1466) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.MinecraftServer.waitUntilNextTick(MinecraftServer.java:1473) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:1318) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:330) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at java.base/java.lang.Thread.run(Thread.java:1583) ~[?:?]
Caused by: java.lang.NullPointerException: class org.bukkit.Material is not a valid registry type
        at java.base/java.util.Objects.requireNonNull(Objects.java:360) ~[?:?]
        at io.papermc.paper.registry.PaperRegistryAccess.getRegistry(PaperRegistryAccess.java:48) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at org.bukkit.craftbukkit.CraftServer.getRegistry(CraftServer.java:2946) ~[paper-1.21.1.jar:1.21.1-DEV-925c3b9]
        at org.bukkit.Bukkit.getRegistry(Bukkit.java:2588) ~[paper-mojangapi-1.21.1-R0.1-SNAPSHOT.jar:?]
        at InvSee++.jar/com.janboerman.invsee.spigot.impl_1_21_1_R1.InvseeImpl.materials(InvseeImpl.java:344) ~[InvSee++.jar:?]
        at InvSee++.jar/com.janboerman.invsee.spigot.InvseePlusPlus.materials(InvseePlusPlus.java:221) ~[InvSee++.jar:?]
        at InvSee++_Give.jar/com.janboerman.invsee.spigot.addon.give.GiveTabCompleter.onTabComplete(GiveTabCompleter.java:47) ~[InvSee++_Give.jar:?]
        at org.bukkit.command.PluginCommand.tabComplete(PluginCommand.java:138) ~[paper-mojangapi-1.21.1-R0.1-SNAPSHOT.jar:?]
        ... 21 more

Steps/models to reproduce

  1. Install my plugins InvSee++ and InvSee++_Give: https://hangar.papermc.io/Jannyboy11/InvSee-plus-plus
  2. Use the command /invgive Notch and then press tab to tabcomplete materials.

Plugin and Datapack List

Plugins: InvSeePlusPlus, InvSeePlusPlus_Give

No datapacks were used.

Paper version

This server is running Paper version 1.21.1-DEV-master@925c3b9 (2024-09-08T17:37:37Z) (Implementing API version 1.21.1-R0.1-SNAPSHOT)

I know this is running 16 versions behind, but the bug seems to be present still, per this patch: https://github.com/PaperMC/Paper/blob/master/patches/server/0471-Add-RegistryAccess-for-managing-Registries.patch#L878

Other

Per this message on the PaperMC discord, I implemented this new method to obtain the set of available materials on the server, as can be seen in the source code of my plugin here (using the 'upstream compatible' api): https://github.com/Jannyboy11/InvSee-plus-plus/blob/5c09d698fa74d302ae88d50dcdc02a79036b0735/InvSee%2B%2B_Platforms/Impl_1_21_1_R1/src/main/java/com/janboerman/invsee/spigot/impl_1_21_1_R1/InvseeImpl.java#L344

If this is the incorrect way to do it, then I'd like to know.

electronicboy commented 2 hours ago

Material isn't backed by an registry, so, I'm not really sure what the expected behavior would be otherwise?

Jannyboy11 commented 2 hours ago

Material isn't backed by an registry, so, I'm not really sure what the expected behavior would be otherwise?

To not have an exception thrown.

lynxplay commented 2 hours ago

The method should still yield null if that is the method contract instead of throwing an exception yea. The entire "registry lookup by class type" is deprecated af on paper anyway, fixing this won't have a negative impact on the paper registry API alterantives.

Jannyboy11 commented 2 hours ago

So what is the correct way to obtain all available Materials on Paper? Continue to use Material.values()?

electronicboy commented 2 hours ago

Yes, as Materials are not a registry concept, that's a bukkit/spigot concept, mojang doesn't have a registry of all Materials

lynxplay commented 2 hours ago

BukkitägetRegistry(Material.class) will always return null, so yea. Will still fix that it is throwing instead of yielding null, so, pls keep the issue open.

Jannyboy11 commented 2 hours ago

Yes, as Materials are not a registry concept, that's a bukkit/spigot concept, mojang doesn't have a registry of all Materials

So this announcement on the PaperMC Discord is fake news then? I quote:

Going beyond this, please especially make sure you move away from Material.values() calls and instead get them [via the Registry API](https://jd.papermc.io/paper/1.21/io/papermc/paper/registry/RegistryAccess.html#getRegistry(io.papermc.paper.registry.RegistryKey)) (or the [upstream compatible one](https://jd.papermc.io/paper/1.21/org/bukkit/Server.html#getRegistry(java.lang.Class)))
kennytv commented 2 hours ago

We'll fix it, but that would be

Registry.MATERIAL

or if you already want to use the future/wip api

for (final ItemType itemType : Registry.ITEM) {
   ...
}
// or getting it via the key, which is slightly more future-proof
RegistryAccess.registryAccess().getRegistry(RegistryKey.BLOCK)...

I also edited the Discord announcement to clarify these