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

Switch to Fabric Mixin #94

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Conversation

Matyrobbrt
Copy link
Member

@Matyrobbrt Matyrobbrt commented Mar 1, 2024

This PR switches FML to use Fabric Mixin instead of Sponge's.

Background

Mixin's last release was in December 2021, and until 2 months ago it saw no public commits. In late January, a 0.8.6 snapshot was pushed, but it has several issues and missing features (an injection point was broken, and the new mixin source feature isn't actually used), and we have seen no progress on fixing them.
Mixin has never shown a desire to bring on more than its current one maintainer (who has little time available), and PRs don't end up being merged for the same time reason. Moreover, our communication with the Mixin maintainer is horrendous, not only because there are few channels of communication (they use a Discord IRC bridge that only shows them 2 channels from our Discord server), but also because we have been pinging them constantly with no reply in weeks, and it has been that way for years.

Additionally, we have also promised in our 2023 end-of-year blog post to look into alternatives. It's been 2 months, it's about time.

The Fabric fork

Fabric has their own fork of Mixin, which has several features that "stock" mixin doesn't provide (although some are provided in the new snapshot, but they either cause other issues, or are simply not fully implemented):

  • injections in interface methods
  • automatic mixin method prefixing with the mod ID
  • fewer restrictions when it comes to constructor injection (stock mixin allows only RETURN targeting)
  • array creations in field initialisers
  • CIs are only created when needed, improving performance

Using the fork comes with no penalty for us, as we can proxy their maven and forever store their mixin artifacts, without allowing them to be changed.
Assuming Mixin will eventually have all of the features Fabric's mixin has, we can switch back to Sponge Mixin without causing any issues.

Testing

You can test that interface injections and mixin prefixing works by writing a mixin that mixes into a vanilla interface, and that throws an exception. The mod ID should show up in the stacktrace.

Testing previous behaviour

To test that Fabric's version of Mixin does not break intended Sponge Mixin behaviour, you can run with Mixin Booster in 1.20.1 packs

See Sinytra/Connector#383 too for more information.

@Matyrobbrt Matyrobbrt added the enhancement New feature or request label Mar 1, 2024
@TelepathicGrunt
Copy link
Contributor

I oppose!..

…to not merging this.

Looks good to me

@marchermans
Copy link
Contributor

Do we want to consider creating our own fork of mixin / fabric-mixin? So that we are not bound to their changes?

Or do we want to trust the process here?
I am not against this change, just want the answer to these questions documented.

@TheCurle
Copy link
Member

TheCurle commented Mar 1, 2024

I feel like Fabric Mixin is the better option over maintaining our own fork:

  1. We're already limited on available manpower.

  2. Fabric Mixin is extensively tested to provide what people want.

  3. Fabric Mixin is in a pseudostable state, no further features or changes are expected to be made to it.

  4. The intricacies and potential pain points of Fabric Mixin are well documented.

I feel like all of these are pure advantages and together form a good reason to use that over a custom fork, especially since getting a personal fork working will take yet more time to implement.

@marchermans
Copy link
Contributor

I feel like Fabric Mixin is the better option over maintaining our own fork:

1. We're already limited on available manpower.

2. Fabric Mixin is extensively tested to provide what people want.

3. Fabric Mixin is in a pseudostable state, no further features or changes are expected to be made to it.

4. The intricacies and potential pain points of Fabric Mixin are well documented.

I feel like all of these are pure advantages and together form a good reason to use that over a custom fork, especially since getting a personal fork working will take yet more time to implement.

That seems well documented and thought through. Perfect. I will approve the PR then

@marchermans marchermans merged commit efe2c67 into neoforged:main Mar 1, 2024
1 check passed
@Matyrobbrt Matyrobbrt deleted the fabricmixin branch May 15, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants