Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vastly simplify mod initialization #110

Merged
merged 7 commits into from
Apr 16, 2024

Conversation

Technici4n
Copy link
Member

Redo mod loading to remove a lot of unnecessary indirection, and finally make the code understandable.

All the steps are now handled by straightforward methods in ModLoader such as dispatchParallelEvent or runInitTask. Dispatch happens in a single NeoForge class.

See neoforged/NeoForge#774.

@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Apr 3, 2024

  • Publish PR to GitHub Packages

Last commit published: f45adb69e7dd04a9794046cf689928a546f9f89b.

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 #110' // https://github.com/neoforged/FancyModLoader/pull/110
        url 'https://prmaven.neoforged.net/FancyModLoader/pr110'
        content {
            includeModule('net.neoforged.fancymodloader', 'earlydisplay')
            includeModule('net.neoforged.fancymodloader', 'loader')
        }
    }
}

@Elec332
Copy link

Elec332 commented Apr 10, 2024

There is a usecase (that would need s simple hook) that will no longer be possible after this PR, namely getting a callback after the modlist (net.neoforged.fml.ModList) has been set (AKA after all mods have been gathered).

Previously this was (perhaps unintentionally) possible with the following:

public class NeoStatesProvider implements IModStateProvider {

    @Override
    public List<IModLoadingState> getAllStates() {
        return List.of(ModLoadingState.withInline("POPULATE_MODLIST", "", ModLoadingPhase.GATHER, ml -> LoaderInitializer.INSTANCE.finalizeLoading()));
    }

}

(Until recently this had the downside that an IModStateProvider couldn't be loaded from a LANGPROVIDER so it had to be loaded from a JIJ'ed mod, but I just noticed this is no longer the case in NF)

The usecase for me specifically is a sub-loader that supports Neo, Forge & Fabric (and Quilt).
Like any modloader it builds a (mirrored & abstracted) list of containers before doing any initialization stuff.
This method allowed me to finalize the loader before going into the mod lifecycle (constructing & events).

I do have a net.neoforged.neoforgespi.language.IModLanguageProvider that does the containers for NF, which is relevant when the modlist is getting built. (https://github.com/Elecs-Mods/ElecLoader/blob/master/neo/src/main/java/nl/elec332/minecraft/loader/impl/neolang/NeoModContainer.java)

But when I have build the containers for NF I would now have no more options to hook into FML to make sure I can also built my modlist before the mods get constructed.

@Technici4n
Copy link
Member Author

@Elec332 thanks for the reply.

This method allowed me to finalize the loader before going into the mod lifecycle (constructing & events).

I don't have the impression that it must be done like that. Couldn't you build the mod list on-demand? For example the first time it is queried. You could also do the loader finalization the first time an ElecModContainer is built.

@Elec332
Copy link

Elec332 commented Apr 10, 2024

While a part of it could be done lazily like that (and I considered it before I found the custom loader state way worked, but I really disliked it), it is really not a nice way of doing things.

First of all saying "the first of an unknown number of mods is being loaded, so we're building the modlist I guess, let's do part of the finalization" is not a great way of doing things.
Secondly, the loader does some sanity checks against the "real" and mapped list and also makes sure that all custom ModContainers have been loaded correctly.
Lastly, finalizing the modlist lazily is in this case an ugly way to do it. When the first opportunity arises that the loader can be queried by a mod it should be fully finalized & immutable, just like all other modloaders.
(Also, because loading events are parallel it can block multiple mods while the loader is still lazily initializing)

It is currently possible with all (big) loaders; Fabric (and Quilt) makes their containers so early that a PreLaunchEntrypoint can lock in the modlist (and to make sure loading behavior is consistent between all loaders it my loader mimics the Neo/Forge way of loading mods later on, so it ends up not needing it that early) and for Forge I can use the above described method.

Neo would in this case be the one loader where it would be impossible to fully finilize the loader before any mods get loaded, which considering the amount of hooks available for custom 'stuff' in FML would seem a bit strange to me.

A nice place for something like this could be a (default) method in IModLanguageProvider.
It seems like there used to be ways to hook into the lifecycle, which are no longer in use:

default <R extends ILifecycleEvent<R>> void consumeLifecycleEvent(Supplier<R> consumeEvent) {
}

Since I guess lifecyle events were replaced by the IModStateProvider stuff (and a bit more of course) it would seem like there have been ways to hook into the lifecycle for a long time until now. Perhaps a reconsideration of a similar hook for the lifecycle, or for this case, a default method in IModLanguageProvider like

    default void afterAllModsGathered() {
    }

which would be called right before the new constructMods method could be considered.

@Technici4n
Copy link
Member Author

Some sort of SPI-loaded early initialization hook would make sense in my opinion. I could see it being used by other loader abstractions.

@Technici4n
Copy link
Member Author

@Elec332 I added IModListReadyCallback which should be a suitable replacement for your use case.

@Elec332
Copy link

Elec332 commented Apr 14, 2024

Looks good, thanks!

@Shadows-of-Fire
Copy link

Well I don't entirely know what structural changes are going on here, and it's non-obvious from just browsing the changeset, but I don't see anything egregious

@Technici4n Technici4n merged commit ad5ed5b into neoforged:main Apr 16, 2024
2 checks passed
@Technici4n Technici4n deleted the cleanup-transitions branch April 16, 2024 20:08
Elec332 added a commit to Elecs-Mods/ElecLoader that referenced this pull request Apr 16, 2024
Changed finalization logic for all loaders for consistent behaviour.

Will look into a better alternative later
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants