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

[1.20.6] Switch to Forge's fork of Fabric Mixin #9958

Open
wants to merge 2 commits into
base: 1.20.x
Choose a base branch
from

Conversation

PaintNinja
Copy link
Contributor

@PaintNinja PaintNinja commented May 4, 2024

This PR switches Forge from Sponge's Mixin to a fork of Fabric's, which offers various benefits, such as:

  • Support for "compatibilityLevel": "JAVA_21" in mixin json files

    • Internally this maps to JAVA_13, same as Forge does for "compatibilityLevel": "JAVA_17"
    • This just removes a hurdle some devs have when porting their Forge mods to 1.20.6, rather than crashing with an error saying 21 isn't supported
    • image
  • Support for interface mixins (FabricMC Mixin PR 51)

  • Improved performance/efficiency/memory usage due to avoiding allocations of unused CallbackInfo objects (FabricMC Mixin PR 52)

    • image

Known limitations:

  • Logs will say "from mod (unknown)" as we currently don't decorate Mixin config objects with their associated mod ID (a Fabric Mixin feature). I'm still looking into this and will consider fixing in a future PR, but it's not trivial to support amongst ModLauncher's and FML's complexity - especially without a dev experience regression where we would require mod devs to explicitly list all mixin configs in their mods.toml. Once I've figured out something nice for this, I'll do it later.

Quick Q&A

  • Q: Why not wait for Mumfrey to finish Sponge Mixin 0.8.6?

A: We've given him ample time to do this - much longer than downstream did, but we understand life can get busy and don't want to rush him. To avoid stagnation and help mod devs, this PR was made as a stop-gap solution. Once upstream Mixin catches up, we can switch back.

  • Q: What about safety?

A: We'll host our own fork that of Fabric Mixin that we'll keep up to date, so any concerns about that (however valid or paranoid they may be) are avoided.

  • Q: How do I test this?

A: You can either use this installer made for your convenience or do the usual testing process of cloning the PR branch, running the setup gradle task and publish: forge-1.20.6-50.0.7-1.20.6-fabric-mixin-installer.zip

  • Q: Are there any breaking changes?

A: Not to my knowledge - it should be a drop-in replacement. Did some testing and didn't run into any problems myself. The only change for Forge mod devs writing mixins from 1.20.4 is about no longer needing the MixinGradle plugin or refmaps, as 1.20.6 uses MojMap/official runtime mappings.

@PaintNinja PaintNinja added Feature This request implements a new feature. 1.20 labels May 4, 2024
@autoforge autoforge bot added the Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. label May 4, 2024
@autoforge autoforge bot requested a review from a team May 4, 2024 16:21
@PaintNinja PaintNinja added the Performance This request deals with performance, whether reporting issues or claiming to improve it. label May 4, 2024
@Jonathing
Copy link
Member

I think that the final decision of whether this PR should be merged or not should be up to Lex. I can't speak on his personal feelings and experiences on this, but I wouldn't blame him if his negative experiences working with external libraries causes him to veto merging this PR. It's not a comment Fabric Mixin itself, but it is at least important to keep this on record considering the state of affairs of the general Minecraft modding community.

@Jonathing Jonathing requested review from LexManos and removed request for a team May 4, 2024 16:35
@PaintNinja
Copy link
Contributor Author

Fair enough. One thing I'd like to reiterate is that concerns of safety shouldn't be an issue on the basis that we perform extra scrutiny on it and host our own copy. The goal of this PR isn't to promote Fabric, but to help solve a pain-point for some mod devs and make it easier for them to write safer, more compatible coremods when their use-case is too niche or complex for a PR, which in turn could help the wider Forge modding ecosystem.

@PaintNinja PaintNinja marked this pull request as draft May 5, 2024 16:23
@SrRapero720
Copy link
Contributor

About this PR
Can be good if (with this change) also was included MixinExtra? Is the most jarJar library just to make mixing more compatible each others

@Jonathing
Copy link
Member

I don't agree. While I use MixinExtras on my own projects, I don't want to get to a point where we just start bloating Forge more than it already has been.

@LexManos
Copy link
Member

LexManos commented May 8, 2024

If MixinExtras functions correctly when jarJar'd then there is no need for us to ship any particular flavor of it.
Eventually I want to stop shipping Mixin itself. And let the modders be able to ship whatever they choose.

Ideally the official Mixin would release an update making the bulk of Fabric's fork obsolete. But that hasn't happened hence looking for another solution.

At the end of the day Mixin is a library Forge doesn't use, and should be treated like any other library that Forge doesn't use.
In so much that they can be provided by mods, be it in the mods folder or JarJar'd, and Forge just loads/manages it like anything else.

Its not a promotion of condemnation of any variant of Mixin {tho yes I obviously have my own personal misgivings about the fabric one} It's just that Forge itself shouldn't dictate one or the other.

However the current state of JarJar is horrible and reportedly doesn't work for Mixin itself. So having a audited self-hosted variant is the best way to go as a interm step. I've spoken with Paint about my stance and left it in his hands.

@PaintNinja PaintNinja added the Work In Progress This request has lots of changes that need attention. label May 8, 2024
@SrRapero720
Copy link
Contributor

SrRapero720 commented May 8, 2024

Eventually I want to stop shipping Mixin itself. And let the modders be able to ship whatever they choose.

Android apps development works completely standalone on apps. Android doesnt provide anything else than a JVM and the android API to do whatever you want

