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

[RFC] Convert Forge's CoreMods to Mixins, deprecate JS CoreMods #10177

Draft
wants to merge 3 commits into
base: 1.21.x
Choose a base branch
from

Conversation

Jonathing
Copy link
Member

It's time to kill CoreMods.

Background

Since the FML rewrite in 1.13, JS CoreMods have existed as a replacement to the previous Java-based CoreMods transformers that has existed in versions 1.12.2 and older. This was done to keep transformers within a sandbox where they couldn't cause unhinged early classloading issues and other nonsense. However, since 1.16 (and eventually 1.15), Forge has been shipping SpongePowered's Mixin as an alternative solution to transforming classes at runtime. It has since become the defacto "coremodding tool" and effectively supersedes CoreMods. Besides, if people really wanted to write ASM transformers in Java, they already can with ModLauncher services.

All that said, I have made this preliminary PR for a handful of reasons:

  1. Begin the process of converting Forge's existing CoreMods into Mixins.
  2. Get help implementing and testing Mixins in ForgeDev.
  3. Garner discussion and feedback on the topic of the deprecation of CoreMods and the potential enhancements of Mixin in ForgeDev and UserDev.

Migration to Mixins

With regards to migrating to Mixins, there are a few key points that need to be addressed.

Forge's Mixins

As of the creation of this PR, I have converted the existing CoreMods into Mixins.

  • These Mixins are very verbose, but offer the same functionality that these CoreMods did.
  • Additionally, since these Mixins are written in Java, it can prevent bugs such as [1.21.3] MobSpawnEvent.FinalizeSpawn is never fired #10157 from occurring at all.
  • In the future, similar Mixins could be made that move binary Forge patches into Mixins.
    • Of course, this would only apply to patches that would be similar to field or method redirects: repetitive patches that would be better to have effectively matched by mixin targets.

Mixin Libraries

  1. Are we going to continue to use SpongePowered Mixin? As in, do we have any need to fork it to include patches of our own?
  2. Do we want to include LlamaLad's MixinExtras in ForgeDev and UserDev?
    • MixinExtras is an add-on companion library to Mixin that includes several safer injection types that keep mod compatibility (i.e., it is better to use @WrapOperation than @Redirect since MixinExtras can chain injectors of the former properly).
    • Fabric and NeoForge already bundle MixinExtras in production and their toolchains, which gives modders better defaults when it comes to creating safer injection points. The same could be done for Forge's UserDev.
    • Bonus points from me personally because I love MixinExtras and can't live without it (this is why).

Note

MixinExtras can already be, and is already commonly, shadowed in with mods by using the JarJar system. It is not imperative that this is added to ForgeDev or UserDev, but would add to the convenience of modders.

Deprecation of CoreMods

As mentioned above, an additional purpose of moving Forge's CoreMods to Mixins is to incentivize the deprecation, and eventual removal, of the CoreMods framework. CoreMods's only purpose was to provide a method of creating ASM transformers within a sandbox, and it has ultimately failed at that goal considering the status of Mixin and the availability of ModLauncher transformer services.

So, we need to decide the following:

  1. When are we going to officially deprecate CoreMods? When this PR is merged? The following Minecraft version after this PR is merged?
  2. When are we going to remove CoreMods from Forge? The following major Minecraft version after this PR is merged?
    • For example, if this PR was merged during 1.21.3's lifecycle, then CoreMods could be set for removal in 1.22.

Note

Raw ASM transformers can still be created, with Mixin, by using a Mixin config plugin and overriding postApply().

Merge Process

Before this PR can be merged, the following must happen:

  • The core premise must be approved by the Core team.
  • The PR must be fully complete and removed from draft status.
  • ForgeDev, UserDev, and Installer must be thoroughly tested to make sure that Forge's own Mixins do not cause problems with the game environment.
    • In addition, the IDE support of Mixins must be identical to that of UserDev. If this isn't done automatically, it may be necessary to look into bringing in some hacks from MixinGradle.
  • Some sort of plan needs to be discussed, or implemented, regarding the deprecation of JS CoreMods in favor of Mixin.
  • The PR must be approved by the Triage team.

Disclaimers

Please note that, as of writing, this PR is still a work-in-progress and things about it may change, including the information in this PR description and the discussion surrounding it. Major testing still needs to done and discussions need to happen in order for this to go anywhere. There is no time limit, but I hope that this RFC PR is enough to start preparing for this change.

None of the changes in this PR will be backported to any versions before the merged version unless there are specific changes that can be which don't break compatibility, binary or otherwise.

Important

If you are a modder, you should not be using CoreMods or Mixins before attempting to use existing Forge APIs. You are more than welcome to write PRs that add your desired functionality to the mod loader.

- Minecraft Forge can now ship its own Mixins.
- `ForgeMixinPlugin` is used as both the connector and plugin.
  - The plugin can be very useful if we need to do manual ASM for any
unhinged reason.
- `minecraftforge.mixins.json` acts as the Mixin counterpart to
`coremods.json`, giving all the necessary info for Forge's mixins.
@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. RFC This request aims to gather opinions on something that may or may not be planned for the future. labels Nov 17, 2024
@Jonathing Jonathing added Regression This request demonstrates something missing in a newer version that was present in a prior version. Feature This request implements a new feature. Cleanup This request reorganizes messy code, or fixing it would require doing so. Work In Progress This request has lots of changes that need attention. labels Nov 17, 2024
@PaintNinja PaintNinja self-requested a review November 17, 2024 23:39
@Redirect(
method = {
"<init>(Lnet/minecraft/world/effect/MobEffectInstance;)V",
"getParticleOptions()Lnet/minecraft/core/particles/ParticleOptions;",
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way that these strings can be encoded better to provide better error when things change?
@MethodRef("<init>", args = { MobEffectInstance.class }, ret = Void.class) or something?
Perhaps a build script task that verifies that we have all the expected targets?
We already have the code to generated the json that feeds the existing coremod, can something be expanded to mixins?

This comment was marked as outdated.

@Jonathing
Copy link
Member Author

Jonathing commented Nov 18, 2024

As discussed on Discord, I will be taking a different approach to these larger-scale Mixins that require several classes in order to have the same functionality that the original CoreMods did. This will likely mean writing a tool that generates Mixin classes on Gradle build. Smaller-scale mixins, like for patches we make already through bin patch, are more suited to the way Mixins are currently written.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21 Cleanup This request reorganizes messy code, or fixing it would require doing so. Feature This request implements a new feature. Regression This request demonstrates something missing in a newer version that was present in a prior version. RFC This request aims to gather opinions on something that may or may not be planned for the future. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. Work In Progress This request has lots of changes that need attention.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants