Potential Mixin incompatibility in other mods

[border999]

Potential Mixin incompatibility in other mods

[border999]

YerovaArusu/botanicpledge#2 reporting to both suspected mods author's

[AnonymousHacker1279]

This is a strange report. I've never seen a failure from my combat rules mixin till now. The other mod doesn't directly touch that class from what I can tell either.

I'll do some testing in my own environment and see if I can reproduce it. I recently used a pack of about 150 mods on 1.20.1 including IW and never ran into an issue like this.

[border999]

it happened a couple seconds after what I believe to be an Apotheosis mob spawned, got the little message the appeared on my screen just before I got the boot from the world.

[border999]

https://gist.github.com/border999/d04a676b04478ecbf8a090f17f3302f6

[border999]

Here's the crash I got when trying to get back into the world. Looks essentially the same to me but it's somewhere you can pick through it a little easier.

[AnonymousHacker1279]

Spun up an environment on Forge 47.2.21 with:

• Botania
• Botanic Pledge
• Curios
• Immersive Weapons
• Patchouli
• TerraBlender

All are at the latest version. Spawning a few of my mobs and vanilla mobs, then hitting them, seems fine. No crashes yet.

Adding Apotheosis and its dependencies:

• ApothicAttributes
• AttributeFix
• Placebo

Still no crash, game loads as expected and attacking mobs seems to work. I'm not sure what's causing the failure.

Both logs say that my combat mixin has an invalid descriptor, which is very strange. Especially considering it works in all of my test environments and in other packs. If you remove Botanic Pledge from your pack, does it still crash?

[border999]

The crash at my best guess seems to have been triggered just after an Apotheosis named mob spawned. Perhaps some kind of magic attack was sent at the mob in the crash log? I remember an issue with Grimoire of Gaia 4 a year or two ago where when one of their mobs attacked the player and dealt magic damage it caused the game to crash.

[border999]

Removing BotanicPledge let me get back into the world. Now I'm gonna readd it and see if the crash returns.

[AnonymousHacker1279]

I'll do more testing and see if I can find anything. Perhaps the dev of the other mod can help to figure out what's going on.

[YerovaArusu]

I am currently trying my best myself.
But i cant seem to find the issue...
I testet it in a custom bigger modpack... but there were absolutely no problems

[border999]

got back into the world after readding Botanic Pledge. Perhaps this is some odd one off crash or something that is just hard to replicate

[AnonymousHacker1279]

Definitely something that's hard to replicate. I'll keep the issue open for a while in case anything else arises that could point to the issue.

[border999]

ummm.... I got hit by a ghost mob. Then this happened. https://gist.github.com/border999/65da998368a365697c7d8448c96b9d2f

[AnonymousHacker1279]

Something is going on that is breaking the mixin application behavior. Basically, the mixin in question that is being destroyed is my CombatRulesMixin. The purpose is to unlock the max vanilla armor cap of 20 points.

In the vanilla class CombatRules, there is this method which calculates damage after armor absorption:

public static float getDamageAfterAbsorb(float pDamage, float pTotalArmor, float pToughnessAttribute) {
    float f = 2.0F + pToughnessAttribute / 4.0F;
    float f1 = Mth.clamp(pTotalArmor - pDamage / f, pTotalArmor * 0.2F, 20.0F);
    return pDamage * (1.0F - f1 / 25.0F);
}

The problem here is that vanilla uses a clamp function to cap armor protection to 20 points. So, I use a mixin to override that:

@Inject(method = "getDamageAfterAbsorb(FFF)F", at = @At("RETURN"), cancellable = true, locals = CAPTURE_FAILSOFT)
private static void getDamageAfterAbsorb(float damage, float totalArmor, float toughnessAttribute, CallbackInfoReturnable<Float> ci, float f) {
    float clampedArmorProtection = (float) Mth.clamp(totalArmor - damage / f, totalArmor * 0.2F, ImmersiveWeapons.COMMON_CONFIG.maxArmorProtection().get());
    float newDamage = damage * (1.0F - clampedArmorProtection / 25.0F);
    ci.setReturnValue(newDamage);
}

What this basically says is "use the vanilla calculation (f) to determine how many armor points the player has to use in defense, then calculate how much damage they still take". I am effectively replacing the cap of 20 points with a user-configurable value.

You'll notice the method signatures are technically different. The vanilla method has three float parameters, while mine has those plus a CallbackInfoReturnable<Float> and an extra float. Mixin automatically expects the CIR to exist at the end. However, when locals = CAPTURE_FAILSOFT is set in the injection annotation, it will "capture" the local variable of the same name in the vanilla method, adding it as a parameter to the end. This means that the f variable in the vanilla method (which is the armor toughness calculation) gets passed to my handler.

