PaperMC / Paper

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

getChunk() loads chunk #8302

Closed paul-palmer closed 2 years ago

paul-palmer commented 2 years ago

Expected behavior

Block::getChunk().isLoaded() and Location::getChunk().isLoaded() would not cause a chunk to load (per Spigot behavior).

Observed/Actual behavior

Block::getChunk() method and Location::getChunk() method have the side effect of loading the chunk. It makes it hard to test if a particular block is in a loaded chunk as the act of calling getChunk() in order to call Chunk::isLoaded() loads the chunk.

The only workaround I have so far is block.getWorld().isChunkLoaded(block.getX() >> 4, block.getZ() >> 4) which unfortunately requires me to hard-code the number of blocks in a chunk.

I do not believe that it has always been this way. I have code in a plugin I developed in the 1.18 time frame where I was able to successfully avoid loading chunks unnecessarily by testing block.getChunk.isLoaded().

Steps/models to reproduce

call Block::getChunk() or Location::getChunk() on an unloaded chunk and verify that a ChunkLoadEvent is processed for that chunk.

Plugin and Datapack List

/plugins CMI, CMILib, Craftmatic, PlugManX (Plugman)

/datapack list [vanilla (built-in)] [file/bukkit (world)]

Paper version

This server is running Paper version git-Paper-130 (MC: 1.19.2) (Implementing API version 1.19.2-R0.1-SNAPSHOT) (Git: 4ba43fe) You are running the latest version Previous version: git-Paper-99 (MC: 1.19.1)

Also seen with paper-1.19.1-99

Other

I diagnosed this by dumping a stack trace in a ChunkLoadEvent listener. Below is a sample trace. I do not get any traces like this when running with the latest Spigot image.

[13:14:23] [Server thread/WARN]: at java.base/java.lang.Thread.dumpStack(Thread.java:1380) [13:14:23] [Server thread/WARN]: at Craftmatic.jar//dev.metanoia.craftmatic.plugin.CraftmaticListener.onChunkLoad(CraftmaticListener.java:88) [13:14:23] [Server thread/WARN]: at com.destroystokyo.paper.event.executor.MethodHandleEventExecutor.execute(MethodHandleEventExecutor.java:37) [13:14:23] [Server thread/WARN]: at co.aikar.timings.TimedEventExecutor.execute(TimedEventExecutor.java:80) [13:14:23] [Server thread/WARN]: at org.bukkit.plugin.RegisteredListener.callEvent(RegisteredListener.java:70) [13:14:23] [Server thread/WARN]: at org.bukkit.plugin.SimplePluginManager.callEvent(SimplePluginManager.java:670) [13:14:23] [Server thread/WARN]: at net.minecraft.world.level.chunk.Chunk.loadCallback(Chunk.java:862) [13:14:23] [Server thread/WARN]: at net.minecraft.server.level.PlayerChunk.lambda$updateFutures$19(PlayerChunk.java:767) [13:14:23] [Server thread/WARN]: at net.minecraft.server.level.PlayerChunkMap$CallbackExecutor.run(PlayerChunkMap.java:201) [13:14:23] [Server thread/WARN]: at net.minecraft.server.level.PlayerChunk.a(PlayerChunk.java:777) [13:14:23] [Server thread/WARN]: at net.minecraft.server.level.ChunkMapDistance.a(ChunkMapDistance.java:246) [13:14:23] [Server thread/WARN]: at net.minecraft.server.level.ChunkProviderServer.r(ChunkProviderServer.java:828) [13:14:23] [Server thread/WARN]: at net.minecraft.server.level.ChunkMapDistance.addPriorityTicket(ChunkMapDistance.java:433) [13:14:23] [Server thread/WARN]: at net.minecraft.server.level.ChunkMapDistance.markUrgent(ChunkMapDistance.java:384) [13:14:23] [Server thread/WARN]: at net.minecraft.server.level.ChunkProviderServer.getChunkFutureMainThread(ChunkProviderServer.java:744) [13:14:23] [Server thread/WARN]: at net.minecraft.server.level.ChunkProviderServer.a(ChunkProviderServer.java:648) [13:14:23] [Server thread/WARN]: at net.minecraft.world.level.chunk.IChunkProvider.a(IChunkProvider.java:13) [13:14:23] [Server thread/WARN]: at org.bukkit.craftbukkit.v1_19_R1.CraftWorld.getChunkAt(CraftWorld.java:339) [13:14:23] [Server thread/WARN]: at org.bukkit.craftbukkit.v1_19_R1.CraftWorld.getChunkAt(CraftWorld.java:355) [13:14:23] [Server thread/WARN]: at org.bukkit.craftbukkit.v1_19_R1.block.CraftBlock.getChunk(CraftBlock.java:142) [13:14:23] [Server thread/WARN]: at Craftmatic.jar//dev.metanoia.craftmatic.plugin.CraftingBlock.isLoaded(CraftingBlock.java:140) [13:14:23] [Server thread/WARN]: at Craftmatic.jar//dev.metanoia.craftmatic.plugin.CraftmaticPlugin.onTick(CraftmaticPlugin.java:324) [13:14:23] [Server thread/WARN]: at Craftmatic.jar//dev.metanoia.craftmatic.plugin.CraftmaticPlugin$1.run(CraftmaticPlugin.java:114) [13:14:23] [Server thread/WARN]: at org.bukkit.craftbukkit.v1_19_R1.scheduler.CraftTask.run(CraftTask.java:101) [13:14:23] [Server thread/WARN]: at org.bukkit.craftbukkit.v1_19_R1.scheduler.CraftScheduler.mainThreadHeartbeat(CraftScheduler.java:483) [13:14:23] [Server thread/WARN]: at net.minecraft.server.MinecraftServer.b(MinecraftServer.java:1492) [13:14:23] [Server thread/WARN]: at net.minecraft.server.dedicated.DedicatedServer.b(DedicatedServer.java:446) [13:14:23] [Server thread/WARN]: at net.minecraft.server.MinecraftServer.a(MinecraftServer.java:1416) [13:14:23] [Server thread/WARN]: at net.minecraft.server.MinecraftServer.v(MinecraftServer.java:1192) [13:14:23] [Server thread/WARN]: at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:305) [13:14:23] [Server thread/WARN]: at java.base/java.lang.Thread.run(Thread.java:833)

