Experiment with fully integrating EventBus 7 into Forge #1

PaintNinja · 2025-03-18T22:51:48Z

Unlike the PoC PR, this tries to get it compiling and booting, without taking full advantage of the new features.

My observations from this experiment:

FML is really, really horrible in regards to getting this working. So many gross hacks and workarounds involved...
- Classloader shenanigans where you sometimes have the event instance but can't call getClass() on it
- ~~Thread-safety issues, sometimes requires multiple attempts to successfully boot~~
The separation of cancellation state from the event object itself is problematic for patches and various events
- A lot of events do a fire(), check isCanceled() and may read other parts of the event object even when cancelled. The separation of cancellation state means that you need to hold the object, post(), then conditionally return null for some events, and longer patches for other events
- Some events even call setCanceled() inside certain unrelated methods or set a custom cancellation state before the event is posted
- I gave up after awhile and put a dummy interface in place to move onto other areas
Forge's networking assumes the old architecture. Can get it working with more effort, but not a find and replace level of change and may need some internal redesign
RenderHighlightEvent.Entity seems to have an oversight where it's not intentionally cancellable but inherits cancellable from the superclass
RenderLivingEvent involves instance generics that aren't accessible from a static final context. Not sure how to solve this

This compiles and boots, but fails to load into a world due to the networking internals not being fully ported over yet.

Jonathing · 2025-03-18T23:10:50Z

Yup. A lot is going to need to be cleaned up in order for us to play with our new legos. Do what you are able to for now, while I do my best to try and get FG7 underway.

PaintNinja · 2025-03-18T23:13:50Z

src/main/java/net/minecraftforge/common/ForgeMod.java

LexManos

overall it looks fine, bulk of the changes being adding the bus to each event.
need to go through and migrate as many things off the mod bus as possible for simplicities sake.
should be simple enough to gather a list of events and throw exception if wrong bus.

aside from that a few comments.
tons of your todos to finish

would it also be possible to have a

public Helper extends EventBusGroup {
  register(class) {
    wrapped.register(getLookup(class), class)
  }
}

namely a wrapper with helprr functions doing stuff you dont want in the official event bus api, but i want exposed to modders to make their lives easier.

LexManos · 2025-03-19T00:39:53Z

src/main/java/net/minecraftforge/event/AddReloadListenerEvent.java

