Bug in InventoryClickEvent of ArmorListener

[Orscrider]

The if statement in line 50 of ArmorListener will always be true if it's not a crafting inventory: if((e.getSlotType() != SlotType.ARMOR || e.getSlotType() != SlotType.QUICKBAR) && !e.getInventory().getName().equalsIgnoreCase("container.crafting"))

The first half always returns true because armor can't be quickbar and vice versa.

[Arnuh]

I don't think you quite understand the intended purpose of that check.. The inventory name check doesn't even need to exist as it is most likely something I forgot to remove in early testing.

Don't fully understand what you are trying to point out as your post is a tad bit confusing to me. But I would like the say the only way to equip a piece of armor via InventoryClickEvent is via dropping it in the armor tab or from right clicking it in the quick bar, as I said before the last check is probably a fix for a issue I encountered in early testing and never removed.

[Orscrider]

(e.getSlotType() != SlotType.ARMOR || e.getSlotType() != SlotType.QUICKBAR) You check if the slot type is not armor or not quickbar and only if both statements are false the whole bracket will return false. Obviously one will return true and therefore the whole bracket will be true.

[Arnuh]

Please tell me where this error occurs. As from testing this doesn't cause any issues.

[Orscrider]

It doesn't fire the ArmorEquipEvent since it just returns. Tested with Spigot 1.10.2.

[Arnuh]

What are you doing that it doesn't fire

[Orscrider]

if((e.getSlotType() != SlotType.ARMOR || e.getSlotType() != SlotType.QUICKBAR) && !e.getInventory().getType().equals(InventoryType.CRAFTING)) return; Let's dissect this line: If the whole if statement is true, it returns. For that to happen it has to be not a crafting inventory and the slot type needs to be not armor OR not quickbar.

Case 1: Slot type is armor (we try to put on armor), not in a crafting inventory e.getSlotType() != SlotType.ARMOR is false e.getSlotType() != SlotType.QUICKBAR is true But it checks if one of these two return true: (e.getSlotType() != SlotType.ARMOR || e.getSlotType() != SlotType.QUICKBAR) therefore returns true !e.getInventory().getType().equals(InventoryType.CRAFTING) also returns true since we are not in a crafting inventory if((e.getSlotType() != SlotType.ARMOR || e.getSlotType() != SlotType.QUICKBAR) && !e.getInventory().getType().equals(InventoryType.CRAFTING)) Altogether the if statement is true and returns.

