EbonJaeger / perworldinventory-kt

Multi-world inventory plugin for Spigot written in Kotlin.
MIT License
46 stars 35 forks source link

MobArena Compatibility #44

Open SoraShiunin opened 6 years ago

SoraShiunin commented 6 years ago

Hello. With mobarena inventory saving, Mob arena or sometimes this plugin overwrites the players inventory, kinda random. As of the latest update with the rewrite it seems that this plugin always takes over.

Could compatibility be made with the following plugin: https://www.spigotmc.org/resources/mobarena.34110/

In their anniversary updates it is said that gamemode forcing inventories causes this issue and I can confirm that by disabling it makes it work fine however this later causes obvious issues with creative worlds.

EbonJaeger commented 6 years ago

There was an issue in his repo about this that I think had a resolution... I'll see if I can find it again.

EDIT: https://github.com/garbagemule/MobArena/issues/423#issuecomment-385433920

SoraShiunin commented 6 years ago

Will it eventually be fixed tho?

garbagemule commented 6 years ago

@Gnat008 I think there might still be something about this issue. I haven't tested with the rewrite of PWI (mad respect for the Kotlin direction - love it!), but if it works like it did previously, it looked like the issue was with cached vs. non-cached inventories, where non-cached inventories would get loaded a tick later. Could this perhaps be solved by adding an option to ignore the potential TPS impact of forcing the loading of non-cached inventories to happen on the same tick as the teleport event?

EbonJaeger commented 6 years ago

@garbagemule The one-tick later nonsense doesn't happen anymore. You can see what happens in the listener class here: https://github.com/Gnat008/perworldinventory-kt/blob/master/src/main/kotlin/me/ebonjaeger/perworldinventory/listener/player/PlayerChangedWorldListener.kt

If you want you can double check the logic there and point out anything that jumps out at you. I am open to suggestions!

garbagemule commented 6 years ago

@Gnat008 it looks like this is where the problem is rooted: https://github.com/Gnat008/perworldinventory-kt/blob/1a70323dd850ba92abc345208df30be84fb3cb9d/src/main/kotlin/me/ebonjaeger/perworldinventory/data/ProfileManager.kt#L91

Correct me if I'm wrong, but doesn't this mean that if a given inventory has not yet been loaded into the cache, the async file loading followed by the next-tick task scheduling would result in exactly the kind of behavior that we're trying to avoid?

I honestly think that a per-world sync vs. async setting in PWI could solve this with the caveat that some lag is to be expected. Or perhaps a setting that forces a full load of on-disk inventories at world load time, and then perhaps an async cache write-through on switching, i.e. let the cache be the source of truth rather than the file system, and let the file system "subscribe" to changes to the cache.

If the problem must be addressed in MobArena, it would require a dependency on PWI and an InventoryLoadCompleteEvent or something like that, and then MobArena's join/leave process would need to be rewritten in an async-friendly way.

EbonJaeger commented 6 years ago

@garbagemule Do you feel that it would be okay to do file reads on the main thread? Most people generally avoid doing that.

garbagemule commented 6 years ago

Performance optimization without profiling is just fumbling in the dark, pulling hard on whatever you grab, and hoping it's a light switch instead of a tiger's tail.

File I/O on the main thread is not necessarily as bad for performance as a lot of people make it sound. It all depends on the context and in how tight of a loop the operation exists. With SSDs and small files, the round trip doesn't take as long as it used to. And considering this literally only happens once per player per world/gamemode, I seriously doubt that it will be noticeable.

If it does get hairy, there are a few things that you could do to prefetch inventories. For instance, why not simply hold all player inventories of all online players in the cache at all times, and then evict inventories of players who have been offline more than 10 minutes or so? These are fairly small chunks of memory compared to the ones used for actual world chunks and entities.

Do you have a way to maybe stress test with 100+ dummy players while profiling? That would give you a good idea of what kind of performance impact you're looking at.

EbonJaeger commented 6 years ago

@garbagemule Are you able to build PWI and see if it works as expected? I don't know if you have a setup with both plugins or not.

garbagemule commented 6 years ago

Excellent work @Gnat008!

Perhaps @SoraShiunin should do the verification since he's experiencing the issue? @SoraShiunin, if we give you a link to a build, can you verify that it works as expected?

garbagemule commented 6 years ago

I just tried to reproduce the issue. I have a survival world with an arena in it and a barebones creative world. I use default PWI settings except I have disabled the per-gamemode inventories disabled, but I don't think it changes the outcome.

Steps to reproduce:

  1. With the server running, teleport to the creative world and log out
  2. Restart the server (full stop and then start back up) to clear the PWI cache
  3. Join MobArena *
  4. Pick a class
  5. Leave MobArena
  6. Teleport back to the survival world **

Right, so with the previous version of PWI (1.x), the survival inventory is restored into the now-wiped inventory after joining the arena (*). This is a good indication that caching is the problem. Upon leaving MobArena, I end up in the creative world, and when I teleport back to the survival world (**), my survival inventory is gone.

With the new version of PWI, repeating the steps above, my inventory is empty when I join the arena (*), and it appears untouched when I teleport back to the survival world (**). In other words, I'm confident that the new version of PWI fixes the issue! 🎉

I still think that @SoraShiunin should verify that it works on his setup. My test is contrived, so it'd be nice with confirmation from a real setup.

SoraShiunin commented 6 years ago

With 'per-gamemode inventories: enabled' and a setup I can confirm it -kinda- works for me as well. It teleports me back to the main world for a second after typing '/ma leave' and thus the items are added to that inventory.

However some players still lost inventory sometimes so I investigated. It seems that a combination of teleports between un-linked and linked worlds with different gamemodes causes the gamemode inventories to switch after a round of mobarena inbetween . I tried it myself and my creative inventory in the un-linked world (that is not creative,it is a seperate arena world) overrid my survival inventory in that un-linked world and my survival inventory for that world just vanished however anything won in the mobarena is still correctly in the main worlds inventory strangely enough.

If I find how exactly to reproduce this, I will comeback to you, I tried for 20 minutes, unable to reproduce.

My theory is that it is changing inventories for older players that had inventories in the older version of PWI and thus why it doesn't work for me anymore since all inventories have been "repaired".

EbonJaeger commented 6 years ago

@SoraShiunin Your theory of old data is likely correct; if players have been in that group already, they will have data for it already.