Solved InventoryClickEvent lags server full of players

Discussion in 'Spigot Plugin Development' started by tubeycat, Jun 30, 2020 at 9:18 PM.

Thread Status:
Not open for further replies.
  1. Hey! I'm a developer on a server that gets like over 100 players every day, and I am trying to make a plugin that prevents illegal enchantments. The plugin I made works perfectly fine without a bunch of players online but when there are a bunch of players online it starts to get hectic and starts to lower the TPS by 2 or 3.

    What I thought of is making the entire event runs when a tool or armor piece is clicked, yes I thought of an if statement but that would not help as it will still be running the event.

    Here is my event listener with the temporary enchantment blocks inside it. (I am going to make a config don't make fun of me)

    Code (Java):
    public class OnInventoryClick implements Listener {
        @EventHandler
        public void InventoryClick (InventoryClickEvent e) {
            Player p = (Player) e.getWhoClicked();
            if (e.getCurrentItem().hasItemMeta()) {
                if(e.getCurrentItem().getItemMeta().hasEnchants()) {
                    if (banenchantsCommand.enabled) {
                        boolean message = false;
                        if(!p.isOp()) {
                            //p.sendMessage(ChatColor.YELLOW + "[TKBanEnchants] " + ChatColor.RED + "You have an illegal enchantment on an item! Removed the enchantment.");
                            ItemStack item = e.getCurrentItem();
                            if (item.getEnchantments().containsKey(Enchantment.FROST_WALKER)) {
                                item.removeEnchantment(Enchantment.FROST_WALKER);
                                message = true;
                            }
                            if (item.getEnchantments().containsKey(Enchantment.LOOT_BONUS_BLOCKS)) {
                                item.removeEnchantment(Enchantment.LOOT_BONUS_BLOCKS);
                                message = true;
                            }
                            if (item.getEnchantments().containsKey(Enchantment.CHANNELING)) {
                                item.removeEnchantment(Enchantment.CHANNELING);
                                message = true;
                            }
                            if (item.getEnchantments().containsKey(Enchantment.RIPTIDE)) {
                                item.removeEnchantment(Enchantment.RIPTIDE);
                                message = true;
                            }

                            if (message) {
                                p.sendMessage(ChatColor.YELLOW + "[TKBanEnchants] " + ChatColor.RED + "You have an illegal enchantment on an item! Removed the enchantment.");
                            }

                        } else {
                            if (message) {
                                p.sendMessage(ChatColor.YELLOW + "[TKBanEnchants] " + ChatColor.GREEN + "You bypassed an enchantment block! (You are a server operator)");
                            }
                        }
                    }
                }
            }
        }
    }
     
    Are there any solutions to this?
     
  2. TheViperShow

    TheViperShow Previously FendiTony777

  3. Thanks! Great feedback! Really helpful!
     
  4. It is feedback ngl
     
  5. I am not 100% if this will be your problem (because we can't see the rest of your code).
    You can try too make a arraylist that contains all tools & armor pieces so it doesn't get fired on other blocks or items.
    If you gonna make a arraylist just add the words in it that are contained in the Material (emum) and getType of your clicked item.
    (This makes its so you don't have to add every material in you arraylist like diamond_sword etc but only sword)

    But you sure you don't got other codes like looping thru all players and do other kinds of checks?
     
  6. Its not a solution where he is asking for...
     
  7. You could try to cache the things you retrieve from the event and ItemStack and see how much of a difference that makes (probably some micro optimization, but nevertheless):
    • The ItemStack returned by e.getCurrentItem() creates and returns a new wrapper object around Minecraft's internal item stack everytime you call it.
    • hasItemMeta internally checks if the item as a non-empty tag and then creates a new ItemMeta object and checks if it actually has any data or is considered 'empty'. Creating the ItemMeta requires retrieving all the data from the internal Minecraft ItemStack and converting it to corresponding Bukkit objects. Once you call getItemMeta, the ItemMeta object has to be created again. Maybe it would suffice to call getItemMeta directly. This would avoid having to construct the ItemMeta (and converting all the item data) twice. On the other hand, there would then be some minor overhead everytime you retrieve the ItemMeta for an item without ItemMeta (because it has to create an empty ItemMeta object then). However this should be negelectable (creating an empty ItemMeta is quite cheap in comparison).

      However, since you only use this ItemMeta object to check if the item has some enchantments, you can probably avoid creating it altogether, check below.
    • Everytime you call itemStack#getEnchantments() it internally converts Minecraft's enchantment data from the item into the corresponding Bukkit equivalent. Consider calling this only once and then reusing the returned enchantments Map. Alternatively, since you only use this to check if the item has a certain enchantment, and you already created the ItemMeta object above already, you could also use ItemMeta#hasEnchantment(enchantment) instead. The ItemMeta object has already done this conversion of the enchantments data.

      However, since you only use the ItemMeta object to check if the item has enchantments, I would probably do it the other way around: Use ItemStack#getEnchantments(), store that inside a variable, and use that to check if it is empty or contains a certain enchantment. That way you only need to do the conversion for the item data related to the enchantments. And this is also quite cheap if the item has no enchantments.
      Or, maybe even better if you only expect to encounter few of these enchantments, use ItemStack#containsEnchantment
    • You could also use ItemMeta#removeEnchantment. However, this would then require you to apply the modified ItemMeta back to the item, which has to do the conversion in the opposite direction then (Bukkit -> Minecraft). This might be preferably if you do multiple modifications to the item. However, if you only expect to remove a single enchantment then your current way of removing (and thereby avoiding this backwards conversion) might be preferred. And in your case you can even avoid creating the ItemMeta object (see above).

    In summary, I would try the following:
    • Store the ItemStack retrieved via e.getCurrentItem() inside a variable and reuse it from there.
    • Either store the Map returned by item.getEnchantments() inside a variable and reuse that to check if the item has certain enchantments. Or use ItemStack#containsEnchantment.
    • Avoid hasItemMeta and getItemMeta. You probably don't require them in your usecase.

    I have no idea how much of a difference this would actually make, but this might be worth trying anyways. Maybe this is already enough to make a difference here.

    Another possibility would be to not do this on every InventoryClickEvent but maybe perform a one-time (long running) task to cleanup your world and player data from any 'illegal' enchantments once.

    The event will run no matter what. All you can influence is how much work you do inside your specific event handler.
     
    #7 blablubbabc, Jun 30, 2020 at 10:46 PM
    Last edited: Jun 30, 2020 at 10:54 PM
  8. Post a timings report.
     

  9. This helped chip off some stress off of the server, thanks! I ran a timings report and it seems like it is working completely fine now!
     
Thread Status:
Not open for further replies.