New Mixin Fixer system

[XXMA16]

The system itself

There's still a lot of stuff that needs to be polished, mainly the error handling. I'm also not particularly satisfied with the naming and package structure so I might slightly alter them. The PR can be merged, but there's still a lot of refactoring to do.

Goal

The main advantage over the old system is its lack of reliance on InterceptingMixins and InterceptingMixinPlugins in order to placate Injectors, changing said Injectors directly by modifying the annotation and/or the method parameters and in some cases even the body, using raw ASM, although this should be avoided. By not relying on mixins there will be way less boilerplate (i.e.: things with 'Newerererer' in their name), making the project easier to maintain. When the pull request is ready to merge, this system should be able to fully replace the following:

• @Shim
• @PlacatingSurrogate
• @InterceptingMixin
• InterceptingMixinPlugin
• @LoudCoerce
• the shim sourceset
• all compat mixin configs

Usage

Similarly to how OptifineFixer works, addFixer method calls may be added to ModMixinFixer's private constructor, the first argument being the fully qualified name of the targeted mixin class and the second an IMixinFixer that changes the IMixinInfo and the ClassNode of the given mixin. Said changes will, at least in most cases, be to:

• an Injector's arguments, which is facilitated by the helper methods in MixinFixerUtils
• an Injector annotation, obtained via InjectionInfo.getInjectorAnnotation(mixinInfo, method)

Design

The system relies on Mixin internals (as suggested by the class with this name), in similar fashion to the way MixinExtras works, making use of an IExtension loaded by OptifabricMixinPlugin (which is the plugin of the main mixin config) and some hacky reflection. Because it does not rely on mixins, creating custom mappings from named to intermediary is unnecessary as RemappingUtils will be used instead.

Error handling

Since this system gives no clue whatsoever that OptiFabric tampered with the mixins of other mods, logging errors is required.
OptifabricSetup.LOGGER is now used in favor of STDOUT & STDERR, fixing the issue where messages aren't printed in the Minecraft console until the game starts. The logger, named "OptiFabric", is created via log4j's LogManager since slf4j doesn't exist in older versions.
All errors should be handled by either OptifabricMixinErrorHandler or MixinFixerExtension.postApply(). The latter takes care of any LVT related exceptions thrown by generated error injectors, while the former should take care of any other mixin related error. At the moment, potential exceptions thrown by OptiFabric itself aren't properly handled. Besides the obvious necessity of
properly logging errors, one of the goals of error handling is to allow the game to start and show the crash screen. A better way of providing the end user with crash information would be a dialogue box, similar to the way Fabric handles mod conflicts, due to the following:

• the crash screen errors are poorly formatted
• only one error is allowed
• the "Copy stack-trace" button doesn't provide sufficient information; the full logs are necessary
• Mixin's error handling is quite lacking
• if a mixin fails to apply, the respective class won't be replaced with the OptiFine equivalent (likely crashing the game)
• getting the game to start in order to show the crash screen is difficult

if a mixin fails to apply, the respective class won't be replaced with the OptiFine equivalent (likely crashing the game)

Concrete example: Fabric Model Loading API's ModelLoaderBakerImpl fails to apply without a patch. While the game does start (since the exception is caught in ModelLoader.bake()), the font cannot be loaded and there is a re-entrance issue ("Already patched net/minecraft/class_1088$class_7778"). By insisting (not returning when a class has already been patched, which is really bad), the class will successfully be replaced with OptiFine's, but only after heavily spamming the console with said re-entrance logs and "Unable to bake model: ...". I have no idea how to fix the re-entrance problem, but I want to move away from the Minecraft-based crash screen and simply show a dialogue box clearly stating the issue and showing potential fixes (namely downgrading the mod causing issues) anyways.

Fabric API 0.86.0+ fix

Quick note

If the Optifine cache in the .optifine folder already exists, the new mappings will not take effect. The easiest way of fixing this is manually deleting the .optifine folder. Users will likely expect the game to work properly if they update to a version with the changelog Fixed Fabric API 0.86.0+ without having to do any additional stuff, making this a slight issue.