This was done by security reasons (and don't bind apps to have a specific set of libraries each android version). And that specific ecosystem works because apps doesn't interact each other and whatever one app do doesn't affect others in runtime (also helps with the retro compatibility)

Modding on Minecraft is basically a connected ecosystem between Minecraft, MinecraftForge and the Mods jar. The distribution of these is not same as an Android app because it uses the libraries used on Minecraft and Forge so it can't run standalone. That means it requires a specific set of libraries.

Mixins is not just a arbitrary library who doesn't have repercussions on modding, is a commonly used library. And if is a basic tool not used by Minecraft or Forge but is used by the modders it was not bloat. Because is a modding tool like Forge, and if forge can't supply a specific need from a modder then have the mixin/MixinExtra library as a secondary option, so modders doesn't have to jarJar or shade the same library used by other mods. (JarJar is shit for these basic needs)

This is just to keep compatibility with other mods, in a future will end in version conflicts (an outdated mod will load earlier an outdated version of mixins) or conflicts with vanilla mixins and fabric mixins (because modders have their own preferences) or forked versions of mixins (because why not?). So that means compatibility is primordial over "let modders do shit".

Not because is not used by forge doesn't mean it should be removed just to make modders have to install a mixin mod dependency like the hell was done in 1.12.2 with 6 FUCKING MIXIN BOOTERS DEPENDENCY (and for some reason are appearing more of these)

Bloating is basically add libraries not even asked, used or needed on modding, such as put kotlin or Vulkan because that's for a specific mod and not a common use library

So please, reconsider that, is basically the worst idea ever had. Specially considering how mature was the state of modding with mixins.

@PaintNinja
Copy link
Contributor Author

PaintNinja commented May 8, 2024

Lex doesn't intend on unbundling mixin before JiJ is changed to support it properly, and doing so isn't a small task, so I wouldn't worry about it too much at the moment.

I'm of the opinion that Forge should bundle mixin & mixinextras for the same reason we offer popular events for mods to use that forge itself doesn't use. It's not much work to bundle and encourages their use over less friendly solutions like js coremods.

However, if it becomes a single line of code in a buildscript to JiJ mixin in the future and it resolves to the latest version of whatever mods pick (no bootstrap mods required), then while not ideal, I don't think it's the end of the world. It means people aren't reliant on Forge to update the bundled versions, and isn't complicated for mods to add.

@LexManos
Copy link
Member

LexManos commented May 8, 2024

Ya, I can't really type this without sounding like an ass. But your wall of text is both irrelevant to the current discussion. And completely misunderstanding/failing to read what has been said.

It will not cause compatibility issues in any more sense then the core issues using Mixins inherently causes.
Order of loading will not be an issue, the bootstrap mods will not be making a return.
All your fear mongering is unwarranted.

@ricksouth
Copy link

Mixins are essential for modding nowadays. It allows authors to go beyond the limits of a mod loader. If something doesn't exist for which there is a need, it can be created. Without having to redesign or PR changes for just one feature, that someone else wouldn't need.

Using the Multiloader template myself, it's often easier to use mixins to solve a problem than using a hacky solution for each. Even if Forge in itself doesn't use it, it seems like a major oversight removing its built-in support.

While I appreciate the Forge teams continued development in keeping everything updated, the trend seems to be towards temporary solutions currently. Keeping mixin compatibility without having the need to JiJ or depend on external dependencies which bloats mods needlessly would be much preferred. Just wanted to voice my opinion on this.

To remain on topic, using the Fabric mixin and mixin extras seems like a very good move to me. Especially for being able to use the same mixin code in a multiloader environment. Unfortunately users have to choose between 3 mod loaders, I'm trying to keep support for all of them. It would be great to have that same mindset for Forge, regardless of the bad history that has happened.

Copy link
Member

@Jonathing Jonathing left a comment

Choose a reason for hiding this comment

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

@MinecraftForge MinecraftForge locked as resolved and limited conversation to collaborators May 9, 2024
@MinecraftForge MinecraftForge deleted a comment from JTK222 May 9, 2024
@LexManos
Copy link
Member

LexManos commented May 9, 2024

Locking the conversation as nobody is providing any useful contributions. Nore are they taking the time to understand anything that is said by me. My entire point is that I do not want to deal with that type of stupidity/drama.

I am not an incompetent programmer, nore am I ignorant about Mixin's place in the community.
At the end of the day my proposal does nothing but give modders enhanced flexibility and attempt to reduce my interactions with the dramatic side of the community.
At the cost of 0 to 1 line change in a modder's build script.

There is literally no downside.

The course of actions has been decided, and will move forward.

@PaintNinja PaintNinja changed the title [1.20.6] Switch to Fabric Mixin [1.20.6] Switch to Forge's fork of Fabric Mixin May 9, 2024
@PaintNinja PaintNinja removed the Work In Progress This request has lots of changes that need attention. label May 9, 2024
@PaintNinja PaintNinja marked this pull request as ready for review May 9, 2024 21:05
@PaintNinja PaintNinja requested a review from Jonathing May 9, 2024 21:05
@autoforge autoforge bot requested a review from a team May 9, 2024 21:06
Copy link
Member

@LexManos LexManos left a comment

Choose a reason for hiding this comment

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

Good enough for a stop-gap until JiJ is re-designed with needed functionality.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.20 Feature This request implements a new feature. Performance This request deals with performance, whether reporting issues or claiming to improve it. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants