Commit abd5498 causes mixin injection failure in prod with some mixins

Justsnoopy30 commented 3 years ago

After this commit https://github.com/FabricMC/fabric-mixin-compile-extensions/commit/abd549827f576c602c7530e3c04ac2b15a3431a3, some mixins such as the below mixin fail to inject outside of a dev environment. This MCE update was included in loom 0.7.28 and is now breaking mods with certain mixins using loom 0.7.

Mixin:

    @Inject(method = "Lnet/minecraft/client/render/WorldRenderer;renderSky(Lnet/minecraft/client/util/math/MatrixStack;F)V", at = @At(value = "INVOKE", target = "Lnet/minecraft/client/world/ClientWorld;getSkyAngle(F)F"),
        slice = @Slice(from = @At(value = "FIELD", target = "Lnet/minecraft/client/util/math/Vector3f;POSITIVE_Y:Lnet/minecraft/client/util/math/Vector3f;")))
    private void iris$renderSky$tiltSun(MatrixStack matrices, float tickDelta, CallbackInfo callback) {
        matrices.multiply(Vector3f.POSITIVE_Z.getDegreesQuaternion(pipeline.getSunPathRotation()));
    }

Crash Report: crash-2021-04-27_09.57.31-client.txt

The mixin injects successfully on the previous loom version 0.7.27 with MCE 0.4.1.

sfPlayer1 commented 3 years ago

Please provide the refmaps for old+new

Justsnoopy30 commented 3 years ago

Old: iris-refmap.json.txt

New: iris-refmap.json.txt

(attached as .json.txt cause github doesn't support .json)

modmuss50 commented 3 years ago

Uhh, I will revert this for now

modmuss50 commented 3 years ago

The change has been reverted in loom.

Justsnoopy30 commented 3 years ago

Should I close this issue or leave it open (as the change is still in this repo)?

sfPlayer1 commented 3 years ago

I think I have a proper fix, checking atm

sfPlayer1 commented 3 years ago

https://github.com/FabricMC/fabric-mixin-compile-extensions/pull/10 should fix it

sfPlayer1 commented 3 years ago

It is merged now, please close after confirming.

Pyrofab commented 3 years ago

Loom 0.7.29 works fine, loom 0.7.30 has the same issue. Tested with this mixin:

    @Inject(
        method = "fall",
        at = @At(value = "FIELD", opcode = Opcodes.GETFIELD, target = "Lnet/minecraft/entity/LivingEntity;fallDistance:F", ordinal = 0),
        cancellable = true
    )
    private void onFall(double fallY, boolean onGround, BlockState floorBlock, BlockPos floorPos, CallbackInfo info) {

(https://github.com/Ladysnake/Requiem/blob/8c9a480fcf55dbb9d3a2666cef0bb683722cc2c6/src/main/java/ladysnake/requiem/mixin/common/possession/possessor/PossessorLivingEntityMixin.java#L120-L137)

sfPlayer1 commented 3 years ago

This is a different issue, since it's a field access using a subclass owner with the mappings only translating Entity.fallDistance, not EntityLiving.fallDistance.

Not sure why/if this ever worked since it's what the commented out code in MixinMappingProviderTiny.getFieldMapping would have to deal with.

sfPlayer1 commented 3 years ago

I think Mixin disables its super lookup fallback path with the compile ext change yielding a partial result (treating it as if the field name was not obfuscated, but the owner is)

sfPlayer1 commented 3 years ago

Please try https://github.com/FabricMC/fabric-mixin-compile-extensions/pull/11 - Requiem doesn't build as it is in the repo.

sfPlayer1 commented 3 years ago

Build 0.4.4 is about to release, you can quite easily evaluate it by employing this trick: https://github.com/FabricMC/fabric/commit/33dd200e7395d43b2bfa96d4779ecda95755f150

FabricMC / fabric-mixin-compile-extensions

Commit abd5498 causes mixin injection failure in prod with some mixins #9