The crash in the log is related to the method signatures being different from what's expected. Normally, without capturing locals, this signature looks like:

(FFFLorg/spongepowered/asm/mixin/injection/callback/CallbackInfoReturnable;)V

That is, the parameters being float, float, float, CallbackInfoReturnable and the return being void.

Now, with captured locals, it is:

(FFFLorg/spongepowered/asm/mixin/injection/callback/CallbackInfoReturnable;F)V

Notice the extra float after the CIR. Mixin is automatically supposed to handle this (considering local capture is a mixin feature...). For some reason in your crashes, it is not, which is why I am so confused as to what's happening.

My only guess is another one of your mods is either mixin'ing into the same method in a hacky way or overwriting the class entirely. Or maybe it's a mixin-specific bug?

[border999]

BadOptimizations-2.1.0
modernfix-forge-5.15.0+mc1.20.1
saturn-mc1.20.1-0.1.2
canary-mc1.20.1-0.3.3
KryptonReforged-0.2.3
redirector-5.0.0
These are the mods I can find on my mod list that I think could reasonable be messing with the mixin. Most of them probably don't even go near your mixin but you never know.

[AnonymousHacker1279]

A quick search through the source repositories of those mods doesn't seem to bring up any references to the CombatRules class.

Try adding -Dmixin.debug.verbose=true -Dmixin.debug.export=true to your launch arguments and run the game. Then, go into your profile folder and look under .mixin.out\class\net\minecraft\world\damagesource. There should be a CombatRules.class file created, which contains any mixin changes applied to the file and its source. Upload the file itself, not a text version because it'll be compiled code (and unreadable if you put it on something like Gist). Hopefully this will point out other mods that modify the same thing.

If you can recreate another crash with the updated arguments, upload a new log too as it might provide extra mixin-related information.

[border999]

I apparently ALREADY had these arguments applied. Here's the two files in that folder.
damagesource.zip
had to put it as a zip file because github wouldn't accept the .class files

[AnonymousHacker1279]

There isn't a CombatRules.class inside that zip. It might not be generated until you try to hit something - can you try opening the game and attacking an entity, then seeing if it makes one? I figured it would have dumped all the classes at launch but it doesn't appear that was the case.

[border999]

I did in fact attack a mob before crashing.

[AnonymousHacker1279]

The only thing I can suggest doing now is a binary search if you haven't already done one. The binary search is simple:

1. Remove half of the mods and put them aside in a different folder.
2. Open the game, try hitting things, etc.
3. Does it still crash? -> Repeat from step one, if not, swap to the other half and try again.
4. Repeat the process until only the problematic mod(s) are found.

It may be time consuming but technically speaking it can be done in as little as eight launches.

Another thing you could try is using NeoForge instead of classic Forge. IW should be compatible with both, but I haven't kept up with classic Forge in a while to know if they changed anything that would cause mixins to suddenly break here (very unlikely that it would happen but still).

Worst case scenario I'll just make a build of IW without that mixin and upload it here so you can still use it. Of course, you'll be limited to a max of 20 armor points then, so things like Tesla Armor won't be as effective. That really is a bandaid fix but I'm not at all sure what is causing this to happen.

[border999]

at this point If I crash again I will just hand over the log and debug log as well so you get to see everything.

[AnonymousHacker1279]

That may help. It might let me see what other mods are trying to modify that method. Unfortunately, on 1.20.1 method names are obfuscated so it might take me a bit to read through it.

[border999]

https://gist.github.com/border999/f7a9b3053b2653a3d5402363a9b6b55b -crash
https://gist.github.com/border999/9eb374cc377b0a76a770427b49a7a026 -debug
https://gist.github.com/border999/e2ec8126b86c927ca5e0bd4ab6d180fa -latest

[AnonymousHacker1279]

I may have found the issue. Looks like ApothicAttributes is also mixin'ing into the same methods. It seems to be using overwrites which are known to cause compatibility issues regarding other mixins. It is very well possible that the overwritten method makes it impossible for my mixin to be applied as an injection.

Can you remove ApothicAttributes (and by extension, Apotheosis) and see if the crash still occurs?

[border999]

wait one, let me open up an issue thread with them and reference here.

[AnonymousHacker1279]

I'd test and see if it solves the crash first before making a report.

[border999]

The problem with that is that the crash is at random and can take a large amount of time to occur in normal gameplay. I wouldn't be able to know if I haven't crashed because I'm getting lucky or because the issue is resolved.

[AnonymousHacker1279]

