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

Make mixin entries from the mods.toml optional. #237

Merged
merged 7 commits into from
Jan 18, 2025

Conversation

marchermans
Copy link
Contributor

This allows modders to specify that a mixin config is optional.
It will not be loaded if it does not exist.

Usecase:

  • Compatibility mixins loaded from a seperate sourceset, for example when creating compatibility layers with shader mods.

@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Jan 18, 2025

  • Publish PR to GitHub Packages

Last commit published: 3be393341f68b35436955dfafdcd0db8c056da61.

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

@embeddedt
Copy link
Member

Interesting, but what is the advantage over the current convention of overriding shouldApplyMixin in a mixin plugin (which is also more flexible)?

@marchermans
Copy link
Contributor Author

Interesting, but what is the advantage over the current convention of overriding shouldApplyMixin in a mixin plugin (which is also more flexible)?

Mixin crashes before that if the json is not found in the jar.
So when I am running in none shader compat mode, it just hard crashes as the mixin json it self lives in the other sourceset with the compat code.

@@ -85,17 +88,32 @@ protected static List<CoreModFile> getCoreMods(final ModFile modFile) {
.toList();
}

private record PotentialMixin(String name, boolean optional) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: PotentialMixinConfig since it's a mixin config we're specifying, not an individual mixin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, let me fix that.

@Technici4n
Copy link
Member

That's really weird because it is a dev-only feature. I wonder if adding mixin configs that depend on some mods being loaded would not be a more general solution. 🤔

@marchermans
Copy link
Contributor Author

That's really weird because it is a dev-only feature. I wonder if adding mixin configs that depend on some mods being loaded would not be a more general solution. 🤔

Maybe, but this is a good first step, and when the need arises we can definitely iterate on this design and expand the conditions.

@Technici4n
Copy link
Member

It's weird to ever have this in a production environment. And in dev there should be a suitable way to trim the unwanted mixin entries, no? (E.g. Gradle magic)

We should also think about why this isn't done with accesstransformers.

@marchermans
Copy link
Contributor Author

It's weird to ever have this in a production environment. And in dev there should be a suitable way to trim the unwanted mixin entries, no? (E.g. Gradle magic)

We should also think about why this isn't done with accesstransformers.

It is possible to do with a lot of gradle magic, where gradle edits the file, and injects stuff into it, all in all, with running with IDEA support in the back of my mind (as that is now the default way of running my mods), it is a lot of work, for something I think we should start working towards (aka optional, conditional on mods, etc). This PR provides initial infrastructure to do that (with the capturing record, and filtering logic) to eventually get to full conditional support for this. But we can iterate and see what the community wants or needs.

@shartte shartte merged commit 17de0d5 into main Jan 18, 2025
4 checks passed
@neoforged-releases
Copy link

🚀 This PR has been released as FancyModLoader version 6.0.6.

@Technici4n Technici4n deleted the feature/optional-mixins branch January 18, 2025 20:34
marchermans added a commit that referenced this pull request Jan 19, 2025
… when certain mods are present. (#237)

(cherry picked from commit 17de0d5)
marchermans added a commit that referenced this pull request Jan 19, 2025
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