KaiKikuchi / QuickShop

A shop plugin for Bukkit
47 stars 41 forks source link

when sneak-to-create & sneak-to-trade are false, unable to break chest #139

Open smmmadden opened 7 years ago

smmmadden commented 7 years ago

I ran into an issue where I thought the bug was with OpenInv as when I set the /silent command to mute opening/closing of chest sounds, I was unable to then break the chest. I use /silent again to enable the sounds and I could break it. Though this isn't where the problem ended up being.

After removing all the plugins in my test server and going through one at a time, I got to QuickShop. When these two properties are set to false, sneak-to-create and sneak-to-trade, the default behavior is the left clicking of the mouse allows the player to create the Shop. Which would be fine, but the only way I could get the shop to actually create is using the shift + left mouse, then I saw the chat window prompt for the amount. If I just use the left mouse button, I get this: image It thinks I'm opening the chest silently but that's the right mouse button's event. I think I reported this last year when I first started using the plugin and we couldn't figure out why it worked for you and not for me. Now I can at least reproduce it on 1.12.

So the question is what is expected from a player striking a chest with an Axe (what they would use to break wood with). The only option they have is using an empty hand to break the chest OR set the two properties above to TRUE and always use the sneak (shift) key + left mouse.

Is this something you can reproduce now that I've provided the right steps? ;-)

KaiKikuchi commented 7 years ago

When creating a shop, QuickShop checks if you can open the chest by simulating a right click. This is necessary to prevent making shops on chests that aren't accessible to the player.

The message "That Private Chest is locked by you" is sent by another plugin, so I cannot do much about it.

Still your "steps to reproduce" aren't working for me (and most servers, as I asked few networks if they had your issues). sneak-to-create and sneak-to-trade settings are false by default, and that's what I am using on my test server. I can break shop chests with any tool (diamond axe included) or empty hand.

You should try with a new empty server with all default configs and just QuickShop, Vault and an economy plugin. It's likely that all your issues are caused by other plugins or some wrong configuration.

Tested with:

KaiKikuchi commented 7 years ago

I made a video about it: https://youtu.be/kXx8AMZHsbg

I am not sneaking and the config is the default one, with the "display-item" set to true.

smmmadden commented 7 years ago

Hi Kai, thank you for the details - I've just tested it out with the default settings and just having the basics to have QuickShop & OpenInv working (Vault, Perms, etc) and the Left Click with an Axe (IS) working as you suggested. So I posted this on the OpenInv developers (@Jikoo) page but I am not sure who needs to fix what at this point.

Jikoo commented 7 years ago

The problem is that when using SilentChest, OpenInv cancels right click interaction with the chest to prevent it being opened the normal way, then opens it silently. Not sure that we can resolve this - if I don't cancel the interaction, the chest will open normally. If I do, you will see the event cancelled.

There's a (poor) solution on my end - technically I COULD grab the current trace and search it for plugins before handling right clicks, but that seems excessively intensive.

There are a few solutions on your end, but most of them involve special casing OpenInv. 1) Unregister and re-register OpenInv's listener every time you check for interaction 2) Manually call the event, doing all the work the event system ordinarily would do for you.

The second allows you a couple different options. 1) Ignore any Listener originating from OpenInv 2) Ignore any Listener with priority Monitor (It's bad practice to cancel with that priority anyway. The reasoning was that OpenInv is not technically cancelling the event, only changing the method with which the same end result is achieved)

KaiKikuchi commented 7 years ago

@Jikoo thank you for your input. Unfortunately this is quite tricky and I think we shouldn't sacrifice performances just because a message pops in chat.

@smmmadden isn't SilentChest just for admins? Also even if OpenInv's SilentChest is cancelling the event while a shop is being created, QuickShop is not cancelling the event interaction with the chest so you should be able to break the chest anyway. Just ignore the message.

Jikoo commented 7 years ago

I agree, yeah, it seems excessive for me to grab a full trace just for this edge case. I did figure out why block breaking stops, though.

  1. QS calls the interaction
  2. OI handles the interaction by cancelling it and opens the chest inventory silently
  3. QS closes open inventories to close the chat window, if open
  4. Shop creation fails because the event was cancelled

So, it's not that left click interaction is cancelled, it's that an inventory window is opened and then immediately closed, causing the client to stop sending. Our options really haven't changed, but it's interesting to know what's actually happening.

KaiKikuchi commented 7 years ago

I don't think I can do the second option you suggested because I'm afraid it may affect cauldron-based servers.

Although I don't like this solution, just temporarly disabling the event listener for OpenInv should be the way to go because it's faster/easier to develop and fixes the issue.

KaiKikuchi commented 7 years ago

Actually @Jikoo what do you think about a simple boolean variable added to this https://github.com/lishid/OpenInv/blob/master/plugin/plugin-core/src/main/java/com/lishid/openinv/listeners/PlayerListener.java that if is true your onPlayerInteract will just return immediately? Because unregistering listeners seems to be tricky and I want to avoid using nms/craftbukkit/spigot code as much as possible.

Jikoo commented 7 years ago

Seems easy enough sans NMS:

List<String> pluginNames = // Configurable, possibly?
HandlerList handlerList = PlayerInteractEvent.getHandlerList();
List<RegisteredListener> listeners = new ArrayList<>();

for (String pluginName : pluginNames) {
    Plugin plugin = Bukkit.getPluginManager().getPlugin(pluginName);

    if (plugin == null || !plugin.isEnabled()) {
        // Plugin not present
        continue;
    }

    listeners.addAll(handlerList.getRegisteredListeners(plugin));
    handlerList.unregister(plugin);
}

// Call event
handlerList.registerAll(listeners)
KaiKikuchi commented 7 years ago

Yeah, I figured it out already in a similar way, sorry.

KaiKikuchi commented 7 years ago

@smmmadden Do you mind testing this?https://drive.google.com/file/d/0B9IQQiW8O1UEVFdJQ0RyLXlaM0U/view?usp=sharing

smmmadden commented 7 years ago

@KaiKikuchi / @Jikoo - thanks gents for working out a solution. I'm testing out the file you just provided. Will know shortly.

smmmadden commented 7 years ago

I'm able break the chest with the axe, sword, pix axe with the two properties set to false or true. The left click (without sneak), doesn't show block info for the block in hand clicking the chest. The click event is caught with QS, but no matter what block I click the chest with, it thinks it is a Diamond Axe. image

KaiKikuchi commented 7 years ago

What you have in your screenshot is a shop chest selling diamond axes.

Sorry but I have to ask you to follow this exact procedure:

smmmadden commented 7 years ago

worked perfectly, with /silent and both props set to false.

Thanks Kai -looks like that'll fix it. :-)