neoforged / fancymodloader Goto Github PK

The key services in (neoforge.)mods.toml was introduced in MinecraftForge 37.0.0, the 1.17.1 update. It uses a new class ModJarMetadata to implement this by using ModuleDescriptor.Builder.uses.
However, the Builder was created by newAutomaticModule, which doesn't allow calling uses:

   public ModuleDescriptor$Builder uses(String service) {
      if (this.automatic) {
         throw new IllegalStateException("Automatic modules can not declare service dependences");
      } else if (this.uses.contains(Checks.requireServiceTypeName(service))) {
         throw new IllegalStateException("Dependence upon service " + service + " already declared");
      } else {
         this.uses.add(service);
         return this;
      }
   }

(Taken from decompiled OpenJDK 21 (using FernFlower decompiler), but it's same for OpenJDK 16/17.)

This key is documented, and is still not removed in Fancy Mod Loader 3.0.19.

An example that "supports" from MinecraftForge 37.x.x to NeoForge 20.6.20-beta (or later), please gunzip it.

It is equivalent to the LIBRARY type at the moment - since we load the langproviders via ServiceLoader, we load them from LIBRARY jars too.

DisplayWindow forces minecraft classes to load too early in the "updateModuleReads" method. This needs to be refactored to not classload anything prior to actual launch.

If I want the biggest and most bloated modid, dangit I'll get it! It's my right to put in modids that causes people to have to buy a horizontal scrolling mouse! UI sizes? pfff just go off screen! All that has to change is these 2 regex right here

Embrace the freedom! U N L I M I T E D M O D I D S

(Note, I am joking lol. But wanted to kick start convo on if this limit is actually needed or if it was just some legacy limit. Datapacks themselves don't have the 63 character namespace limit iirc)

There’s one for squirrel but none for foxes. Asking on behalf of someone who dislikes foxes. But config option still sounds like a good idea anyway.

Wayland is becoming more and more popular between Linux desktop users. Many desktop environments support (or plan to support) Wayland. GLFW and LWJGL supports Wayland, too.

If one defined a dependency in the mods.toml file and omits the version range, the game will accept no versions of that dependency which is contradictory to what intuition would think. If a field is optional, then omitting it would imply no version restriction right? It's the opposite currently and bit me twice already.

So either make omitting version range mean accept any version of the dependency. Or make the version range a required field. Either is fine as long as it stops the confusion I keep falling for.

Neo allows for some optional arguments (IEventBus, ModContainer, FMLModContainer, Dist) to be passed to the constructor of a mod main class.

Initializing a mod via FMLConstructModEvent has long been an alternative to the constructor setup.
It would be great to be able to access those optional arguments from the event as well, especially ModContainer, which the event already has a package-private getter for (ModLifecycleEvent to be precise).

Right now I have to get the issue URL by casting net.minecraftforge.forgespi.language.IModFileInfo to net.minecraftforge.fml.loading.moddiscovery.ModFileInfo to extract the issue URL from it, because IModFileInfo has no getter for that.

ex.

public String getIssuesUrl() {
    var file = ModList.get().getModContainerById("modid").orElseThrow().getModInfo().getOwningFile();
    return file instanceof ModFileInfo info ? info.getIssueURL().toString() : "fallback";
}

Could not be done in 20.1 due to it being a potential (although unlikely) breaking change.

It would be much better to get the actual exception instead of the exception reflection throws when a method call results in an exception.

ModConfig::getHandler was used to get an implementation of ConfigFileTypeHandler for a specific ModConfig.
In #40 all invocations of that method have been inlined with ConfigFileTypeHandler#TOML.

The ability to return a custom file handler instance extending ConfigFileTypeHandler was useful to me, as it allowed to alter some behavior, mainly placing my server config files in .minecraft/config so they can be used globally instead of relying on the default local placement in .minecraft/saves/<world>/serverconfig.
https://github.com/Fuzss/puzzleslib/blob/98f507fdaf1fa4c33eb8e783d3713f15e231af24/1.20.1/Forge/src/main/java/fuzs/puzzleslib/impl/config/core/ForgeConfigFileTypeHandler.java#L73

I'm also aware that the Corail Woodcutter mod uses this method, since I got the idea from there: https://github.com/Corail31/woodcutter/blob/1.20/src/main/java/ovh/corail/woodcutter/config/CustomConfig.java

Getting ModConfig::getHandler back would be much appreciated, as there is no reason ConfigFileTypeHandler#TOML has to be inlined everywhere.
Alternatively giving this suggestion another thought would also solve this issue for me (since implementing that change to the server config file path is the main purpose of my custom ConfigFileTypeHandler).

This causes https://github.com/neoforged/FancyModLoader/blob/main/loader/src/main/java/net/neoforged/fml/util/DevEnvUtils.java#L72 to throw when it detects multiple Minecraft classes on the classpath

As of #19, the LocatedPaths.minecraftFilter() method can now return null by default when it was previously non-null, with no code previously indicating it was nullable. Consumers of the LocatedPaths record may break if they don't perform null checks on the minecraftFilter.

Wayland is becoming more and more popular between Linux desktop users. Many desktop environments support (or plan to support) Wayland. GLFW and LWJGL supports Wayland, too.

The ModLoadingContext introduces a fair amount of global state through the field private static final ThreadLocal<ModLoadingContext> context, which is primarily used to resolve modids automatically in registration code. This usage should be removed, as global variables promote poor design and reduce readability of all code that reaches them.

There are two uses of this global state:

1. Automatic modid deduction (for registration of registry entries, configs, and client objects, and for IMC).
2. Provision of the language-specific loading context (i.e., FMLJavaModLoadingContext).

Case 1 is trivially resolvable. Modders should (and are very capable) of providing a modid to registration code at the time of registration. At most it adds one method argument, and produces a much higher degree of readability and transparency, and the registered object's modid is not at the whim of a global state object.

Case 2 is resolvable by passing the language-specific loading context as a constructor argument to the target mod.

Calling it mods.toml is fairly ambiguous for various reasons. First, of course, mods.toml is already commonly associated with Lex' Forge, and NeoForge is not compatible with Forge mods. But even ignoring Forge this is ambiguous, just like the index.json in the initial Modrinth pack draft (which was later renamed to index.modrinth.json after I mentioned that).
It should be renamed to something clearly identifying it as NeoForge (following the Modrinth example, mods.neoforge.toml would be an idea, but it could be something else as well)

Seems a bit pointless, and the T generic for the type of the mod class is really not a pattern to encourage IMO.

If a language provider isn't present, an errors list is created and passed to the sorter.

However, ModSorter.sort completely ignores its second argument, leading to missing language providers (modLoaders) simply skipping mods with no more than a logged error message. The language can't even be put into dependencies, because mods with erroring language providers get entirely removed from the mod list (before dependencies are checked).

I had set an invalid version in development, something like development+202401010000-1.20.1, which failed to parse and then subsequently made it appear as if neoforge was missing entirely.

changing my dev version scheme to 1.20.1-development+202401010000 fixed this.

Yes, there was a message about my version failing to parse higher up, but this "missing javafml" error should not appear nonetheless.

tested on: 1.20.1-47.1.85

With the upcoming incompatible and conflicts clauses being added to mods.toml, it reminded me of how Forge ecosystem has kinda a problem with the mods.toml file limits. Specifically around what user/pack makers can control.

I am in the camp that the user/pack maker should have final say on what MC version and what mods a mod can run with. So there should be a way to have users/pack makers be able to override version ranges, incompatible, and conflicts for any given mod in their pack. Fabric has dependency overrides for a long while and I have seen users/pack makers make use of it to keep their packs running and updated properly. There has been no influx of reports to modders when their mods are forced to load so any concerns about issue report influx is not really a thing.
https://fabricmc.net/wiki/tutorial:dependency_overrides

Consider this, a modder does not maintain their mods forever and may not exist in modding scene forever. Uers and pack makers will continue to make packs and play on any Minecraft version. With the current system, if a mod does not load in a pack due to a mods.toml restriction, users/pack makers are stuck and cannot do anything forever.

Example 1:

Mod A and Mod B together will crash Mod A. Mod A as a result sets Mod B as incompatible to stop loading with it. Mod A's dev eventually leaves the modding scene. Mob B releases an update that fixes the crash. However, users and pack makers still cannot use the mods together because of the incompatible clause being unable to be overridden. This can happen with the version range too as mods can set "" as the range to not run with any version of another mod.

Solution 1: Fork the mod, change the mods.toml file, reupload it to CurseForge/Modrinth, and then add it to modpack to distribute. Problem of that? Ridiculously excessive, taking points and downloads away from original dev, requires mod to be open source and buildable, pack makers to know how to build project, and for CurseForge/Modrinth to allow a reuploaded clone to exist with only a mods.toml change. Awful in every sense of the word.

Solution 2: Allow pack makers to specify a dependency override for the mod in their modpack. Clean, simple, and works.

Example 2:

Mod A's dev is a jerk and decides to mark Mod B as incompatible or version range lock out Mod B in latest Minecraft version. Pack makers now have to either remove Mob B to update Mod A to fix crashes or whatever or remove Mod A. this is a blatant misuse of the mods.toml dependencies and users/pack makers have no way of bypassing it cleanly without yeeting mods or forking and reuploading. Dependency Overrides allow users/pack makers to say "no" and bypass the mods.toml misuse to keep the pack updated properly and running.

If mod loading fails early then the crash log will not include the mod list and will only include Minecraft & forge.

Example of this type of log: https://gist.github.com/NeoCamelot/5e5d94426d3d872bbb04256484740323

Edit:

After thinking about it for longer, the whole IMC system doesn't make much sense. What is the purpose of IMC? What problem is it trying to solve?

In the case of ObjectShare on Fabric, it's trying to account for the lack of load ordering on Fabric. With IMC, it's not quite clear. FML doesn't have a load order problem, mods can just ask to load one after the other and avoid IMC. And even if you can't rely on load order for some reason, you can use events instead.

Since the networking refactor, it is not being used anymore.

We might want to remove the extension point system in general: the heavy supplier usage makes it a bit painful and we could instead use more standard ways of registering things.

Just needs to be added to this map:

With the changes to Neo + FML, there is now much less information being sent into the User Agent. Notably, it is now missing the Minecraft version (previously included in the version itself) along with the actual implementation that is using FML. Accurately, the version it has provided is not even for FML, but for NeoForge. This means it is misleading.

Prior Example:
Java-http-client/17.0.1 MinecraftForge/1.18.2-40.2.13 journeymap/5.9.7

Current:
Java-http-client/17.0.9 FancyModLoader/47.1.47 extendedslabs/3.0.1

At the least it would be nice if "Minecraft/" was added to this User-Agent, so that it can be determined which version of the game is calling it.

Additionally, FML should not be the name, but it should be NeoForge instead.

Preferred:
Java-http-client/17.0.1 NeoForge/47.1.47 Minecraft/1.20.4 journeymap/5.9.7

Acceptable:
Java-http-client/17.0.1 NeoForge/1.20.4-47.1.47 journeymap/5.9.7

The signature is:

    interface IModLanguageLoader {
        <T> T loadMod(IModInfo info, ModFileScanData modFileScanResults, ModuleLayer layer);
    }

however T should actually be ModContainer, which is NOT part of the SPI. A bit tricky to handle I think.

EventBusSubscriber is currently an inner class of Mod for no obvious reason. This creates a double inner class and makes EventBusSubscriber less convenient to use. Additionally, according to Orion, both Mod and EventBusSubscriber are part of the Java language provider specification, yet these classes are in the common package. I propose that Mod be moved to the javafmlmod package and EventBusSubscriber be moved to an outer class in the same package.