OpenInv

4M Downloads

Opening an offline player's inventory will reset the player's world when using Paper

KioProject123 opened this issue · 10 comments

commented

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

commented

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.

commented

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,

if (entity.level() == null) {
// Paper: Move player to spawn
entity.spawnIn(null);
}
always resets them to spawn.

commented

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.

commented

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

commented

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.

commented

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.

commented

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

commented

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.

commented

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.

commented

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.