[1.21.x] Topologically sort reload listeners based on dependency ordering #1915

Shadows-of-Fire · 2025-01-25T09:27:36Z

Overview

This PR introduces dependency sorting for both client and server reload listeners. The sort is topological, using FML's TopologicalSort class as the backbone. Reload listeners are added to a DAG where an edge from a->b means that a must run before b.

The sorting logic, and the modder-facing API, is in SortedReloadListenerEvent, the parent class of both AddReloadListenerEvent and AddClientReloadListenerEvent (formerly RegisterClientReloadListenersEvent).

Requirements

As a prerequisite, this change enforces that all reload listeners are named. The name must be provided via SortedReloadListenerEvent#addListener at the time of registration. Vanilla listeners are preemptively named, via VanillaClientListeners and VanillaServerListeners, which hold the class->RL maps for all known vanilla listener types.

Note: Mods that are currently using mixins or other non-event means to inject reload listeners will crash after this change.

Sorting

Each SortedReloadListenerEvent maintains a Graph<PreparableReloadListener> which holds the directed edges that dictate the dependency information. An edge from a->b means that a must run before b. New edges are added to the graph via #addDependency(first, second), which creates a new edge from first->second.

When the vanilla listeners are added in the event's constructor, edges are added such that each vanilla listener depends on the previous one in-order. This guarantees that the vanilla order is maintained without relying on implicit behavior from the toposort.

Finally, after the event has concluded, but before the sort, a search is performed to find dangling nodes and attach them to the last vanilla node, such that all mod-added listeners run after vanilla if they do not otherwise specify dependency ordering.

Error Handling

A dependency cycle in the toposort is an unrecoverable error and will cause the game to crash. If any cycles are detected, an IllegalArgumentException will be thrown containing a string representation of the cycle(s).

For example:

java.lang.IllegalArgumentException: Cycles were detected during reload listener sorting:
0: neoforge:branding->neoforge:client_mod_loading->minecraft:language_manager->neoforge:branding

Notes

This PR supersedes #1289.

This class injects a ModLoader.hasErrors() check into code that would not be reached if the mod loader errors.

Also adds static keys for the neo-added listeners.

neoforged-pr-publishing · 2025-01-25T09:27:49Z

Publish PR to GitHub Packages

