neoforged / bus Goto Github PK

Event listeners ordinarily do not receive cancelled events. However, they can specifically opt-in to receiving cancelled events through either the receiveCanceled parameter of the IEventBus#addListener methods or the receiveCanceled field of the @SubscribeEvent annotation.

However, code investigation and testing (prompted by @PlatinPython, thanks btw) shows that the receiveCanceled field of the @SubscribeEvent annotation is not respected, thus causing cancelled events to never reach such an annotated listener. (The addListener methods are unaffected as far as I can tell.)

Sample output from the code below:

Sending non-canceled event
StaticListener: Received event; canceled: false
InstanceListener: Received event; canceled: false
Sending canceled event

Sample code:

import net.neoforged.bus.api.*;

class Scratch {
    public static void main(String[] args) {
        final IEventBus bus = BusBuilder.builder().build();
        final TestEvent event = new TestEvent();

        bus.register(StaticListener.class);
        final InstanceListener listener = new InstanceListener();
        bus.register(listener);

        System.out.println("Sending non-canceled event");
        bus.post(event);

        event.setCanceled(true);
        System.out.println("Sending canceled event");
        bus.post(event);
    }

    static class StaticListener {
        @SubscribeEvent(receiveCanceled = true)
        public static void listen(TestEvent event) {
            System.out.println("StaticListener: Received event; canceled: " + event.isCanceled());
        }
    }

    static class InstanceListener {
        @SubscribeEvent(receiveCanceled = true)
        public void listen(TestEvent event) {
            System.out.println("InstanceListener: Received event; canceled: " + event.isCanceled());
        }
    }

    public static class TestEvent extends Event implements ICancellableEvent {
    }
}

I don't see the use case, and it makes the implementation of ListenerList a lot more painful.

This should use listeners[x].listenerName() to have a more readable name (right now the method is unused!).

They could just be regular subclass fields. We could move Event.Result to forge as a general-purpose "boolean or default" enum.

Hi!
I've been discussing event priorities with someone over on the Twilight Forest Discord server as they wanted to change the priority of our death event so their mod would properly work. While doing this, we had the realization that only having 5 different priorities that had no sorting order (at least I don't think it does?) is a rather flawed system.

My proposal: take a page out of how Mixin does it and allow people to specify an int for the priority. The default could just be 0, and mods that need a lower priority could just specify a negative number if needed. Mods could then be sorted based on this priority (perhaps by the actual mod loading order if multiple mods use the same value) and it would allow for finer control of when things fire.

Minecraft Version: 1.21.1

NeoForge Version: 21.1.22

Logs:

Steps to Reproduce:

1. Join some world with Spark installed
2. Leave to Main Menu
3. Join world again
4. Profile memory and look for NeoForgeServerSparkPlugin
5. You will see 2 living instances, consequently 2 IntegratedServer instances

Description of issue:
When you unregister from the event bus, it marks the ListenerList to be rebuilt

The rebuild process is triggered when you call getListeners, which happens the next time this event is posted.

For this Spark case this ServerStoppingEvent will be only called again when you leave the server for a second time, making you always have at least 1 leaked object if you leave and join a world.

With that said, should Spark clear their object and do not rely on Event Bus to release their object or there is something that can be fixed on NF side?

As far as I can tell it's just a matter of adding such a method to Event:

    protected void reset() {
        isCanceled = false;
        result = Result.DEFAULT;
        phase = null;
    }

(event.setCanceled(false).setResult(Result.DEFAULT).setPhase(null) will not work due to setPhase)

Of course, this should come with a nice test too.

It doesn't seem too useful and right now it isn't particularly performant - once capabilities get reworked Forge won't be making use of it anymore.