PaperMC / Paper

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

Player#openEnchanting method does not respect the world #9943

Open KioProject123 opened 1 year ago

KioProject123 commented 1 year ago

Expected behavior

Location location = new Location(Bukkit.getWorlds().get(0), 0.0, 0.0, 0.0);
player.openEnchanting(location, true);

If there is a level 30 enchanting table at this location, the player can open the level 30 enchanting table normally at any location.

Observed/Actual behavior

If the player is in the overworld, the level 30 enchanting table will be opened. If the player is in the Nether or the End, the lowest level enchanting table will be opened.

Steps/models to reproduce

1、Place a level 30 enchanting table at 0,0,0 in the overworld 2、Travel to the Nether or the End 3、Execute the above code

Plugin and Datapack List

Only plugins for testing

Paper version

This server is running Paper version git-Paper-290 (MC: 1.20.2) (Implementing API version 1.20.2-R0.1-SNAPSHOT) (Git: f186318) You are running the latest version

Other

https://github.com/PaperMC/Paper/blob/f186318a91cbd3b2a2259d39cb88576989a496b8/patches/server/0911-Fix-force-opening-enchantment-tables.patch#L17C141-L17C141 It looks like the world the player is in is used when opening the enchanting table, rather than the world specified by the coordinates. All openXXX series methods have the same issue, but may only affect the enchanting table (Because it has levels)

Sytm commented 12 months ago

At the moment all openXXX methods on HumanEntity creating real inventories discard the world of the provided location and instead use the world of the player. It could be easily done to respect the provided world, but a general issue of these methods is that some of these inventories try to access the actual blocks in the world at some point of time and if the block is in an unloaded chunk, same world or another, a sync main thread chunk load will occur which isn't really desirable.

So I guess at the moment an implied contract is that the linked location should be in a loaded chunk, preferably close to the player

electronicboy commented 12 months ago

opening inventories cross world was never an intended usecase for these methods, it was always intended that either you'd be opening a "fake" inventory to a player backed by real world logic, or something actually within some reasonable boundary of them; I don't think that this will be fixed, mainly inso is that if we support it, we need to ensure that it's safe to do and won't induce potential state issues, i.e. the chunk unloads, the world unloads, etc

Sytm commented 12 months ago

Should some sanity checks be added to these methods that ensure that the location is in the same world as the player and maybe a chunk loaded or even a distance check to prevent misuse? Or amend the JavaDocs to explicitly state these restrictions and possible caveats?