Inventory Loss when editing inventory in another non-main world about openinv HOT 27 CLOSED

Yeah, unfortunately OI can't load the player via PlayerList without causing breakages, last I was aware. The default load process within PlayerList also doesn't load a complete and ready to use ServerPlayer, it's a bit half-baked. A lot of data gets loaded in other places.

That save method only saves stuff that actually gets loaded into memory. OI copies it, but needs to make sure that the correct stuff is actually loaded and selectively reload other values that are automatically update like the session timestamps from Paper and Spigot. The root problem is that OI isn't loading your data successfully, so there's nothing to save.

If you have more insight into your save and load process, I'm definitely interested in improving compatibility with other plugins having fully-custom NBT in a more abstract fashion.

from openinv.

@Technofied I've pushed f3236f2 as a temporary measure, you should be able to download and unzip the "dist" artifact when the Actions run for it completes. I would also urge you to not use OI and just use commands from MyWorlds if available to edit inventories.

from openinv.

I actually never considered adding inventory editing in myworlds. I could, but I could also just make a PR for OpenInv to add support for it. After all the only real difference is in the loading and saving of data files, everything else is identical.

Would such a PR be accepted or is it too much?

Also, sounds like you got a good system in place for editing online players. No worries there. I just wasn't certain as I'm not familiar with your plugin :)

from openinv.

I'll look more into it later today then, as it might be something specific to myworlds. If it keeps the MyWorlds tag around im sure there's a way to restore the original data regardless by storing something in there if needed.

from openinv.

Your inventory modification plugin will need to provide you with commands for editing its data. OI only works with data stored in the player's save file, and has no idea where or how other plugins store their data.

from openinv.

Your inventory modification plugin will need to provide you with commands for editing its data. OI only works with data stored in the player's save file, and has no idea where or how other plugins store their data.

The plugin MyWorlds does not actually have its own proprietary data storage format. It stores playerdata in the seperate worlds

from openinv.

MyWorlds stores additional tracking data in the same NBT format, in its own MyWorlds namespace NBT tags. Like "Bukkit". Does openinv delete these when saving? It shouldnt touch foreign/unknown keys. Inventory data is saved in the same mojang format, pretty much everything is actually. At the same tag positions. Its backwards-compatible with vanilla Minecraft.

It uses this information to know what world a player was last on, and to load that worlds' inventory if so. There is some detection for vanilla-unmodified-inventories and it's possible this is triggered due to metadata deletion.

The main difference is that with per-world inventories, it uses the playerdata folder in each individual world folder. As such OpenInv probably cannot edit the offline inventory data of players on worlds other than "world", I would assume. But even so it should not corrupt it.

No inventory data is saved in plugins/My_Worlds folder.

from openinv.

Provided your data is capable of being loaded by Minecraft, OI should preserve it after it saves.
Loading done here:

from openinv.

Im 100% sure player.saveWithoutId() will strip it of all tags it did not parse in player.server.getPlayerList().playerIo.load(player);

If openinv does not simply decode nbt, modify nbt, resave nbt. That explains the issue.

There is a method in PlayerList that saves a 'ServerPlayer' to disk. If you call that, it hooks into myworlds to save the data, and this will work too.

Does it use ServerPlayer when modifying inventories when the player is not online? How does the ServerPlayer come to be in that case?

from openinv.

OI does not want to load players into the server list, that causes a bunch of extra problems that would need to be managed, particularly with other plugins, but some with data integrity. The save process is recreated manually.

I get that MyWorlds and BKCommonLib, which I'm assuming is doing the writing, have existed since before PersistentDataContainers, but you should really consider migrating your data into the PDC location.

If you have a root tag name you use for all this data I'm willing to make an exception to my usual policy of "ask your inventory plugin to add offline editing because there are a ridiculous number of per-world-inventory plugins and that's an insane support burden."
That could go in here:

from openinv.

All myworlds data sits in a compound at the "MyWorlds" root https://github.com/bergerhealer/MyWorlds/blob/master/src/main/java/com/bergerkiller/bukkit/mw/MWPlayerDataController.java#L49

from openinv.

Sweet, okay. OI still won't be able to read other worlds, but we can at least keep it from wiping your data.

from openinv.

This problem would also be fixed by (spigot name mappings, sorry) calling server.getPlayerList() -> data field -> save(ServerPlayer) method. This would make myworlds add the data itself. But if you use a custom way of parsing the server player, maybe that will break. MyWorlds also expects a player to be online, so probably a bad idea.

That save method pretty much matches your section here, but with a bonus of synchronization:

         Path playerDataDir = worldNBTStorage.getPlayerDir().toPath(); 
         Path file = Files.createTempFile(playerDataDir, player.getStringUUID() + "-", ".dat"); 
         NbtIo.writeCompressed(playerData, file); 
         Path dataFile = playerDataDir.resolve(player.getStringUUID() + ".dat"); 
         Path backupFile = playerDataDir.resolve(player.getStringUUID() + ".dat_old"); 
         Util.safeReplaceFile(dataFile, file, backupFile); 

PDC indeed would be ideal, but I have to support all the way back to 1.8 as well. Migrating it to be under the pdc root would cause some issue with existing profiles. But I'll keep it in mind.

from openinv.

Really the main challenge is the safeReplaceFile atomic swap thing. Does this run for players that are online too? I've long had weird issues of some other process asynchronously saving player data too, causing "file locked" kind of errors. I'm afraid there isn't really much you can do about it.

I use:
Files.move(fromFile.toPath(), toFile.toPath(), StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);

I would however maybe call it .dat_old_{timestamp} so it cant possibly clash with other processes saving at the same time. Replace timestamp with the nanostime or something. The atomic move thing I use too on my end, and I'd hope this would work together well. It's something that can help a little bit.

In MyWorlds itself I have a lock object to minimize the issues of multiple threads loading and saving. It won't protect against file manipulation though.

I would personally approach this problem not with a list of tags to 'keep', but the inverse. Iterate all NBT tag keys at root, and if they do not match one of the vanilla-known tags, set them in the saved nbt copied from the original nbt tag.

from openinv.

I currently use the same file save that Mojang does in the player list, net.minecraft.Util#safeReplaceFile(Path, Path, Path), which I believe makes 10 attempts each to back up the old file, delete the original old file, and then rename the temp file. Honestly not sure if it does an atomic write. I assumed it does, but it may just be relying on the baked in sync of the player list. Using a timestamp for the old data is a good idea, but I'm not sure I'd want the responsibility of making sure it's cleaned up on the next write... maybe an "oi" suffix instead for consistency.

OI does its loading and saving on the main server thread due to server implementation limits. Creating a ServerPlayer off the main thread trips the async catcher even though the entity isn't ever added to the world (or did, last I was aware - I'll admit that I haven't checked in a hot minute), and OI currently saves when the last player exits an offline player's inventory. The save could definitely be moved async, but I'm not sure that it would make too much of a difference.

An ignore list for tags is a good idea. A little more painful to maintain, but ideally it won't take too much extra work to keep on top of.

I thought I had seen a question about OI handling online players before I had to run out for a bit - OI doesn't save players who are online, it's a hack in the worst sort of way. The view it opens of the player inventory is actually just a wrapper backed by the real player's inventory contents, same internal list in memory. Any change by OI affects the real player immediately, no handing off of clicks or anything. It also piggybacks the inventory update subscription, which is how OI manages "always correct" state. Changes notify everyone with the inventory, even the owner. When the player logs off, their data save saves any changes from OI. That also has the side effect of booting out everyone in the inventory on the current version, but I haven't been able to figure out how to get around that without breaking the viewer list.

I want to rewrite OI to be dependent on NBT editing and not so much a hot mess of live fire, but that depends on me finding the time to rewrite it from the ground up in a more sane and modern fashion. I also want to expose the player loading to other plugins so that inventory separation plugins can take over and tell OI what to do without putting me on the hook for support for a dozen plugins. Then I can make an addon with support for like 3 major plugins and call it a day.

from openinv.

In OI's current state, probably not. It's a bit of a mess internally, and I don't want to open the door to supporting an arbitrary number of inventory plugins - that's a mistake I've made before with protection plugins. It felt draining maintaining a bunch of simple hooks that were pretty stable. I also don't want to gatekeep new devs who are doing their best based on download count or some other arbitrary measure, so I've been denying every request instead.

I really need to get OI in a state where another plugin can tell it how to load data. Then I'd be happy to write addons for major inventory separation plugins, which I do include yours in the list of. The problem is that OI is a lot of old code that I inherited, and while I've definitely changed things up a little, I've tried my best to avoid breaking the API. If I do open the door to plugins, I imagine that the data load and save could take any form whatsoever, which means the data might be fetched from SQL or something. Suddenly the raw nullable Player return is a lot less informative about the dangers of how and where you're asking for the load. My worst case scenario is a loader that fetches data async and then returns to the main thread being called by another plugin on the main thread and potentially locking up the server entirely.

