Skip to content

Fix: Avoid RemapperAdapterFML referencing Remapper class that FMLDeobfuscatingRemapper extends - #236

Open
Meldexun wants to merge 1 commit into
FabricMC:mainfrom
Meldexun:fix-remapper-shadowing
Open

Meldexun wants to merge 1 commit into
FabricMC:mainfrom
Meldexun:fix-remapper-shadowing

Conversation

@Meldexun

Copy link
Copy Markdown

Been thinking about this some more, one thing you could possibly do is bundle and repackage mixin + asm in your mod, this way you can make use of the latets mixin and asm without interferring with other mods and the existing asm version.

Originally posted by @modmuss50 in #153

I tried bundling mixin and asm in a mod for MC 1.12.2 which only uses ASM 5. FMLDeobfuscatingRemapper extends Remapper and is used as such in RemapperAdapterFML. But mixin itself also uses Remapper (e.g. InnerClassInfo).
This makes it impossible to bundle and relocate the Remapper class because that would make mixin expect FMLDeobfuscatingRemapper to extend the relocated Remapper class.

I changed RemapperAdapterFML to use a wrapper Remapper instance that uses method handles to invoke the method of FMLDeobfuscatingRemapper. This completely decouples whatever Remapper class mixin uses from what FMLDeobfuscatingRemapper uses.

(As a small bonus, using method handles is also a bit faster than reflection which was used previously)

@LlamaLad7

Copy link
Copy Markdown
Member

You absolutely cannot shade and repackage Mixin in your mod. All of Mixin's compatibility guarantees rely on there being only one active instance at a time, and violating that will lead to all sorts of issues. The only option on old versions is to use one of the established libraries which provide mixin, such as MixinBooter or EssentialLoader.

@modmuss50

Copy link
Copy Markdown
Member

Yes, I think what I was suggesting was to only repackage ASM and not Mixin.

@Meldexun

Copy link
Copy Markdown
Author

That was poorly worded by me. I'm working on my own mixin loader mod, which bundles mixin and asm. But only asm is relocated.

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.

3 participants