[Incompatibility]: Not compatible with Do a Barrel Roll due to invasive handling of mouse movement
enjarai opened this issue ยท 5 comments
Have you checked if a similar issue is already reported by someone else?
- I checked there are no similar issues have been reported.
Have you read the support policy?
- I read it and I accept the policy.
Are you using the latest Epic Fight and recommended Forge version?
- I checked I'm using latest Epic Fight and recommended Forge version.
Is this issue related to mod incompatibility?
- This is a mod compatibility issue and I'm aware of the problem.
The mod
https://github.com/enjarai/do-a-barrel-roll
Minecraft Version
1.18.2
What happened?
The issue
I recently received an issue report about this on my mod, Do a Barrel Roll. When used with the Epic Fight mod installed, there seems to be a problem with mouse movement not being properly intercepted when in elytra flight, which is an essential feature of my mod. This causes significant issues as seen in the attached video included with the aforementioned issue report:
2023-02-19.22-25-44.mp4
The probable cause
I suspect this problem is caused by Epic Fight essentially bypassing the vanilla MouseHandler#onMove method, which is where my mod has a mixin to capture and modify mouse movement.
As this doesn't seem like something I can fix or work around from my side, could you look into other, less invasive options for handling mouse movement on your side?
My code uses Yarn out of personal preference, I don't have branches for other mappings. I usually use a mappings viewer when browsing differently mapped code.
Is this the mixin you mention? https://github.com/Yesssssman/epicfightmod/blob/1.18.2/src/main/java/yesman/epicfight/mixin/MixinMouseHandler.java
This definitely doesn't seem like an ideal implementation, copying an entire method into your handler is very prone to causing compatibility issues. That being said, this would probably not solve the issue at hand, as you'd most likely end up having to use a @Redirect in the exact same location I'm using.
Aside from using MixinExtras, which could easily solve this issue, but is a pain to use on Forge, I don't see any simple solution for this conflict.
I might be able to work around the redirect with a combination of other injects on my end, but that'd still require your side to switch to a more limited injection scope. This could be a redirect, but another solution would definitely be preferable. I'm not familiar with your code, any ideas on that front?
I didn't have any other choice because I wanted to insert the checking code in the middle of the original code. I think I can check the whole condition in the first of the code and cancel the rest when it's true. it's very inefficient and it still can conflict with your mixin. But I think it's gonna happen very rarely because it's very hard to be hitten by melee attacks from the mobs while player is flying with elytra. What do you think, can it be a valid solution?
Mixin is designed to inject in the middle of vanilla code, so you definitely don't need to copy the whole function. Unless I'm missing something, you should absolutely be able to get your code working with just a @Redirect at Lnet/minecraft/client/player/LocalPlayer;turn(DD)V.
The problem with redirect mixins is that two mods using them on the same instruction will conflict, luckily https://github.com/LlamaLad7/MixinExtras exists to provide alternatives to do similar things while being compatible. I mentioned before that its a pain to get working on Forge, but I've done some digging on the subject, and it shouldn't actually be that hard.
So, to get compat working in an optimal way, we have two things we need to do:
- Your mod should switch to a more limited inject, that probably being a
@Redirector a MixinExtras@WrapWithCondition - One or both of our mods has to use a MixinExtras injector instead of using a redirect.
That logically boils down to three options:
- Only you make changes to your mod, depend on MixinExtras with JiJ, mods are compatible.
- We both make changes, you switch to a redirect, saving you a dependency, and only I switch to MixinExtras, also making us compatible.
- We both make changes and switch to MixinExtras, this is probably the optimal solution if we want to keep compatibility with other mods in mind, as we can be very sure there wont be conflicts in the future on this front.
I'd personally prefer option 1, not only because it'd save me some work, but primarily because I've been having significant issues getting newer versions of my mod working on Forge 1.18.2, and I was just about to drop support for that version when this issue came up.
I should mention that MixinExtras works completely independent of mc version and modloader, so it wont hamper updates to new versions. Do let me know what you think.
Well, I'm also using mixin cause it's necessary to fix the player's rotation while turning the camera in the third person. I tried to inspect your code but I was confused because it still uses the old MCP name. Do you have any other branch that consists of an official mapping name?