PerWorldInventory icon indicating copy to clipboard operation
PerWorldInventory copied to clipboard

Incompatibility with other plugins due to 1-tick delay on gamemode changes

Open blablubbabc opened this issue 7 years ago • 2 comments

Assume some minigame plugin does the following in this order when players enter/leave the minigame:

Player entering the minigame:

  • Save location.
  • Teleport player to minigame lobby (potentially in different world).
  • Save gamemode.
  • Set gamemode to survival.
  • Save potion effects, inventory, and any other player attributes.
  • Clear player inventory and reset attributes.

Player leaving the minigame:

  • Clear player.
  • Restore inventory, potion effects and attributes.
  • Restore gamemode.
  • Teleport player to previous position.

Note: The order has been chosen this way to be (hopefully) most likely compatible with other plugins reacting to world and gamemode changes, while not having to introduce (and handle) additional delays in there: It first teleports, then changes the gamemode, and then applies its store and clear inventory logic. So any plugins reacting to world or gamemode changes are hopefully done with 'their thing' before the minigame plugin does its inventory saving/clearing. And on leaving the minigame, the order is exactly reversed.

However, PerWorldInventory has a 1 tick delay between saving the inventory and then setting the new inventory when handling gamemode changes: https://github.com/Gnat008/PerWorldInventory/blob/master/src/main/java/me/gnat008/perworldinventory/listeners/player/PlayerGameModeChangeListener.java#L63

I can see the following potential issues with this:

  • On entering the minigame: PerWorldInventories stores the correct inventory for the previous gamemode of the player. But the above minigame plugin stores and clears the inventory of the previous gamemode as well. Then PerWorldInventories applies the new gamemode's inventory 1 tick later. The player ends up with the wrong inventory in the minigame.
  • On leaving the minigame: The above plugin restores the inventory of the player's previous gamemode (wrong), then PerWorldInventories stores that as the inventory for the current (minigame specific) gamemode (wrong). And 1 tick later it restores the previous inventory (correct).
  • However, in case there are per-world-inventories active (possible implemented by other plugins even): On leaving the minigame: PerWorldInventories/Or some other per-world-inventory-plugin applies the new-world inventory, before PerWorldInventory restores the inventory due the previous gamemode change.

In case PerWorldInventories has to load the player data from database first, there might by an additional delay before the inventory gets set, resulting in similar issues, but with arbitrary delay (instead of 1 tick).

An other issue I see in regards to mingame plugins: If the minigame involves dynamic gamemode changes, PerWorldInventories will store and restore inventories while the player participates in the minigame..

And here is an other 1-tick delay which is possibly not needed when changing the gamemode after a world change: https://github.com/Gnat008/PerWorldInventory/blob/5f1ddd7bafe9dcd69d044a40589692a74b07e071/src/main/java/me/gnat008/perworldinventory/process/InventoryChangeProcess.java#L144

So my questions/ideas on how to improve potential compatibility issues with minigame plugins:

  • Is this 1 tick delay inside the gamemode-change handling really required?
  • Is the 1 tick delay before changing the gamemode on world changes requried? (the player has already swapped the world there when the event is being called by craftbukkit)
  • Haven't checked that yet myself: Are you pre-loading player data (to prevent delays due to database access)?
  • Would it be possible to add config options to disable the per-gamemode inventories if the player is in a specific world(-group)? So the minigame could be set up to happen in certain world(s) only, handle storing/restoring the inventories on its own there and PerWorldInventories wouldn't interfere due to reacting to gamemode changes. Maybe the 'SHARE_IF_UNCONFIGURED' setting could be used to affect gamemode-changes as well? (or a new setting; or a way to disable the gamemode inventories for specific worlds, but still be able to use per-world inventories for those worlds..)
  • Would it be possible to have per-world-gamemodes enabled, but exclude certain worlds? So that for example when entering a world with unconfigured default-gamemode the gamemode will not be changed.

Edit: Just found another potential issue fitting into this as well: There seems to be a repeating task which saves and removes cached player inventories, potentially adding a delay due to async inventory load at a later point. https://github.com/Gnat008/PerWorldInventory/blob/master/src/main/java/me/gnat008/perworldinventory/data/players/PWIPlayerManager.java#L368 It might make sense to keep the data of all online players cached until they leave. Because any delay related to inventory loading can result in compatibility issues as pointed out above.

blablubbabc avatar Nov 10 '17 14:11 blablubbabc

@Gnat008 i suffer from inventory lost whrn players use Paintball. Before we didnt have any problems for years. Could you fix this pls?

ScuroK avatar Jan 03 '18 14:01 ScuroK

This is being address in the rewrite.

EbonJaeger avatar Jan 05 '18 20:01 EbonJaeger