Testing

The following configurations have been tested:

• 1.20.1 I5 FAPI 0.86.1
• 1.20 pre5 FAPI 0.83.0

This should suffice, but you can never be sure. I will do more testing soon.

Fabric Screen API, Architectury, Charm & Cloth port

Testing

The following configurations have been tested:

• 1.16.5 G7 FAPI 0.42.0 Architectury 1.32.68 Cloth 4.17.101 Charm 2.3.2
• 1.17.1 H1 FAPI 0.46.1 Architectury 2.10.12 Cloth 5.3.63 Charm 3.3.0
• 1.18.2 H7 FAPI 0.76.0 Architectury 4.11.93 Cloth 6.5.102 Charm 4.4.4
• 1.19.2 I2 FAPI 0.76.0 Architectury 6.5.69 Cloth 8.3.103 Charm 4.4.4
• 1.19.4 I4 FAPI 0.87.0 Architectury 8.2.89 Cloth 10.1.105 Charm N/A
• 1.20.1 I5 FAPI 0.89.0 Architectury 9.1.10 Cloth 11.1.106 Charm N/A

Due to the fact that the original fix consisted of a lot of mixins (6 for Fabric API and 8 for Architectury) and was required across many versions, the port might have some issues.

Fabric Rendering Data Attach API port & (new) Fabric Block View API fix

Fixing the latter somewhat requires porting the former. I tested the following configurations:

• 1.20.1 I5 FAPI 0.89.0
• 1.20.1 I5 FAPI 0.88.1
• 1.20.1 I5 FAPI 0.86.1
• 1.19.4 I4 FAPI 0.87.0
• 1.19.2 I2 FAPI 0.76.0
• 1.18.2 H7 FAPI 0.76.0
• 1.18 H3 FAPI 0.43.1
• 1.17.1 H1 FAPI 0.46.1

Fixed issues

This list is not exhaustive
fixes #1123 fixes #1124 fixes #1127 fixes #1134 fixes #1136 fixes #1139 fixes #1151 fixes #1153 fixes #1156 fixes #1157 fixes #1161 fixes #1166 fixes #1169 fixes #1172 fixes #1177 fixes #1181 fixes #1182 fixes #1183 fixes #1184 fixes #1185 fixes #1186 fixes #1187 fixes #1188 fixes #1189 fixes #1215 fixes #1226 fixes #1222 fixes #1221 fixes #1216 fixes #1217

[XXMA16]

@Chocohead should I continue this PR? The next step would be to port everything from compat, but that would take a while. You hinted at a new system and if it does, at least loosely speaking, the same thing as my system it would be a waste of time continuing this PR.

[Chocohead]

I think it's worth continuing the PR. I would start with converting #1179, given it's the messiest fix needed yet.

What I have locally is not dissimilar, but approaches the problem from the opposite side. I was going to change the presented Minecraft classes that mixins see, then correct it back afterwards. I've thought about changing mod's mixins directly in the past. Beyond the obvious difficulties in reflecting your way to the MixinInfo ClassNodes, by changing the mod code directly you're staking more risk on the mod changing, and blurring the lines of who is responsible if it crashes. Not to say the shim mixins aren't dependent on the mod not changing, but they at least normally make it clear either OptiFabric has a bad mixin, or that the compatibility doesn't work/apply any more. Contrast that to if the game crashes in some of the mod's code you've changed, there's no indication it's been changed by OptiFabric, or indeed changed from the mod's source at all. Your method does have the benefit that the fixes can precisely correct the injector, whilst mine are more restricted to masking out OptiFine changes so that the mixins don't realise they're there.

I think there is probably a balance such that moving locals around (such as from a new parameter) is fine, but adding or changing the logic purely in bytecode is a bit obscure. At least without marking the line numbers as being from OptiFabric. I was also going to deduplicate the fixes further; where certain methods in GameRenderer for example gain a parameter, any mod with a mixin that suffers from that can just request to be fixed rather than each one needing a clone of the fix. The repetitive native of the current shim mixins for this case is definitely a drawback. Although there is little escape to the problem of supporting multiple versions at once requiring different fixes (named clearly or not ;) ).

