FabricMC / fabric

Essential hooks for modding with Fabric.
Apache License 2.0
2.31k stars 404 forks source link

Extending fabric events to prevent recursion #2013

Closed ExcessiveAmountsOfZombies closed 2 years ago

ExcessiveAmountsOfZombies commented 2 years ago

Bit of an odd title.

I was taking a look at making an item that would break more blocks when you broke a single block ex: tree felling, hammers etc. and what I come across is if I want to be compatible with other mods that listen to PlayerBlockBreakEvents.BEFORE, I have to use weird workarounds to avoid recursion. Magna avoids this by mixing into the same place as the BEFORE event and adding a boolean check to see if it's one of their items (???).

In Bukkit, Sponge and presumably Forge (couldn't find a mod while researching that implemented something similar), I could avoid this fairly easily, in Bukkit's case I could extend their block breaking event with my own, and then in the original listener check if the event is an instanceof my event and just skip handling it if it is. This allows any other listeners to still handle if the event should cancel or not, but lets me skip it.

So really my question or request is would it be possible to have a similar feature with fabrics event system? I can't imagine that the event system gets rewritten to look more like bukkit/forge with the speed of fabric's event system being brought up.

Technici4n commented 2 years ago

What about the following?

// static variable somewhere
ThreadLocal<Object> BREAKING_MORE = new ThreadLocal<>();

// in the event listener
if (BREAKING_MORE.get() == null) {
    BREAKING_MORE.set(Boolean.TRUE);
    try {
        // break more blocks
    } finally {
        BREAKING_MORE.remove();
    }
}
warjort commented 2 years ago

What about the following?

// static variable somewhere
ThreadLocal<Object> BREAKING_MORE = new ThreadLocal<>();

I don't think you even need the ThreadLocal.

Minecraft is not thread safe so unless you break blocks on the main client/server thread(s) you are gonna have problems. Since this makes it single threaded, the overhead of a thread local is not needed, you can just use a normal variable.

See for example FTBUltimine (isBreakingBlock) here: https://github.com/FTBTeam/FTB-Ultimine/blob/0b1d049cfb6704387bfb6d74657fdea680323cc3/common/src/main/java/dev/ftb/mods/ftbultimine/FTBUltimine.java#L75

If you were going to use some non-global state, the player rather than the thread would be the more logical place for it?

Technici4n commented 2 years ago

Minecraft is not thread safe so unless you break blocks on the main client/server thread(s) you are gonna have problems. Since this makes it single threaded, the overhead of a thread local is not needed, you can just use a normal variable.

The event could be fired on the client and server threads at the same moment, and that's why it's imo preferable to use a thread local. Besides, the performance overhead is negligible here.

ExcessiveAmountsOfZombies commented 2 years ago

oh, hey that works pretty well actually. I don't know why I didn't think of that, I guess after being pointed toward how magna did it I figured there wasn't a better way. Thanks!