Crash due to a @Redirect conflict

[khankul]

I assume removing the telemetry button does not fare well with other mods that mess with it, such as sc-client here.

[09:41:12] [Render thread/WARN]: @Redirect conflict. 
Skipping sc-client.mixins.json:OptionsScreenMixin 
from mod sc-client->@Redirect::add10(Lnet/minecraft/class_7845$class_7939;Lnet/minecraft/class_8021;)Lnet/minecraft/class_8021; with priority 1000, 
already redirected by no-telemetry.mixins.json:OptionsScreenMixin 
from mod no-telemetry->@Redirect::removeTelemetryButton(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; with priority 1000
[...]
*crash*

Minecraft version: 1.20.1, mod version: 1.8.0

[kb-1000]

I was not able to find that mod, it appears to be closed source and non-public, I have no intentions of fixing conflicts with mods that effectively don't exist. You also didn't upload any logs. But let's go on... I chose the most compatible way to remove the button: Redirecting the first add call in a @Slice from the first (and only) occurence of the Telemetry button's text to the method's end to a no-op method. This expresses the intent perfectly, and the only way to conflict with it is either a badly written mod redirecting all add calls (there's not much I could do about that other than going down to raw ASM, which is not planned), or another mod messing specifically with the Telemetry button (so either it duplicates this mod's functionality, in which case a conflict is correct, or it messes with the button to add its own telemetry to the screen, which it shouldn't)

[khankul]

You also didn't upload any logs.

Sorry, I believed all the other parts of the log to be irrelevant since they're just domino crashes of other mods. You're right though, it's better to provide the full picture. Here's logs. Now, about sc-client... This mod is part of the SwitchCraft 3 modpack, and it's currently closed source. This is how the settings screen looks like in this modpack: sc-client indeed seems to "mess" with the button but doesn't affect anything at the telemetry screen. It doesn't seem to touch the telemetry mechanism itself either. Now, the SC developer I've talked to about this issue said that they can take the responsibility for this edge case (and i.e. reduce their mixin's priority if that helps), but... what if it happens again with another "invasive" modpack? I'd like No Telemetry to take a few additional steps towards compatibility, if possible. Please share your thoughts.

[kb-1000]

So first up to make it clear: I will not hardcode anything specifically for sc-client, since that might just end up in a list of special cases, though I'm open for improving compatibility in another way. Regarding other invasive modpacks... well, I'd fundamentally recommend not doing something like this and in the 1.5 years of this mixin's existence, this is the first report, but here we are I guess. I don't touch the telemetry options screen either, I only hide the button. So, we've got a few options here: I could make the redirect not required to be applied, which would have the disadvantage that the button is still shown in such cases, and that potential future vanilla changes would have the same effect (for that reason, I like to keep such errors fatal, because that both means I'm more likely to notice it, and makes it more likely for me to get a useful bug report if not). Or some sort of API could be introduced that both communicates to me that the telemetry button is custom and I should disable the mixin, and to the other mod that it shouldn't show that custom button. But the other mod has to adopt this API, and it has to happen in or before prelaunch due to how mixin works, and probably shouldn't be specific to either mod. Or you could just check whether no telemetry is loaded and skip this mixin if so, but that's not exactly ideal either and doesn't solve it for other mods.

[khankul]

I think I'll close the issue for now because the SC developer has agreed to fix this on their side.

[Lemmmy]

Hi kb-1000, I'm really sorry to take up your precious time with this silly issue. Like you mentioned, this is merely the consequences of both of us using @Redirect. It's absolutely a problem on SwitchCraft's end. I don't think you need to do anything to 'ensure compatibility' with invasive modpacks like ours - invasive modpacks take responsibility for their own mods and own compatibility problems by choosing to be invasive in the first place.

Just for transparency, here's what our mixin looks like: https://gist.github.com/Lemmmy/0dcdd1a8ba595f0db10a381b809a6a19

I'm sure we can address this on our end, either by adjusting the priority or conditionally applying the mixin. Thank you for trying to help out anyway!