Super-Santa / EssentialAddons

Fabric Carpet extension that adds things from the Spigot plugin Essentials, or other features I think are needed for Minecraft
MIT License
50 stars 7 forks source link

Some mixins are too agressive and can prevent other mods from working #39

Closed altrisi closed 2 years ago

altrisi commented 2 years ago

Multiple mixins use the bad "Inject & Cancel" technique, which not only prevents any other mod from touching that area, but also makes it almost impossible to debug given the other mod mixin has actually been applied, it's just being cancelled before it's reaching there.

Suggestion to use proper Redirects or something else so those conflicts can at least be debugged.

Examples include: https://github.com/Super-Santa/EssentialAddons/blob/e7ae92fb7b882eac8b04bd3073aabdbf0226d9ac/src/main/java/essentialaddons/mixins/essentialCarefulDrop/TntMinecartMixin.java#L24-L26 https://github.com/Super-Santa/EssentialAddons/blob/e7ae92fb7b882eac8b04bd3073aabdbf0226d9ac/src/main/java/essentialaddons/mixins/essentialCarefulDrop/ChestMinecartEntityMixin.java#L24-L26 https://github.com/Super-Santa/EssentialAddons/blob/e7ae92fb7b882eac8b04bd3073aabdbf0226d9ac/src/main/java/essentialaddons/mixins/essentialCarefulDrop/AbstractMinecartEntityMixin.java#L28-L30

senseiwells commented 2 years ago

Yes, I am aware of this conflict, I made a quote 'fix' by just implementing a rule the provides similar functionality, I didn't have your source code to work out a nice solution but you are welcome to pr, or I'll get around to it at some point.

altrisi commented 2 years ago

Well, the suggestion in this issue wouldn't really fix the conflict I believe (with the new rule at least), it's mostly a suggestion to not use Inject & Cancel but a Redirect. Even if it's technically not compatible with other mods mixing into the same place, it'll at least error instead of silently failing, given then it's really hard to debug the conflict (especially when people use lots of mods).

About this specific compat: Assuming you mixed in for carefulDrop, couldn't you just mix after item stack creation and capture the local ItemStack? That way you also wouldn't need to set the name but just capture after it's set. Cancelling/Redirecting the dropStack call. No idea how to add compatibility with the new dropCartItem rule though.

senseiwells commented 2 years ago

The dropCartItem is not set in stone, I just added it because someone asked me about the compat earlier, I have not officially released this so it may be removed in future. About capturing locals, from what I understand capturing locals is slow and generally should be avoided and only should be used if you have to. Redirecting the dropStack method (yarn) would be the best solution I think, this would also be compatible with your mod (I believe)?

altrisi commented 2 years ago

It should, yes, I only redirect the ItemStack instantiation:

https://github.com/altrisi/DropFullCarts/blob/e45364907e54cd366390adc3ac8865f5a8bb6516/src/main/java/altrisi/mods/dropfullcarts/mixin/AbstractMinecartMixin.java#L17-L19

altrisi commented 2 years ago

About local capture performance: From a quick look it shouldn't be slow, it may take longer at initialization when Mixin is trying to find the locals/change their scope in the bytecode, but after the bytecode is finished, the modified code is just passing the local to the method without anything else. Maybe if there's many locals to capture there could be some overhead from passing them, idk.

Example decompiled from carpet extra DispenserBlock_getCustomBehaviorMixin: imagen

altrisi commented 2 years ago

There's only one of this type of mixin remaining, closing this. https://github.com/Super-Santa/EssentialAddons/blob/d0f0141da17c38544570e8d83332fcf4445a3cb6/src/main/java/essentialaddons/mixins/essentialCarefulDrop/StorageMinecartEntityMixin.java#L34-L36