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

Introduce system mods to mod loading #8238

Merged
merged 13 commits into from
Mar 6, 2022

Conversation

sciwhiz12
Copy link
Contributor

@sciwhiz12 sciwhiz12 commented Dec 2, 2021

This PR implements and closes #8049 by introducing the concept of system mods to mod loading.

System mods are mods which are required to exist in the environment during mod loading. These may be specially provided mods (for example, the minecraft mod), or mods which are vital to the framework which FML is connected to (for example, Minecraft Forge and the forge mod). If any of these configured mod IDs cannot be found during mod loading, an exception is immediately thrown to halt the game.

These system mods are used as the only existing mods in the mod list if mod sorting or dependency verification fails. This allows later steps in the which use resources from these mod files to work correctly (up to when the error screen is shown and the game exits).

As an example: in #7993, because the forge mod was missing from this list, the forge universal JAR was not included when the Access Transformers subsystem was searching for AT configuration files to load. This caused a JVM error to be thrown when the first field (that was assumed to be transformed by AT at that point) tried to be accessed by Forge code.

Implementation notes

The set of mod IDs of system mods are passed in as FML service provider arguments: the coreGameMods option (when referenced in the command line: --fml.systemMods), which takes in comma-separated mod IDs for those core game mods.

These are then passed down through ModDiscoverer into ModSorter, which does the finding of the core game mods and adding them to the systemMods field (previously named forgeAndMC). The FML-only project is configured with only the minecraft mod as the core game mod, while the Forge project is configured with both minecraft and forge as core game mods.

The set of mod IDs of system mods are configured from the value of the FML-System-Mods manifest entry from first ModFile whose locator is MinecraftLocator.

Core game mods are mods which are required to exist in the environment
during mod loading. These may be specially provided mods (for example,
the `minecraft` mod), or mods which are vital to the framework which
FML is connected to (for example, Forge and the `forge` mod).

These core game mods are used as the only existing mods in the mod list
if mod sorting or dependency verification fails. This allows later
steps in the which use resources from these mod files to work correctly
(up to when the error screen is shown and the game exits).

Implements and closes MinecraftForge#8049
@sciwhiz12 sciwhiz12 added 1.18 Feature This request implements a new feature. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. labels Dec 2, 2021
@sciwhiz12 sciwhiz12 requested a review from cpw January 15, 2022 05:20
@cpw
Copy link
Contributor

cpw commented Jan 15, 2022

I see.. good idea..

Copy link
Contributor

@cpw cpw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure about passing it all the way in from launch, but otherwise OK.

@sciwhiz12
Copy link
Contributor Author

I was/am unsure if there is a better way to provide the set of core game mod IDs to the mod validator from FMLoader.
I thought about a file resource containing the mod IDs per line, but that would mean somehow configuring the path to the JAR so the resource can be loaded, which would possibly be a bit of a mess.
If anyone has a better idea, I'd like to hear it so we can see if it can be implemented cleanly. Otherwise, this PR is as ready as it'll ever be (pending reviews from Triage/Core for any possible defects).

@cpw
Copy link
Contributor

cpw commented Jan 16, 2022

Manifest? We could add a manifest entry to the forge jar listing the "core mods"?

@cpw
Copy link
Contributor

cpw commented Jan 16, 2022

Also, lets call them something other than "core mods". We have core mods already, and this would just confuse matters. These should be call "system mods" because they're part of ensuring basic system function.

@sciwhiz12 sciwhiz12 changed the title Introduce core game mods to mod loading Introduce system mods to mod loading Jan 16, 2022
@sciwhiz12
Copy link
Contributor Author

I agree with the rename to "system mods", for less confusion with "coremods".

For the manifest idea, I've found a prospective solution which involves iterating over the list of ModFiles in ModSorter#buildUniqueList, filtering out those which do not use MinecraftLocator, and taking the first FML-System-Mods manifest value from these mod files manifests.

However, this does have one big gotcha: the minecraft mod is now hardcoded as an always-existing system mod, because there is no clean way for me to introduce a manifest for the FMLOnly jar in the development environment. We could potentially change that by making the minecraft mod be added as the only existing system mod if there are no system mods configured via the manifest, but I leave that decision up to you.

@sciwhiz12 sciwhiz12 requested a review from cpw January 16, 2022 17:33
@cpw
Copy link
Contributor

cpw commented Jan 16, 2022

Is having minecraft hardcoded as a systemmod a bad thing? Given it's a requirement for basically everything else, it seems like a very small loss of generality... Is FML-System-Mod going to act as a flag? Do we expect many other mods to need this? It requires some very special setup to be able to play in this space, so exposing it generally will come at a very high potential for fuck ups..

@sciwhiz12
Copy link
Contributor Author

On further thought, minecraft being a hardcoded system mod is not really bad, since it is always present as added by MinecraftLocator, but I did want to note it's hardcodedness at least.

And FML-System-Mods is not a flag, but rather a comma-separated list of mod IDs which are designated as system mods. Given that the manifest value is (a) only taken from mod files whose locator is MinecraftLocator, which excludes any mod provided either via the regular locators or a custom locator, and (b) limited to the first found manifest entry from those mod files, it is highly unlikely if not impossible for mod developers to actually affect FML by inserting this manifest entry in their own JARs.

@cpw
Copy link
Contributor

cpw commented Jan 17, 2022

OK, so your idea is to use the MinecraftLocator to locate the "tag". That seems reasonable, agree.

This prevents the remote possibility of a custom mod locator which
extends MinecraftLocator to bypass this check.
Copy link
Contributor

@cpw cpw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that looks much better

@TheCurle TheCurle merged commit 3dcb4a2 into MinecraftForge:1.18.x Mar 6, 2022
@sciwhiz12 sciwhiz12 deleted the 1.18/GH-8049 branch March 6, 2022 14:21
tmvkrpxl0 pushed a commit to tmvkrpxl0/MinecraftForge that referenced this pull request Jul 24, 2022
Core game mods are mods which are required to exist in the environment
during mod loading. These may be specially provided mods (for example,
the `minecraft` mod), or mods which are vital to the framework which
FML is connected to (for example, Forge and the `forge` mod).

These core game mods are used as the only existing mods in the mod list
if mod sorting or dependency verification fails. This allows later
steps in the which use resources from these mod files to work correctly
(up to when the error screen is shown and the game exits).
@autoforge autoforge bot removed the Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. label Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.18 Feature This request implements a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] System mod concept
3 participants