electronicboy commented 2 years ago

getChunk loading chunks has been the behavior for years, I have wanted API to create a Chunk object without the load for a good while

Owen1212055 commented 2 years ago

See above, you can check if the chunk is loaded by using World#isChunkLoaded.

paul-palmer commented 2 years ago

Even if Paper has been doing this for years, it is a non-beneficial (as far as I can tell) departure from Spigot.

electronicboy commented 2 years ago

https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/CraftWorld.java#215 - This has been the behavior in craftbukkit for as long as those methods have existed

paul-palmer commented 2 years ago

Yes, I already discovered that if I am to support Paper with my plugins, that I must use World#isChunkLoaded. So, is there any way to map block location coordinates into chunk coordinates other than hard-coding my own knowledge about the internal workings of the server (that is, dividing block coordinates by 16).

electronicboy commented 2 years ago

for just Paper, Location#isChunkLoaded; otherwise, you'd need to bitshift and check isChunkLoaded on World

paul-palmer commented 2 years ago

Well, it sounds like nothing is going to change. At least this thread will hang around for the next plugin writer that runs afoul of this.

paul-palmer commented 2 years ago

https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/CraftWorld.java#215 - This has been the behavior in craftbukkit for as long as those methods have existed

I do not know how to reconcile that. I have repeatedly run tests that demonstrated the difference in runtime behavior between current Spigot and current Paper in regards to this chunk loading behavior.

electronicboy commented 2 years ago

Both Location#getChunk and Block#getChunk, call getChunkAt internally, getChunkAt will load a chunk if it's not already loaded

There are many other changes within the system which may be why you're seeing chunks which aren't loaded when you're expecting them to be, but, getChunkAt has always loaded chunks which aren't loaded

paul-palmer commented 2 years ago

Thank you for your attention and explanation on this issue.

qixils commented 2 years ago

getChunk loading chunks has been the behavior for years, I have wanted API to create a Chunk object without the load for a good while

Sounds like the issue should be left open then until such an API is implemented? 🙂
(Unless there's already an issue tracking that, of course)

lynxplay commented 2 years ago

Should really be documented in a new feature issue with a reference to this one imo. If it's important enough to be tracked in a feature request rn.