Case 2: Slot type is quickbar (we try to put it on from the quickbar), not in a crafting inventory e.getSlotType() != SlotType.ARMOR this time is true e.getSlotType() != SlotType.QUICKBAR is false now and so on... Since you check whether the slot type is not armor OR not quickbar it always returns true because if one is false the other one has to be true (it can't be armor and quickbar at the same time).

[Arnuh]

I don't care what the line does, I want to know what specific armor equipping/dequipping action doesn't get called because of that line. Or what action is calling an ArmorEquipEvent when it shouldn't be called. Because my tests result in none.

[Orscrider]

Since it just returns it doesn't go past this line and the ArmorEquipEvent isn't fired for the InventoryClickEvent.

[Arnuh]

But as I said.. All ArmorEquipEvents I did in my test work. You need to give me a specific one which doesn't due to this error.

[Orscrider]

Any ArmorEquipEvent called by the InventoryClickEvent won't fire.

[Arnuh]

False. if you did testing other than just throwing code at me you would know this.

[Orscrider]

Read my explanation. It cannot work.

[Arnuh]

Do some testing, it works.

[Orscrider]

I changed the line to the one before commit 'Added EquipMethod.HOTBAR_SWAP thanks to KingPsychopath on spigot for' and it works. Since that point it can't work anymore.

[Orscrider]

The last two comments on Spigot also state that it doesn't work.

[Arnuh]

hrm.. Haven't been very active on spigot due to the password reset system failing to work.. I'll do some further testing, gimme a few.

[Arnuh]

Ok.. after some testing. e.getInventory() returns the current inventory opened.. I don't know how spigot knows which inventory to say with players inventory, but it assumes the inventory is CRAFTING(due to the 5 crafting slots in top right being that)

but.. if you do e.getClickedInventory() the armor slots, secondary slot, and players inventory is all PLAYER. While the 5 slots are the only thing crafting.

So somewhere in spigots code it sets crafting as the full player inventory when doing e.getInventory in InventoryClickEvent even though only 5 slots are actually that.

With all my testing though. ArmorEquipEvent gets called when it should, I've gotten multiple reports somethings it doesn't call 100% and I'm 99% sure its an issue not on my part.

I am 80 versions behind.. so I guess they could of changed it, be hella dumb if they did so far in development

[Orscrider]

This issue doesn't really have anything to do with this though: !e.getInventory().getType().equals(InventoryType.CRAFTING) It's that part: (e.getSlotType() != SlotType.ARMOR || e.getSlotType() != SlotType.QUICKBAR) It always returns true. What's the point of || here?

[Orscrider]

I just realized that the code from before the commit doesn't make sense either (or am I misunderstanding something here?) if(e.getSlotType() != SlotType.ARMOR && e.getSlotType() != SlotType.QUICKBAR && !e.getInventory().getName().equalsIgnoreCase("container.crafting")){ return; } Here you only return if the slot type is not armor and not quickbar and if it's not a crafting inventory. But don't you want to return when the it's not a crafting inventory because it's not relevant then?

[Orscrider]

This version works: if (e.getSlotType() != SlotType.ARMOR && e.getSlotType() != SlotType.QUICKBAR) return; First we check if the slot type is not armor or not quickbar -> irrelevant

if (e.getClickedInventory() != null && e.getClickedInventory().getType().equals(InventoryType.CRAFTING)) return; And we can check if the clicked inventory is a crafting inventory -> irrelevant

[Arnuh]

Interesting.. I still don't get the issue though. I'll implement your above code into the next update..

Then I need to fix dispensers, which requires me to modify spigot to do such thing.. Soo I don't know when I'll release that fix

Also don't have access to my spigot account.. gonna be an interesting journey

[Orscrider]

The issue is that it returns at the said line if it's not a crafting inventory. Alright, dispensers work for me though?

[Orscrider]

That fixes it: if (e.getSlotType() != SlotType.ARMOR && e.getSlotType() != SlotType.QUICKBAR && e.getSlotType() != SlotType.CONTAINER) return; slot type not armor, quickbar or container? -> irrelevant if (e.getClickedInventory() != null && !e.getClickedInventory().getType().equals(InventoryType.PLAYER)) return; clicked inventory is not the player inventory? -> irrelevant if (!e.getInventory().getType().equals(InventoryType.CRAFTING) && !e.getInventory().getType().equals(InventoryType.PLAYER)) return; not the survival (crafting) or creative (player) inventory? -> irrelevant Workbench inventory is named workbench.

[Arnuh]

Why would you check for a container..

[Orscrider]

You can shift click from the container and equip armor. The slot type container is the upper slots of the player inventory.

[Arnuh]

K I did a commit.. won't be uploading a new version to spigot till dispensers are fixed

[Orscrider]

What's wrong with dispensers?

[Arnuh]

@Orscrider decided to get your opinion before I do a potential PR for spigot.

The current radius check is horribly off in some cases.. and is just a mess in general. I took the code from dispensers in minecraft.. but I have been unable to port it without using nms imports due to a lack of an entity bounding box api.

Current code: https://gist.github.com/Borlea/f92cf9da25a2a90cb5f60a3f3d3c601c

Line 8 is the main part.. Spigot doesn't have a sufficient api to do such task.. even without the predicates(getNearbyEntities is off for what we need)

[Orscrider]

I wouldn't use nms code if there are other options available. That's my suggestion that I tested as well: https://gist.github.com/Orscrider/bf836cc177e6d0075cc6c0657a7e2690 Of course it's not really a great solution either since the armor will be equipped first and the event will be called afterwards but I can't think of a better solution.

Is there are better way talking this over than here in the already resolved issue btw?

[Arnuh]

@Orscrider #3