1.8.8 InventoryCloseEvent Issue

Discussion in 'Spigot Plugin Development' started by Religion, Jan 14, 2020.

  1. Code (Text):
        @EventHandler(priority = EventPriority.HIGHEST)
        public void inventoryClose(InventoryCloseEvent event) {
            if (event.getInventory().getTitle().equals(Chat.color(main.getConfig().getString("options.captcha_gui_title")))) {
                System.out.println("true3");
                Player player = (Player) event.getPlayer();
                if (close.contains(player)) {
                    close.remove(player);
                } else {
                    player.openInventory(event.getInventory());
                }
            }
        }
    Upon closing the inventory: https://paste.landon.pw/ojepixex.css. 651 "true3", why is the closing calling it 600+ times?
     
  2. I'm aware there's more to the error, but I assume it has to do with the event being called 600+ times when the inventory gets closed, which is what I want to resolve first.
     
  3. You're likely causing a StackOverflowException to occur. This line is pretty dangerous all things considered:
    Code (Java):
    player.openInventory(event.getInventory());
    That tries to reopen the same inventory, but since the client is already trying to close that inventory, it just instantly closes and calls another InventoryCloseEvent, thus looping forever until it throws an exception. If you place the call to open the inventory into a 1-tick delayed runnable it will work how you want it to.
     
    • Winner Winner x 1
  4. Ahhh, makes sense now. Thank you, appreciate the help!
     
  5. As he said, you can resolve through the next server tick using
    Code (Text):
    new BukkitRunnable().runTask(MainClass);
    Like this:
    Code (Java):
    @EventHandler(priority = EventPriority.HIGHEST)
        public void inventoryClose(InventoryCloseEvent event) {
            if (event.getInventory().getTitle().equals(Chat.color(main.getConfig().getString("options.captcha_gui_title")))) {
                Player player = (Player) event.getPlayer();
                if (close.contains(player)) {
                    close.remove(player);
                } else {
                    new BukkitRunnable() {
                        @Override
                        public void run() {
                            player.openInventory(event.getInventory());
                        }
                    }.runTask(MainClass.plugin);
                }
            }
        }

    [REMEMBER]
    Code (Java):
    .runTask()
    this method is used to execute something or make an action after the next tick.
     
    • Agree Agree x 1
    • Creative Creative x 1
  6. I recommend to store the player UUID instead the Player.
     
    • Agree Agree x 1
    • Informative Informative x 1
  7. It's a temporary store for a captcha system (Click correct item, add, close, remove). No urgent data is stored alongside it, but yeah, for data that needs to be maintained account-specific, UUID is a must.
     
    • Like Like x 1
  8. The Player object itself is pretty big, storing a UUID will use less memory.
     
    • Agree Agree x 1
  9. Gnrrrf please know what putting something to a Map does, it stores the pointer to an object, not the object itself!
    The pointer is a 32-bit signed Integer in most cases (or long or whatever java decides is best), meaning it is really small!
     
  10. This is true, people assume storing big objects in maps = more memory but not true.
    People might even think that using a UUID is faster because the hashCode function for a UUID is much faster, not even true as CraftPlayer implements hashCode and just uses the UUID hashCode..

    The main reason people just use uuid over player because in some cases you might not have a player and it can just be easier and cleaner in a lot more scenarios. (For example if they are offline)

    tl;dr use uuid, but don't be misinformed if you think its for memory reasons.

    EDIT: Just had a thought, technically there are cases where Player uses larger memory, for example if you have a Map<Player, Integer> and a player logs off then nowhere in the code references the Player anymore and you (theoretically) are the only one referencing it meaning you are preventing it from being garbage collected.

    This a pretty niche problem, but theres a very easy fix which is to use a LazyHashMap which basically means if you are the only one in the code referencing it, for example if a player logs off, it will remove that entry automatically, thus making you able to improve on memory usage!

    EDIT2: Woops yes I did mean WeakHashMap
     
    #10 Oscazz, Jan 14, 2020
    Last edited: Jan 15, 2020
    • Agree Agree x 1
  11. Did you mean WeakHashMap?
     
  12. That is indeed the case (and, in fact, not even a niche problem. A lot of people might have something like Map<Player,Home> in their code to implement /home and such commands) and therefore you should in general not implement a Map containing a player if you can expect the player to log off while the Map contains the player.
    In these cases, you should indeed either use a WeakHashMap or unregister the Player in a PlayerQuitEvent
    Something that is also to be noted is that if the player logs in again, the Map will contain two instances of the same player (assuming you did not properly unregister the player) because the player-id is not the same (the UUID is the same, but entity also defines an ID that will be distinct from that of the player).

    In general, I believe that there are a lot of misconceptions about Maps, especially here...
     
  13. Thanks for all the information, really appreciate it. I’m just storing player on an inventory click event, which then closes the GUI for the player (and upon closing the GUI, the player is removed). No long term persistence over logging out or anything, so I really don’t think it will be an issue in this use case. But really helpful information for other projects :)