-
-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid using Inventory.getHolder() in hopper check #2086
Conversation
The real change here is to use event.getInventory().getType(), rather than instanceof event.getInventory().getHolder(). The latter will return snapshots, and will therefore be especially slow when items have lots of metadata. Consider a shulkerbox full of written books, or enchanted equipment.
The owner of a protected item must still be logged in. Feel free to omit these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. If Robo is particularly concerned about the holder being a real in-world holder, it's possible, just a bit messy.
The |
It just occured to me that the current behaviour here is strange. To me it would make sense to allow hoppers picking up items for players with access to containers in the same claim. Thoughts, @Jikoo? |
I'm referring to the fact that the inventory type could be HOPPER but not actually be backed by a hopper minecart or a hopper. I don't think it's worth being concerned about (if anything, does the inventory type check even need to exist in vanilla? Would this offer better weird hybrid server support or futureproofing if it didn't check at all?) but if Robo does want that, it is possible to verify the holder without snapshotting it via inventory location. I made a post in the linked thread about it.
It prevents some abuse, i.e. I make a deathtrap over a hopper in a 1x1 subclaim with |
I haven't checked the implementation, but do you reckon a Per documentation the event is So it could either be removed entirely, or this current PR might avoid an xkcd#1172 situation, if another plugin is doing something weird.
Interesting point. |
This sort of stuff are things I kinda wanna remove, but I'll need to update statistics to see precisely how much PvE rules are in use. My guess is it's higher than creative claim users, and it is a very niche player-side grief. |
See RazielMartelus96's thread on SpigotMC for discussion related to use of
Inventory.getHolder()
.TL;DR
Inventory.getHolder()
return state snapshots, and this can be very slow for items that hold a lot of NBT data, like shulker boxes or custom items.GriefPrevention has a single call to this method:
https://github.com/TechFortress/GriefPrevention/blob/a8d55dbdcef13acad59fa5d14c77cd291ebaf830/src/main/java/me/ryanhamshire/GriefPrevention/BlockEventHandler.java#L1105-L1106
... and it's not needed.
Testing
Using the following code:
Then dropping a protected item spawned with:
Will reveal that different instances are indeed given for subsequent calls:
Solution
Simply change the GriefPrevention code referenced above to this:
This PR tidies up the related code, and as a result is not a one-line change.
In addition to the screenshot below proving behaviour is unchanged, there are also some tests included with the same intent. Feel free to omit those, if you don't want to add the approx. 2 second increase in build time Mockito adds.