PG85 / OpenTerrainGenerator

Minecraft Terrain Generator for Bukkit/Spigot/Forge
MIT License
199 stars 83 forks source link

OTGHooks in asm excluded package #220

Closed sfPlayer1 closed 5 years ago

sfPlayer1 commented 5 years ago

The class should be outside the package excluded by the annotation on your loading plugin, otherwise it won't get remapped from SRG to MCP names when running in-dev with a release jar. In general nothing in a coremod or in an asm package should reference a minecraft class.

PG85 commented 5 years ago

Hey sfPlayer1, Thanks for the tip, I'm not sure I understand though. When publishing, all OTG asm classes are split from the main project and published as a seperate jar via Gradle. The coremod jar is then included in the main jar and is unpacked by Forge at app start. This seems to work fine in both dev and production. Am I misunderstanding something here? Thanks!

sfPlayer1 commented 5 years ago

By package I mean the Java package as used for the prefix in full class names (some.package.MyClass), not the packaging of the coremod, which doesn't affect the problem.

The class com.pg85.otg.forge.asm.OTGHooks is in the package com.pg85.otg.forge.asm, the loading plugin has @TransformerExclusions(value = { "com.pg85.otg.forge.asm" }). This means that the class OTGHooks gets excluded from being processed by transformers, including the SRG->MCP renaming transformer.

OTGHooks references MC members directly, in your production jar they carry SRG names (func.. or field..). It is common practice to use such a production jar in-dev, relying on FML's runtime deobfuscation transformer to translating the references back to MCP names - if the class isn't excluded. Your specific setup prevents runtime deobfuscation from working.

In your case you want to move com.pg85.otg.forge.asm.OTGHooks to com.pg85.otg.forge.OTGHooks or another package outside the excludes.

It probably shouldn't even be part of the coremod jar, having a direct MC reference in a coremod defeats the main purpose of splitting it off in the first place. I'm fairly certain Forge didn't think about this case when coming up with the coremod separation policy.

PG85 commented 5 years ago

Hey @sfPlayer1, thanks for clarifying! Moving the class out of the package because of the annotation makes sense, I hadn't tried running the production jar in-dev. As for using MC references, it seems to me the only obvious way. I assume you mean referencing MC code defeats the purpose because the MC classes I'm using might be exactly what I or someone else are trying to coremod? Do you have any suggestions as to how I could fix that? I can only imagine the mess of asm code I'd have to write in order to achieve the same results, compared to just injecting a few methods :/.

sfPlayer1 commented 5 years ago

Only code directly referencing Minecraft is problematic, proper transformer code doesn't do this. It only uses the string representation. The problem is that if a class A directly references another class B, e.g. in a method's signature, loading class A subsequently loads class B as well within the same context. This context may not be appropriate because it is from the wrong class loader, too early for others to register their edits or triggers infinite load recursion.

If I say "references Minecraft", I don't mean launchwrapper, most libraries or anything in FML's relauncher package. They are fine and partially required to be used. This means that using IClassTransformer is fine/required, it is the entry point for doing bytecode edits after all. You just can't use classes like net.minecraft.world.World and the likes.

Type.getInternalName(OTGHooks.class) was never correct, it may class load OTGHooks at the wrong time. This should use the string representation "some/package/containing/OTGHooks" (=qualified name with / instead of . for package part separators and $ instead of . for nested class separators) directly, instead of going through Type.getInternalName which yields the same string.

Resulting bytecode can access whatever it wants, it is not constrained in any way, only the transforming code needs to be carefully designed (contrary to the transformed code).

The trick is to use strings to describe MC or other non-asm references when emitting/modifying bytecode and moving code accessing outside the coremod, like you mostly did with OTGHooks. You don't have to do anything substantially different, it was really just an unfortunate package choice for OTGHooks and referencing OTGHooks directly instead of with a string.

PG85 commented 5 years ago

*This should explain coremod crashes with Sponge and some non-vanilla launchers.

PG85 commented 5 years ago

For v8_r2: OTGHooks is no longer excluded from transformations, any references to OTGHooks from the OTG-Core classtransformer use the string representation. String references to mc classes in the classtransformer have had their obfuscated representations added, which should hopefully fix problems for Sponge.

PG85 commented 5 years ago

Hey @sfPlayer1, question, could you tell me what's going on with obfuscated mc class names in class transformers?

For instance:

You said earlier that "class names stay the same with mcp<->srg", so what obfuscation am I seeing here?

sfPlayer1 commented 5 years ago

These names are in what the mcp project calls the "notch" namespace, the names Mojang's obfuscator used when publishing the jars for a specific MC version/release.

anh is the correct name for the Biome class, no idea why you are seeing something else. The only method returning anf is net.minecraft.world.NextTickListEntry.setScheduledTime. It shouldn't appear in this context, your observation is likely off.

sfPlayer1 commented 5 years ago

You can change the sorting order of your transformer to be after the deobfuscation transformer (translates notch->srg, priority 1000) to only deal with mcp (in-dev) and src (runtime) names.

PG85 commented 5 years ago

Thanks that's good to know, will take a look at the sorting order. You're right about anf/anh, I've double-checked with some logging and it reports anh, not sure what I was doing wrong earlier.