Last commit published: 2601dc47a353d69a67627b17b8cba5638f481e6d.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1915' // https://github.com/neoforged/NeoForge/pull/1915
        url 'https://prmaven.neoforged.net/NeoForge/pr1915'
        content {
            includeModule('net.neoforged', 'neoforge')
            includeModule('net.neoforged', 'testframework')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1915.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1915
cd NeoForge-pr1915
curl -L https://prmaven.neoforged.net/NeoForge/pr1915/net/neoforged/neoforge/21.4.89-beta-pr-1915-listener-sort/mdk-pr1915.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

neoforged-compatibility-checks · 2025-01-25T23:18:01Z

@Shadows-of-Fire, this PR introduces breaking changes.
Fortunately, this project is currently accepting breaking changes, but if they are not intentional, please revert them.
Last checked commit: 2601dc47a353d69a67627b17b8cba5638f481e6d.

`neoforge (:neoforge)`

net/neoforged/neoforge/event/AddReloadListenerEvent
- getListeners()Ljava/util/List;: ❗ API method was removed
- addListener(Lnet/minecraft/server/packs/resources/PreparableReloadListener;)V: ❗ API method was removed
net/neoforged/neoforge/common/NeoForgeEventHandler
- resourceReloadListeners(Lnet/neoforged/neoforge/event/AddReloadListenerEvent;)V: ⚠ API method was removed
net/neoforged/neoforge/client/event/RegisterClientReloadListenersEvent
- ❗ API class no longer exists
net/minecraft/server/packs/resources/ReloadableResourceManager
- registerReloadListenerIfNotPresent(Lnet/minecraft/server/packs/resources/PreparableReloadListener;)V: ❗ API method was removed

src/main/java/net/neoforged/neoforge/common/util/VanillaClassToKey.java

src/main/java/net/neoforged/neoforge/client/event/AddClientReloadListenerEvent.java

src/main/java/net/neoforged/neoforge/resource/NeoListenerNames.java

src/main/java/net/neoforged/neoforge/resource/VanillaClientListeners.java

src/main/java/net/neoforged/neoforge/resource/VanillaServerListeners.java

src/main/resources/META-INF/accesstransformer.cfg

Matyrobbrt

Looks fine to me, but someone else should review it before merging. When it is merged, do remember to make an announcement as this is a breaking change.

Technici4n · 2025-02-02T21:41:21Z

src/main/java/net/neoforged/neoforge/client/ClientNeoForgeMod.java

+        event.addListener(NeoForgeReloadListeners.CLIENT_MOD_LOADING, ClientModLoader::onResourceReload);
+        event.addListener(NeoForgeReloadListeners.BRANDING, BrandingControl.resourceManagerReloadListener());
+
+        // These run before vanilla reload listeners, so we add them before LANGUAGE, the first vanilla one.


Is it worth having a special FIRST field in VanillaClientListeners? And maybe LAST?

Maybe? The field indices are already ordered by their vanilla order, but sentinel fields that always point to the current first/last values might be useful (or a liability, if we forget to update them...)

They're even more of a liability for modders I think.

In that we should prefer to not add them? Or in that relying on non-sentinel fields would be dangerous for modders?

Yeah I mean that it's a liability for every modder if we don't provide these fields.

Technici4n · 2025-02-02T21:42:16Z

src/main/java/net/neoforged/neoforge/client/event/AddClientReloadListenerEvent.java

+ * 
+ * @see {@link AddReloadListenerEvent} for registering server-side reload listeners.
+ */
+public class AddClientReloadListenerEvent extends SortedReloadListenerEvent implements IModBusEvent {


Add is weird naming compared to Register, as it implies that things can be added multiple times somehow. Also, anyone searching for the event will probably look for "RegisterReloadListener" or similar.

We had this discussion a bit earlier:

As for the names, I used Add because we aren't registering anything. There is no registry for these objects, they're being added to a List.

As for user discoverability, I've found that people are usually aware of the AddReloadListenerEvent but are not aware of the RegisterClientReloadListenersEvent.

The list counts as the "registry" in this case. See also the registration of color providers which is just adding stuff to a map.

Technici4n · 2025-02-02T21:44:36Z

src/main/java/net/neoforged/neoforge/event/AddReloadListenerEvent.java


 /**
 * The main ResourceManager is recreated on each reload, just after {@link ReloadableServerResources}'s creation.
 *
 * 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 NeoForge#EVENT_BUS}
 */
-public class AddReloadListenerEvent extends Event {
-    private final List<PreparableReloadListener> listeners = new ArrayList<>();
+public class AddReloadListenerEvent extends SortedReloadListenerEvent {


Should the name not contain Server somehow?

It could, but I'm okay with the current name. I prefer to imply Server as a default state that doesn't need to be present in the class name, while client-only is non-default and must be reflected (hence AddClientReloadListenerEvent).

A silly example: we have ServerStartedEvent not StartedEvent. The event is limited to server reload listeners and the naming should make it clear. Even the documentation mentions server-side resources.

Technici4n · 2025-02-02T21:46:37Z

src/main/java/net/neoforged/neoforge/resource/NeoForgeReloadListeners.java

+ * @see {@link VanillaClientListeners} for vanilla client listener names.
+ * @see {@link VanillaServerListeners} for vanilla server listener names.
+ */
+public class NeoForgeReloadListeners {


Make class final and add a private noarg constructor.

Technici4n · 2025-02-02T21:50:32Z

src/main/java/net/neoforged/neoforge/resource/VanillaClientListeners.java

+ * @see {@link VanillaServerListeners} for vanilla server listener names.
+ * @see {@link NeoForgeReloadListeners} for Neo-added listener names.
+ */
+public class VanillaClientListeners {


final and private constructor.

Technici4n · 2025-02-02T21:50:49Z

src/main/java/net/neoforged/neoforge/resource/VanillaClientListeners.java

+ * @see {@link VanillaServerListeners} for vanilla server listener names.
+ * @see {@link NeoForgeReloadListeners} for Neo-added listener names.
+ */
+public class VanillaClientListeners {


Should be moved to a package under net.neoforged.neoforge.client.

Guessing that needs to be moved for #1920

Sucks for discoverability, but not much else to do about it. Links from the other classes will suffice.

Indeed. And the links in the other classes will break with split sources but that's no big deal.

Technici4n · 2025-02-02T21:51:06Z

src/main/java/net/neoforged/neoforge/resource/VanillaServerListeners.java

+ * @see {@link VanillaClientListeners} for vanilla client listener names.
+ * @see {@link NeoForgeReloadListeners} for Neo-added listener names.
+ */
+public class VanillaServerListeners {


final + private ctor

Honestly both of those things are pointless. Extensions of this class are meaningless, and so would be instantiations.

I'll stand by not bothering with either unless there is a necessity for them.

It's just good practice, and if we ever get around to generating javadoc it will at least stop generating the constructor.

Shadows-of-Fire added 4 commits January 12, 2025 23:34

Remove WrappedStateAwareListener

e9f6da0

This class injects a ModLoader.hasErrors() check into code that would not be reached if the mod loader errors.

Generate keys for all the vanilla listeners

1f4c404

Also adds static keys for the neo-added listeners.

Sortable reload listeners!

0031246

Rename RegisterClientReloadListenersEvent

cb1f642

Shadows-of-Fire added enhancement New (or improvement to existing) feature or request 1.21.4 Targeted at Minecraft 1.21.4 labels Jan 25, 2025

Shadows-of-Fire self-assigned this Jan 25, 2025

Shadows-of-Fire marked this pull request as draft January 25, 2025 09:27

formatting

290c9be

Shadows-of-Fire marked this pull request as ready for review January 25, 2025 21:08

Shadows-of-Fire requested review from Technici4n and Matyrobbrt January 25, 2025 21:09

Shadows-of-Fire added 2 commits January 25, 2025 13:30

Move sort impl out of the modder-facing event

da6e4bd

licenses

d9488b3

Self-review pass

34d60b0

Matyrobbrt requested changes Jan 26, 2025

View reviewed changes

Shadows-of-Fire added 2 commits January 26, 2025 16:49

Map finality, class rename, internal markers

5175834

Remove AT entry

d06bcd0

Shadows-of-Fire requested a review from Matyrobbrt January 27, 2025 00:58

Shadows-of-Fire added 2 commits January 26, 2025 17:40

formatting

bed8e94

De-finalize the listeners field

2601dc4

Matyrobbrt approved these changes Feb 2, 2025

View reviewed changes

Technici4n requested changes Feb 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.21.x] Topologically sort reload listeners based on dependency ordering #1915

[1.21.x] Topologically sort reload listeners based on dependency ordering #1915

Shadows-of-Fire commented Jan 25, 2025 •

edited

Loading

neoforged-pr-publishing bot commented Jan 25, 2025 •

edited

Loading

The artifacts published by this PR:

Repository Declaration

MDK installation

neoforged-compatibility-checks bot commented Jan 25, 2025 •

edited

Loading

Matyrobbrt left a comment

Technici4n Feb 2, 2025

Shadows-of-Fire Feb 2, 2025

Technici4n Feb 4, 2025

Shadows-of-Fire Feb 4, 2025

Technici4n Feb 4, 2025

Technici4n Feb 2, 2025

Shadows-of-Fire Feb 2, 2025

Technici4n Feb 4, 2025

Technici4n Feb 2, 2025

Shadows-of-Fire Feb 2, 2025

Technici4n Feb 4, 2025

Technici4n Feb 2, 2025

Technici4n Feb 2, 2025

Technici4n Feb 2, 2025

Shadows-of-Fire Feb 2, 2025

Technici4n Feb 4, 2025

Technici4n Feb 2, 2025

Shadows-of-Fire Feb 2, 2025

Technici4n Feb 4, 2025

[1.21.x] Topologically sort reload listeners based on dependency ordering #1915

Are you sure you want to change the base?

[1.21.x] Topologically sort reload listeners based on dependency ordering #1915

Conversation

Shadows-of-Fire commented Jan 25, 2025 • edited Loading

Overview

Requirements

Sorting

Error Handling

Notes

neoforged-pr-publishing bot commented Jan 25, 2025 • edited Loading

The artifacts published by this PR:

Repository Declaration

MDK installation

neoforged-compatibility-checks bot commented Jan 25, 2025 • edited Loading

neoforge (:neoforge)

Matyrobbrt 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

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

Shadows-of-Fire commented Jan 25, 2025 •

edited

Loading

neoforged-pr-publishing bot commented Jan 25, 2025 •

edited

Loading

neoforged-compatibility-checks bot commented Jan 25, 2025 •

edited

Loading

`neoforge (:neoforge)`