Hope that is some use to explain my thinking/plans. A blend of the two approaches might well be the best solution of all, but anything which improves on the current approach can't hurt.

[XXMA16]

Contrast that to if the game crashes in some of the mod's code you've changed, there's no indication it's been changed by OptiFabric

This could be handled by an IMixinErrorHandler and MixinFixerExtension.postApply, the former not being able to handle InjectionErrors since they're thrown when the target is called, in a synthetic method that replaces the Injector when the LVT doesn't match. Non-LVT problems usually relate to Optifine taking a vanilla method, making a copy with slight changes (and additional parameters) and then replacing the original's body with a call of the copy; these are taken care of by the IMixinErrorHandler.

I'm also planning to make the crash screen always appear and explicitly state what went wrong, instead of letting the game crash and only provide a stack trace.

I will push tomorrow some stuff because I don't have time to finish today. This includes the following:

• use Loggers instead of STD_OUT
• port Architectury , Fabric Screen API and Cloth
• port Fix Fabric API 0.86.0+ #1179
• make IMixinFixer provide the targets instead of specifying them in addFixer
• display LVT errors on the Title Screen instead of letting the game crash
• use an IMixinErrorHandler to clarify which failing Injectors have been directly changed by Optifabric
• miscellaneous polishing

[TraderTheWolf]

can u add my discord? its .trolf

[XXMA16]

@Chocohead I finally updated the PR description. I think it will take some time before I can complete a user friendly error handling system, but the logging should suffice for now. Merging the PR in order to fix FAPI 0.86.0+ is unlikely to result in any compatibility regression and I can continue working on the system itself and porting stuff from compat in another PR.

[Carel3DS]

When merge?

[TheUnknownCod3r]

Done a bunch of testing related to this, so far Ive noticed it also seems to fix a bunch of issues related to Optifine reflector (or maybe theres something else, but im no longer crashing on load at all), and mod support seems pretty good. I can confirm it works right out of the box on an instance that was iffy before, even with mixin fixes. I also haven't crashed with a Batching Chunks error since switching to this build.

[dima-dencep]

I think it's worth using https://github.com/Sinytra/Adapter to patch mixins...

[XXMA16]

I think it's worth using https://github.com/Sinytra/Adapter to patch mixins...

I might switch to this if Chocohead is ok with it since Adapter has rigor put into it, unlike my lazy implementation. Porting would mean closing this PR and making a new one, but at this moment I don't have the time, nor the motivation to do it. I'm going to start working on it this week, as this PR is already a mess.

[Kavin7k]

Everything seems to work fine, even with about 50 mods, but for some reason, as soon as I equip some resources packs, the game cant launch and gets stuck in the MOJANG Logo screen

[XXMA16]

I started working on another system that wraps the mixins of other mods instead of messing with their annotations directly. I'm currently stuck on the part where I dynamically generate the wrapper mixins since I've been trying to use Fabric ASM's CasualStreamHandler for this task to no avail.

I'll close this PR when I manage to push something on the new branch. Until then, this PR remains a draft.

Regarding Adapter, there's no way of using it since it requires Java 17 or a forking it and making it compatible with Java 8.

[Chocohead]

Did you work out your class generation problems? Fabric ASM has ClassTinkerers#define as an API for adding new classes. It could work for non-class files too, but does automatically add .class onto the end of whatever file path you give it.

Now that bundling Tiny Remapper is necessary, including Adapter is not as complicated (forked for Java 8 or not). I don't think it's really suitable for what we need though. Lazy or not, your implementation here is fairly simple, which Adapter is certainly not. Your implementation is also designed for what OptiFabric need, compared to Adapter which might coincidentally work with some glue code to actually make it work within Fabric Loader.