from openinv.

that's fair, my plugins have been in a similar state before where I've had to rewrite everything, like traincarts. I'll keep this in mind.

At the moment I don't really have the time to implement a whole new feature like this in myworlds, but maybe someday I'll add a very basic inventory editing thing with an inventory gui.

from openinv.

@Technofied I've pushed f3236f2 as a temporary measure, you should be able to download and unzip the "dist" artifact when the Actions run for it completes. I would also urge you to not use OI and just use commands from MyWorlds if available to edit inventories.

Sadly does not solve the problem

from openinv.

I'll also put more work into better generic support tonight. If there's something else you're expecting there or the format isn't exactly right after OI copies it over, maybe that's the problem. I had made an initial start, but then realized it would be more efficient if I read the original data, copied it to preserve a couple tags, and then let vanilla just clobber its own data. There are a couple root tags that still need to be deleted that way (it won't write false values as the input tag is supposed to be empty), but it's a lot less messy.

from openinv.

from openinv.

(oh btw the MyWorlds.playerPos/Rot is legacy and can be ignored. I only write those to nbt in case people migrate myworlds to a much older version for some reason)

from openinv.

Oh I understand now. You're calling the standard read methods and this causes MyWorlds to do the reading. It's actually reading the right world the player was on...that's hilarious.

Its calling this callback in myworlds:

    @Override
    public CommonTagCompound onLoad(final Player player) {

I'll just have to make sure to include the MyWorlds global meta tag at all times in the returned data. The only issue is that saving will then overwrite the main world inventory.

Probably would be better to somehow detect that it is openinv that is calling this method and not the server. I can probably figure this out through the fake player that openinv is probably using.

[19:23:54 INFO]: [My_Worlds] [STDOUT] LOAD class com.lishid.openinv.internal.v1_20_R3.OpenPlayer
By the looks of it.

@Technofied When you use /openinv to edit an inventory, which worlds inventory do you expect it to edit? The one of the world the player was last on, or the inventory on the main world (world/playerdata). I can make either work (with extra steps) since I can include a marker in the saved data to tell myworlds it was edited by openinv when loading the data again later.

from openinv.

This together now works flawlessly, but only edits world/playerdata:
https://github.com/Jikoo/OpenInv/actions/runs/7890563024/artifacts/1242354402
https://ci.mg-dev.eu/job/MyWorlds/272/

from openinv.

Oh I understand now. You're calling the standard read methods and this causes MyWorlds to do the reading. It's actually reading the right world the player was on...that's hilarious.

Its calling this callback in myworlds:

    @Override
    public CommonTagCompound onLoad(final Player player) {

I'll just have to make sure to include the MyWorlds global meta tag at all times in the returned data. The only issue is that saving will then overwrite the main world inventory.

Probably would be better to somehow detect that it is openinv that is calling this method and not the server. I can probably figure this out through the fake player that openinv is probably using.

[19:23:54 INFO]: [My_Worlds] [STDOUT] LOAD class com.lishid.openinv.internal.v1_20_R3.OpenPlayer By the looks of it.

@Technofied When you use /openinv to edit an inventory, which worlds inventory do you expect it to edit? The one of the world the player was last on, or the inventory on the main world (world/playerdata). I can make either work (with extra steps) since I can include a marker in the saved data to tell myworlds it was edited by openinv when loading the data again later.

The world the player was last on :)

from openinv.

This together now works flawlessly, but only edits world/playerdata: https://github.com/Jikoo/OpenInv/actions/runs/7890563024/artifacts/1242354402 https://ci.mg-dev.eu/job/MyWorlds/272/

Unfortunately our default world is 'Reveille' not 'world', so I wouldn't be able to confirm. Sorry :(

from openinv.

Ended up swamped last night and didn't get home until very late, whoops.

I've broadened the scope of tags preserved in 7c00373 by selectively wiping out tags from a copy of the previous data before loading. I still have to actually test it, but when I verify the functionality and backport it into the supported versions I believe the OpenInv side of this will be about as good as it can get for now.

Thank you for investing so much time into working through this with me.

from openinv.

Happy to help out. I also prefer the idea of editing the last world's inventory, since a common use case is to see what people (were) doing. And then you usually care about the world the player was on when they left the server.

I'll see if I can get that working, as it requires some extra work on my end.

from openinv.