Opening an offline player's inventory will reset the player's world when using Paper
KioProject123 opened this issue · 10 comments
Steps/models to reproduce
1、Player A goes to the Nether
2、Player A leaves the server
3、Player B uses the command /openinv A (Requires op permissions)
4、Player B closes the open GUI
5、Player A enters the server, will spawn in the overworld instead of the Nether
I also reported this issue to paper: PaperMC/Paper#9928
Okay, I'm not able to replicate the issue you're having with Bukkit/Paper special data, those exist as expected. They do update on save, which should be rectified, but that's a separate issue. They are properly loaded and saved, just improperly updated.
Looks like Paper removed loading world from loading entity data, so I guess we get to load that separately now. Since Paper now always has a null world,
always resets them to spawn.I've pushed c9a2ae4, please let me know if the resulting build fixes the issue for you. A jar can be obtained by downloading and unzipping the "dist" artifact when https://github.com/Jikoo/OpenInv/actions/runs/6832530271 completes.
I've pushed c9a2ae4, please let me know if the resulting build fixes the issue for you. A jar can be obtained by downloading and unzipping the "dist" artifact when https://github.com/Jikoo/OpenInv/actions/runs/6832530271 completes.
It looks like this issue still exists
After testing, this issue will also change these NBTs of players:
bukkit.lastPlayed (Also happens with Spigot)
Paper.LastLogin (reset to 0)
Paper.LastSeen
Although it won't cause obvious problems, it will mess up some statistics plugins.
After testing, this issue will also change these NBTs of players:
bukkit.lastPlayed (Also happens with Spigot) Paper.LastLogin (reset to 0) Paper.LastSeen
Although it won't cause obvious problems, it will mess up some statistics plugins.
That data is read and written already.
Read is here:
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/nms-patches/net/minecraft/server/level/EntityPlayer.patch#142
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java#1595-1614
Write is here:
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java#1616-1631
If you have an instance of this data being lost, please let me know exact server and OpenInv versions so I can replicate.
After testing, this issue will also change these NBTs of players:
bukkit.lastPlayed (Also happens with Spigot) Paper.LastLogin (reset to 0) Paper.LastSeen
Although it won't cause obvious problems, it will mess up some statistics plugins.
That data is read and written already.
Read is here:
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/nms-patches/net/minecraft/server/level/EntityPlayer.patch#142
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java#1595-1614
Write is here:https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java#1616-1631
If you have an instance of this data being lost, please let me know exact server and OpenInv versions so I can replicate.
I'm using Paper1.20.2 and OpenInv4.4.0
The data loss may be related to the way they are saved, they save the current timestamp.
Maybe you need to record the original timestamp when reading the data and re-overwrite it after saving.
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java#1629
https://github.com/PaperMC/Paper/blob/4675152f4908431e0f944a7bf9fa3b2181a2dfd6/patches/server/0290-Add-APIs-to-replace-OfflinePlayer-getLastPlayed.patch#L159-L160
In addition, I also tried this file you provided https://github.com/Jikoo/OpenInv/actions/runs/6832530271, the issue of the player world being reset to the overworld still exists
The data loss may be related to the way they are saved, they save the current timestamp. Maybe you need to record the original timestamp when reading the data and re-overwrite it after saving.
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java#1629 https://github.com/PaperMC/Paper/blob/4675152f4908431e0f944a7bf9fa3b2181a2dfd6/patches/server/0290-Add-APIs-to-replace-OfflinePlayer-getLastPlayed.patch#L159-L160
In addition, I also tried this file you provided https://github.com/Jikoo/OpenInv/actions/runs/6832530271, the issue of the player world being reset to the overworld still exists
That would only result in the last played timestamp being updated to the current time, not a reset to 0. You're right that adding it to the special handling with vehicles would be good. Either way, thanks, got a starting point now.
And found time to look into why my fix wasn't working - turns out that of course it wouldn't, because Paper moved the Spigot world parsing into the PlayerList in the same commit. Basically just have to do a full world parse ourselves if it's null, which is exactly what I was trying to avoid for pretty much the same reason Paper is trying to move the parsing to a more maintainable location.
Pushed f9c7ed7 which will resolve this. Available here, including update removal for session timestamps: https://github.com/Jikoo/OpenInv/actions/runs/6839202210
Will probably backport as necessary and merge tomorrow. May end up dropping the commit for undoing session updates to deal with separately because I want to verify that the potential issue with not re-mounting vehicles exists in previous versions.