@@ -29,7 +28,9 @@
 * The event is fired on each reload and lets modders add their own ReloadListeners, for server-side resources.
 * The event is fired on the {@link MinecraftForge#EVENT_BUS}
 */
-public class AddReloadListenerEvent extends Event {
+public class AddReloadListenerEvent extends MutableEvent {
+    public static final EventBus<AddReloadListenerEvent> BUS = EventBus.create(AddReloadListenerEvent.class);


Why do these need MutableEvent?

They don't, but you asked for no record events at first :P

Changing them would be trivial, I left it because the PoC already showcased the feature-set and I wanted to focus more on the integration side of things/getting it booting.

I mean why is it using MutableEvent instead of just Event? I thought that was only needed for monitor aware things. I'm still not solid on the name/use of MutableEvent

Multiple reasons, should be clear from the javadocs once @Jonathing gets around to it. I've left a couple of explainer comments here and here

src/main/java/net/minecraftforge/event/AnvilUpdateEvent.java

LexManos · 2025-03-19T00:43:32Z

src/main/java/net/minecraftforge/event/AttachCapabilitiesEvent.java

@@ -21,8 +21,9 @@
 * Please note that as this is fired for ALL object creations efficient code is recommended.
 * And if possible use one of the sub-classes to filter your intended objects.
 */
-public class AttachCapabilitiesEvent<T> extends GenericEvent<T>
-{
+public class AttachCapabilitiesEvent<T> extends MutableEvent {


the only sane way for now is to make this abstract and have the concrete subclasses for each "generic" we have

LexManos · 2025-03-19T01:21:31Z

src/main/java/net/minecraftforge/client/loading/ClientModLoader.java

@@ -108,7 +109,7 @@ public static boolean completeModLoading() {
        File dumpedLocation = null;
        if (error == null) {
            // We can finally start the forge eventbus up
-            MinecraftForge.EVENT_BUS.start();
+            BusGroup.DEFAULT.startup();


I think we should spend the time to move as many of the mod bus events to the default bus, and instead of starting the bus here. we disable it in the error handler. as the point is trying to get minecraft into a state where we can show a friendly error without mod event handlers erroring it out. it got polluted because a lot of vanilla code gets called, and thus events need to fire, before this handler.

I agree that we should move more events away from the mod bus. Some stuff is on there and doesn't really need to be, but some is there either because it's mod-specific (e.g. ConfigEvent only firing for configs your mod registered) or for parallel mod loading.

The whole 'try to stop mod loading but continue loading the game to show an in-game error screen' idea doesn't seem to work well enough in practice as some mods may have already loaded enough to start doing things and end up crashing the game before the error screen is showed.

These things are wider issues with FML in general that I'd like to tackle in the rewrite.

Yes overall things need to be handled in the re-write.
If we want to hold off on moving the events between busses until then, we can keep this as disabled.
But I would like to see some debug logger that prints out what events are fired before the bus is started.
Something so we can have a concrete list of things that need moving.

src/main/java/net/minecraftforge/common/ForgeHooks.java

LexManos · 2025-03-19T01:30:10Z

src/main/java/net/minecraftforge/common/MinecraftForge.java

    /**
     * The EventBus for all the Forge Events.
     *
     * Events marked with {@link net.minecraftforge.fml.event.IModBusEvent}
     * belong on the ModBus and not this bus
     */
-    public static final IEventBus EVENT_BUS = BusBuilder.builder().startShutdown().useModLauncher().build();
+    @Deprecated(forRemoval = true, since = "1.21.5")
+    public static final BusGroup EVENT_BUS = BusGroup.DEFAULT;// BusBuilder.builder().startShutdown().useModLauncher().build();


I dont think we should deprecate this, instead we should make a wrapper that has helpers like not needing to pass in the module lookup, event type, and the link.

LexManos · 2025-03-19T01:37:48Z

src/main/java/net/minecraftforge/common/util/CancellableAware.java

@@ -0,0 +1,10 @@
+package net.minecraftforge.common.util;
+
+// Temporary solution for events that rely on the cancellation state being stored inside the event object


internal get vs external set? or perhaps turn this into a record <T> Result(boolean canceled, T event){}?

src/main/java/net/minecraftforge/event/brewing/PotionBrewEvent.java

LexManos · 2025-03-19T01:53:20Z

src/main/java/net/minecraftforge/event/entity/player/PlayerInteractEvent.java

-                useItem = Result.DENY;
-            }
-        }
+        // Todo: [Forge][Event] PlayerInteractEvent.RightClickBlock#setCanceled(boolean) override


i think for these we just need to have this propogation on the event factory side

LexManos · 2025-03-19T02:00:39Z

src/main/java/net/minecraftforge/network/NetworkInstance.java

@@ -102,4 +101,8 @@ boolean isRemotePresent(Connection con) {
        var channels = NetworkContext.get(con).getRemoteChannels();
        return channels.containsAll(ids);
    }
+
+    public BusGroup getNetworkEventBusGroup() {


the point is to hide this event stuff from modders. honestly killing off all event style networking would probable be better then trying to migrate it

Made all subclasses package-private too.

And fixed commands...

Signed-off-by: RealMangoRage <[email protected]>

PaintNinja added 5 commits March 18, 2025 15:13

Start fully porting Forge's EventBus usages to v7

162849f

Minimal migration of event classes

7da99d2

Migrate HasResult

7775b30

Fix usages, it now compiles

1d63a79

Some hacks to get it booting

7ac3530

LexManos reviewed Mar 19, 2025

View reviewed changes

src/main/java/net/minecraftforge/common/ForgeMod.java Outdated Show resolved Hide resolved

LexManos reviewed Mar 19, 2025

View reviewed changes

Fix oversight in PotionBrewEvent.Pre/Post

3a099f7

PaintNinja mentioned this pull request Mar 23, 2025

[7.0-beta] Fix recursive update MinecraftForge/EventBus#71

Merged

PaintNinja and others added 15 commits April 2, 2025 21:55

Fix concurrency issues

82ba0fa

AttachCapabilitiesEvent.java converted

5367f66

Made AttachCapabilitiesEvent.java post-able now...

5bcfe9e

Made all subclasses package-private too.

Fixed SubscribeEvent's

f913f4e

Further work, got 4 classes dealt with

8e4f99e

Further work, got another 4 classes dealt with

c0b4216

Gen patches

a68f15c

Fix Screen patch

e1da4ce

Fix most networking events, now ingame

e6d3512

Add IModBusEvent#getBus and convenience methods

ea4e466

Game now ACTUALLY runs properly...

3e75d8f

And fixed commands...

Fixed AttachCapabilitiesEvent

3d14d7a

Fixed AttachCapabilitiesEvent

19ae35f

Signed-off-by: RealMangoRage <[email protected]>

Fixed RenderLivingEvent

7e80d6b

Signed-off-by: RealMangoRage <[email protected]>

Fixed AttachCapabilitiesEvent again...

afa9de7

Signed-off-by: RealMangoRage <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment with fully integrating EventBus 7 into Forge #1

Experiment with fully integrating EventBus 7 into Forge #1

PaintNinja commented Mar 18, 2025 •

edited

Loading

Jonathing commented Mar 18, 2025

PaintNinja commented Mar 18, 2025

LexManos left a comment

LexManos Mar 19, 2025

PaintNinja Mar 19, 2025

LexManos Mar 19, 2025

PaintNinja Mar 19, 2025

LexManos Mar 19, 2025

LexManos Mar 19, 2025

PaintNinja Mar 19, 2025

LexManos Mar 22, 2025

LexManos Mar 19, 2025

LexManos Mar 19, 2025

LexManos Mar 19, 2025

LexManos Mar 19, 2025

		@@ -0,0 +1,10 @@
		package net.minecraftforge.common.util;

		// Temporary solution for events that rely on the cancellation state being stored inside the event object

Experiment with fully integrating EventBus 7 into Forge #1

Are you sure you want to change the base?

Experiment with fully integrating EventBus 7 into Forge #1

Conversation

PaintNinja commented Mar 18, 2025 • edited Loading

Jonathing commented Mar 18, 2025

PaintNinja commented Mar 18, 2025

LexManos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaintNinja commented Mar 18, 2025 •

edited

Loading