Weird bug (with myworlds?) when player joins while invedit menu is open
bergerkiller opened this issue · 9 comments
When editing an inventory of a player and that player joins, any further changes to that inventory dont get updated with the actual player. It also seems to write corrupted (?) data to the main world or something weird.
First, I have this dev build of MyWorlds installed. It fixes an issue of main world corruption because, I assume, the plugin reads the vanilla main world player .dat file:
https://ci.mg-dev.eu/job/MyWorlds/310/
Steps to reproduce:
- Player is on a world different than the main world ("world") with a different myworlds inventory (unsure if relevant)
- Edit player inventory with /openinv bergerkiller
- Let that player join
- Add an item to that players inventory and close the menu
- Item does not get added to the player that joined
This would be fixed by switching to 'live mode' when the player logs on (not sure how that works internally but I assume theres different modes of operation)
====
Similar situation but in reverse:
- Player is logged in
- I run /openinv bergerkiller from an alt to edit the logged-in players inventory
- That player logs off
- I make more changes and close the menu to 'save'
- Now MyWorlds loses main world inventory, because I guess the plugin did not load the inventory data prior, so myworlds had no chance to add a recovery portion for the main world
This would be fixed if the plugin did a profile load (from file) before applying the changes, instead of blindly overwriting with data loaded from the online player before. But dont know how it works in openinv.
Let me know how I can help to test further. Kind of risky that if youre editing an inventory and that player joins or leaves, you risk corruptions.
Assuming you're talking about using 5.1.1 on 1.21.1?
When a player logs on, OI checks for active views of their inventory. Assuming there is one, it gets updated to use the online player:
The new owner (the player who has just logged on) has their inventory contents set to the internal inventory's content via Inventory#replaceWith(Inventory), which copies inventory, armor, and off-hand.
All slots of the inventory that is opened are redirected to edit the corresponding part of the owner's inventory, i.e. the player from whom the cursor is fetched is updated to the new player here and so on for the other slots so that the syncher is querying the correct slot on the correct loaded player.
OI does not save during this. All slots are just directed to a new player. OI only saves when an inventory is evicted from its cache and the player who is being opened is not online.
OpenInv/plugin/src/main/java/com/lishid/openinv/util/InventoryManager.java
Lines 260 to 262 in f121c11
When going offline, OI just keeps the old player reference around. All edits go directly into it, same as a player OI loads when doing offline access from scratch.
This would be fixed if the plugin did a profile load (from file) before applying the changes, instead of blindly overwriting with data loaded from the online player before. But dont know how it works in openinv.
OI does already load the profile from disk prior to saving:
This was how we addressed the previous problems MyWorld had with its NBT tags not persisting.
Yes, thats the versions.
The loading doesnt work, because it does loading by player name/uuid and not using the player instance object. I kinda suspected this would happen when I saw mojang added that method separate from before. MyWorlds only is informed when loading with a full Player instance is done. So that explains why that isn't working properly at the moment.
I don't think there's a real good way to fix this on my end, as mocking a full player object with only knowing the player name and uuid gets rather ugly. But if there is no other way I can attempt doing it. Is this something you can fix on your end?
Im not sure why the other thing wasn't working for me, though. When I logged back in with 'bergerkiller' I did not see any of the changes occur when I let my alt alter the inventory with the already-open inventory.
Yeah, easiest if I add support for it. There's a few areas where this is annoying as it does stuff like create a new profile for new players that join, but thankfully that's not important for openinv.
I'll add a hook for the loading by player name + uuid, implement that in myworlds and see if that gets me further.
The problem with the method accepting a player is that it writes all the recognized values loaded into the player instance. I would basically have to create a new fake player each time I want to load data from disk. It would technically work, but it feels kinda gross as a solution.
The initial improvements to arbitrary NBT preserving on my end did use that method, so that makes sense in terms of why we were compatible for a while.
https://ci.mg-dev.eu/job/BKCommonLib/1792/
https://ci.mg-dev.eu/job/MyWorlds/311/
This works better now, fixing the issue of data in the main world getting corrupted. One issue is definitely openinv though, so I want to mention it.
When I open the /openinv of a player thats online, I can make changes and they get 'saved' instantly. When that player logs off, then the changes do not get applied. So the menu 'remembers' the changes to be saved. It saves when you close the dialog. Makes sense so far.
But if you've made a bunch of changes and that player rejoins, none of the changes get applied. When you then close the dialog it might save to file, but that won't have any effect as the player is already logged in & has loaded its profile. It'll just get overwritten again during the save when the player logs off.
I see two options:
- OpenInv saves to disk before the player joins the server, maybe by handling PlayerLogin event (does this even work?)
- OpenInv writes its changes to the logged in player inventory on join and switches to 'live' mode thereafter
This bug is not a huge deal but I guess a trolly player could rapidly join and leave to prevent their inventory being editable.
Re: not coming online correctly, looks like #246 is tracking that specifically. I haven't looked into it yet, but I half wonder if this is Paper-specific, because the only explanation for that is the wrong backing player (which is also something I had specifically corrected a problem with during the update to 1.21). Paper had plans to revert to vanilla behavior where the player entity is actually replaced on death/world change/whatnot, so I wonder if that has gone into effect.
Its a hail mary but this did bite me once before: Do you use Player.isOnline() anywhere? Because that method doesnt verify the player instance is online, it does a lookup of that players uuid/name. To check the instance is valid, use Player.isValid(). This is an unfortunate effect of Player extending OfflinePlayer where isOnline() is defined.
edit*
You most definitely do...many of these should be isValid()
Interesting, appreciate the heads up on that.
The "onlining" process uses the Player instance from the PlayerJoinEvent, so unless a plugin is calling fake joins with stale instances I don't think it would be relevant... though now I'm wondering if this is just something incredibly dumb like me deleting some code during one of the many refactors.
/e: a quick search tells me that I am likely a colossal idiot and that is in fact the case.
/e2: Pushed 847f22f, and fortunately my morning appointment got cancelled so I can actually test it and cut a release today instead of having it wait for at least tomorrow afternoon.
/e3: Everything appears to be working correctly and https://github.com/Jikoo/OpenInv/releases/tag/5.1.2 has been tagged, so as soon as Actions finishes building the file and attaching it we should be good.