Replay Mod (Fabric & Forge)

Replay Mod (Fabric & Forge)

787k Downloads

Incompatibility with Amecs

Boobies opened this issue ยท 4 comments

commented

While this is really a problem with how Amecs does things, I am opening this here so potential solutions could be discussed. Before I get into details, one strange things I've noticed is that this only occurs as of Fabric Loader 0.13.0 and with earlier versions, the problem does not manifest. I'm not sure what changed.

Amecs and ReplayMod both do an injection on onMouseScroll[1][2]. That in itself should be fine but earlier on, Amecs does some ugly bytecode manipulation in order to get the return value of a method in a local variable[3] after which it does a normal injection. This seems to mess up the LVT. In the past, Amecs would do a redirect to capture that value but this turned out to be problematic cause other mods did the same.

Is this stuff good enough for what ReplayMod needs to do?[4] It's not enough for Amecs. Or do you have some suggestions?

[1] https://github.com/Siphalor/amecs-api/blob/1.18/src/main/java/de/siphalor/amecs/impl/mixin/MixinMouse.java
[2] https://github.com/ReplayMod/ReplayMod/blob/develop/src/main/java/com/replaymod/recording/mixin/MixinMouseHelper.java
[3] https://github.com/Siphalor/amecs-api/blob/1.15/src/main/java/de/siphalor/amecs/impl/mixin/AmecsAPIMixinConfig.java
[4] https://github.com/FabricMC/fabric/blob/1.18.2/fabric-screen-api-v1/src/main/java/net/fabricmc/fabric/mixin/screen/MouseMixin.java

commented

You don't need to put values into local variables to use them.
The Java Virtual Machine is a stack machine. Meaning you don't specify that a method is called with values stored in local variable a and b and that it puts the result into c, you instead just call the method and it takes its arguments from the top of the stack and puts its result onto the stack (so if you want to call it with values stored in a and b, you have to use LOAD to fetch it from the variable and put it onto the stack before the call. and to put it into c, you have to explicitly STORE the top stack value into c).
Now, if you just leave the return value on the stack, it's already in the right place to serve as an argument to the next function.
Put into pseudo-Java code (as far as bytecode is concerned, the this reference acts sort of like a 0th argument):

// Vanilla
return mouseScrolled(this, amount, x, y);
// Current Amecs
handled = mouseScrolled(this, amount, x, y);
mouseScrolled_onMouseScrolled(this, ..., handled); // this line gets added by mixin
return handled;
// The simpler and more compatible way
return mouseScrolled_onMouseScrolled(this, mouseScrolled(this, amount, x, y));

To do that via ASM (again pseudo-code):

// Current Stack: ..., Screen, double, double, double
invoke mouseScrolled // just for reference; this is already part of vanilla
// Current Stack: ..., boolean
load 0 // loads `this` onto the stack
// Current Stack: ..., boolean, this
swap // swaps the top two stack entries so they're in the right order for our method call
// Current Stack: ..., this, boolean
invoke mouseScrolled_onMouseScrolled // calls our handler method, should have signature `boolean mouseScrolled_onMouseScrolled(boolean)`
// Current Stack: ..., boolean // just like before we came in
commented

Unless I'm misreading acmes' code, it seems like they are intentionally modifying the LVT, so they can then use the modified LVT from their mixin, I'm not particularly surprised it blew up, more so that it only did with 0.13.0.
It seems to me that the most straight forward way to fix this incompatibility is to not modify the LVT and instead just call your handler method directly with the item on the stack.

commented

The problem is that vanilla code does not record the return value anywhere (it's simply ignored) and that's why Amecs tries to store it in handled so it can have access to this return value. So I'm not sure what there is to put on the stack.

commented

Thanks.