Jikoo / OpenInv

Open anyone's inventory as a chest, real-time!
GNU General Public License v3.0
120 stars 41 forks source link

Update to 1.19.4 #130

Closed Jikoo closed 1 year ago

Jikoo commented 1 year ago

I don't have time to test this this morning. If anyone is really chomping at the bit to update their server, dev builds can be found in completed CI runs. Select the run you want, scroll down to the bottom, download and unzip dist. Otherwise, expect a release this afternoon.

molor commented 1 year ago

Hello! In case this issue is caused by OI, I'll mention it here too. https://github.com/EngineHub/WorldGuard/issues/1987

Additional information:

Jikoo commented 1 year ago

Last line in server logs is [com.lishid.openinv.internal.v1_19_R3.OpenPlayer] Failed to save player data for Player: Saving entity NBT

This is super important. It looks either loading or saving changed a little. I'm assuming this means reading the player's NBT no longer sets the player's current position, though I'll need to dive into it some more.

Jikoo commented 1 year ago

Okay, no, nothing has changed here.

if (this instanceof ServerPlayer) {
  Server server = Bukkit.getServer();
  World bworld = null;
  String worldName = nbttagcompound.getString("world");
  if (nbttagcompound.contains("WorldUUIDMost") && nbttagcompound.contains("WorldUUIDLeast")) {
    UUID uid = new UUID(nbttagcompound.getLong("WorldUUIDMost"), nbttagcompound.getLong("WorldUUIDLeast"));
    bworld = server.getWorld(uid);
  } else {
    bworld = server.getWorld(worldName);
  }

  if (bworld == null) {
    bworld = ((CraftServer)server).getServer().getLevel(Level.OVERWORLD).getWorld();
  }

  ((ServerPlayer)this).setLevel(bworld == null ? null : ((CraftWorld)bworld).getHandle());
}

The only way to get this to occur is if you have no overworld worlds and the world the player was already in does not exist. There's... really not a lot I can do about this. Either OI can attempt to recover, which will result in players moving to the default world after being opened, or OI can report those players nonexistent until they log in and the server decides what it wants to do with them.

molor commented 1 year ago

I can reproduce this and send you stacktrace if needed and you tell me how to force OI to display it — maybe this can make the issue more clear

Jikoo commented 1 year ago

A stack trace unfortunately will likely only point to the same issue, namely that Entity.level is null. If you do not have a funky world setup as described, then Paper is somehow messing with saving and loading. In previous versions they just tacked on Paper tags to Bukkit's tags and loaded them at the same time.

molor commented 1 year ago

The only change in my current world list is an plugin-created NORMAL/NORMAL (environment/type) world minecraft:future. Other vanilla worlds (minecraft:overworld etc.) isn't touched. But future sometimes gets removed (entire world folder) to generate a new one, so its UUID sometimes changes (and the player could be saved as in the "old" world) — if that's important then now you know :D

Okay, I'll be waiting for Paper team to update/fix/reply.

Jikoo commented 1 year ago

But future sometimes gets removed (entire world folder) to generate a new one, so its UUID sometimes changes (and the player could be saved as in the "old" world) — if that's important then now you know :D

Depending on how the world is then regenerated, yeah, if it ends up with a new UUID that would be half of this issue. The other half is that somehow the default world also is missing. Either Paper messes with the load and save code or something else is very weird here.

I started thinking it might be an issue with how CraftPlayer#loadData works (because we do already work around shortcomings with the load/save process, namely #16), but that also appears unchanged.

I will rework OI here to spit out a warning when the world isn't loaded correctly (and address some potential issues if injecting our own CraftPlayer implementation with modified saving doesn't work), but it won't fix the underlying issue.

I'll pop over to Paper and post my findings I guess.

Jikoo commented 1 year ago

I was going to throw in a guard, but I don't actually understand how it's possible to hit this situation - OI demands that the default world exists before it even constructs a ServerPlayer. I guess it can be done anyway, but this is weird.

molor commented 1 year ago

Now with latest Paper & OI only this appears in console if I use /oi with random player name (idk how the search works but it's always find some1 old XD)

[com.lishid.openinv.internal.v1_19_R3.PlayerDataManager] Encountered null world while loading kaneuss
molor commented 1 year ago

(can you also please change [com.lishid.openinv.internal.v1_19_R3.PlayerDataManager] to [OpenInv] [PlayerDataManager] or even better to [OpenInv]?)

Jikoo commented 1 year ago

idk how the search works

Search uses a Jaro-Winkler string metric ignoring capitalization. Best match is used for whatever you provide as an argument. It used to use a Levenshtein distance which was available through some ancient bundled library, but that often resulted in wild inaccuracies for short names.

(can you also please change [com.lishid.openinv.internal.v1_19_R3.PlayerDataManager] to [OpenInv] [PlayerDataManager] or even better to [OpenInv]?)

I know this doesn't line up with how Bukkit plugins usually do logging, but I generally try to use vanilla as a guide for working with server internals. I am way more concerned about the fact that you're running into the issue than how the logging compares to what you're used to seeing.

alexchandel commented 1 year ago

Is it really too much to support all of 1.19, that is, all of the current major version? Does the API really change that much? It makes it hard to update from say 1.19.2 to 1.19.4 when plugins don't support both versions.

Jikoo commented 1 year ago

Yes. Each NMS revision is additional maintenance. Backporting fixes is a boring, menial chore, and the more places I have to do it the less motivated I am to bother at all. See for example https://github.com/Jikoo/OpenInv/commit/454467c20ee00fbba0b51959a3bb93f80b87a51a - this is not worth my time. If you can't upgrade your server in a reasonable amount of time, you can always compile your own bridge or just follow standard safe procedures and create a copy of your live server with updated plugins, then apply the plugins and server to the live server when you have confirmed it to be functional.

Also, pretty sure 1.19.2 had the same NMS revision as 1.19.3, so either you already were locked into a very specific version of OI by Spigot not bumping the version or you already could be using 4.3.0 and have the bridge functionality you're asking for.