Fair enough. I still haven't been able to reproduce it in my environment, but I've only messed around in it for about 30 minutes.

[Shadows-of-Fire]

The behavior being seen here is unusual. The use of LocalCapture.CAPTURE_FAILSOFT should just cause the entire inject to be skipped if the locals differ (such as when my overwrite is applied).

In any case, it is good that the error does present in some fashion, as these two mixins can simply never be compatible, as Apoth is providing a full rewrite to those calculations.

You could change your mixin to the following, which will permit apoth to take over when it is present, but shouldn't crash if it is present:

@Inject(method = "getDamageAfterAbsorb(FFF)F", at = @At("RETURN"), cancellable = true)
private static void getDamageAfterAbsorb(float damage, float totalArmor, float toughnessAttribute, CallbackInfoReturnable<Float> ci) {
    if (ModList.get().isLoaded("attributeslib")) {
        return;
    }
    float f = 2.0F + toughnessAttribute / 4.0F;
    float clampedArmorProtection = (float) Mth.clamp(totalArmor - damage / f, totalArmor * 0.2F, CommonConfig.maxArmorProtection);
    float newDamage = damage * (1.0F - clampedArmorProtection / 25.0F);
    ci.setReturnValue(newDamage);
}

[AnonymousHacker1279]

Yeah, that's what was really confusing me when looking at the method signatures. The docs for LocalCapture.CAPTURE_FAILSOFT say:

Capture locals. If the calculated locals are different from the expected values, log a warning and skip this injection.

I can add a check for that mod ID. But I'm still curious why Mixin would be crashing here instead of skipping the injection. Should this be reported to Mixin?

[SiverDX]

your mixin config:

	"injectors": {
		"defaultRequire": 1
	},

isn't crashing if an inject cannot be applied / gets skipped the expected behavior here?

[border999]

It crashes in the sense that the world closes, and I’m booted back to main menu. However, if I didn’t have at least one or two mods that prevent crashes from completely shutting down the game I would expect it would kill the game entirely. If you weren’t actually talking to me this is moot to mention and may be moot regardless

[AnonymousHacker1279]

your mixin config:

	"injectors": {
		"defaultRequire": 1
	},

isn't crashing if an inject cannot be applied / gets skipped the expected behavior here?

I was under the impression LocalCapture.CAPTURE_FAILSOFT would overrule this. I suppose I could use a second mixin config that specifies defaultRequire as 0 for that particular mixin.

To clarify, being skipped would be the expected behavior but crashing is not.

[SiverDX]

you shouldnt need a whole new config for this, the inject itself should have a require (or sth. similar to overwrite the config) as well

[AnonymousHacker1279]

Ah, that's true. Specifying require = 0 on the injection annotation should do the trick, right? Though looking at the docs for require, it seems like by default (being -1), that it wouldn't be required. Perhaps that's just the way I interpret it though.

[SiverDX]

in your config you set a global default require of 1

[AnonymousHacker1279]

So then I completely misinterpreted what defaultRequire was. I thought it was related to the required entry at the top of the mixin config. That makes a lot more sense now.

So next question, should I even bother adding a mod ID check like Shadows mentioned into the mixin? If I mark it as non-required then nothing else should need to be done.

[Shadows-of-Fire]

If your only change is adding require = 0 then you shouldn't need the modid check as your inject will just fail in the presence of apoth.

[AnonymousHacker1279]

Alright that's what I figured. Time to make a new release 👍

[SiverDX]

"
required defines whether the mixin set is required or not. If a single mixin failing to apply should be considered a failure state for the entire game, then the required flag should be set to true.
"

not sure but you may need to remove the required and set/keep the default require to 1 (and override said default for that one inject)

[AnonymousHacker1279]

I can remove the required entry. The other mixins are important but not enough to where the game should crash if they fail. I'll override the default for this particular inject.

While I'm at it I'm also going to remove the locals = CAPTURE_FAILSOFT entirely and use MixinExtra's @Local in its place. After a Discord discussion earlier I was informed that the local capture is very brittle and @Local is a better alternative. I could just re-calculate the armor toughness modifier instead but in the event another mod was specifically modifying that value beforehand, using the previous calculation would be better.

[YerovaArusu]

Is there something i should change on my end as well?
Currently i am not very experienced in using mixins...

[AnonymousHacker1279]

Is there something i should change on my end as well? Currently i am not very experienced in using mixins...

@YerovaArusu you should be fine. The incompatibility lies between IW and Apotheosis (specifically, ApothicAttributes).

@border999 I just released v1.27.7 for MC 1.20.1. It should fix the crashes.

I'll have a MC 1.20.4 build out soon™️