kyrptonaught / EnchantedToolTips

MIT License
11 stars 7 forks source link

Question: Does making the EnchantedBookItemMixin cancelable prevent other mods from adding their own tooltips? #6

Closed mainrs closed 4 years ago

mainrs commented 4 years ago

Just from taking a look at the code and my understanding of ASM: You inject your code right before the first opcode, meaning that your logic gets executed first. You then check for any enchantments on that item and generate the tool tip. First the enchantment name (like vanilla does), then the other fields that are indented by some spaces. That is done in your helper class here. After the call to that class, you cancel the operation here.

If another mod would patch EnchantedBookItem too, for example, to add a line to indicate how many levels are needed to apply this enchantment, the tooltip would never get added, because your mixin would trigger a return before any other opcode gets executed.

Note: I don't know of any mod that does this currently on Fabric. I just got into Fabric and wanted to create a mod to add enchantment descriptions to enchanted books so I get a feel on how modding works in Fabric. I later found your mod and was curious on how you solved it. In comparison, I injected my code right before return using TAIL and append my lines. The tool tip doesn't look as nice as yours, that's for sure. But it should be, compared to yours, compatible with other mods that modify the EnchantedBookItem tool tip.

Maybe I am wrong here. If that is the case, I would really appreciate if you could point out my faulty assumptions and correct them :)

Cheers, Sven

mainrs commented 4 years ago

I created a dummy mod who's sole purpose is to add a tooltip to enchanted book items and indeed, those do not get applied. Your tolltip does overwrite mine. If I do, however, create one with a higher priority, mine gets called first.

There is currently no way to inject tooltips next to yours though. Going back to my example, adding the needed XP right after "applies to" is not possible because your mixin does cancel the callbackinfo.

kyrptonaught commented 4 years ago

Hmm I did indeed overlook this when I was creating the mod, and up until now nobody has made any mods that touch these tooltips so the issue has gone unnoticed. I was using the cancel to prevent vanilla from adding its information about the enchant to the tooltip, perhaps I can use a Redirect instead. I’ll take a look at it now.

Thanks for bringing this to my attention

mainrs commented 4 years ago

Just reading through the documentation of @Redirect it doesn't seem that this would solve the issue. Wouldn't that cause the injected code to replace certain calls instead? As far as I understood, using @Redirect allows you to replace INVOKE opcodes. Would you then try to replace the vanilla calls that add the enchantment description only?

I kind of solved it by iterating over each existing tooltip entry, checking if it is a vanilla enchantment and if that is the case, inserting additional information right after that. I've set the Mixin priority to 1 so mine gets called last. It is injected using @At("TAIL"). Definitely not the most elegant way to do it. I am, however, sure that there are better solutions that Mixin allows but I do not have the knowledge for those at this time.

The issue mainly came after thinking about Thaumcraft. That mod adds the aspects to each item as well if the user holds the shift key.

I am not sure if some kind of API would solve the issue of multiple mods adding tooltips to items. So each new line could be seen as an implementation of that API and your/another mod renders them onto specific items. That way people could add new lines by just writing an "extension" of some kind and not really caring about the rendering part (or ordering, as this could be extracted to the a mod itself).

Not sure how this fits into the Fabric community as I didn't find a lot of libraries and those definitely didn't have something like this in them (or similar features that try to unite the way something is done). The only one that come to my mind are Cloth and ModMenu.

kyrptonaught commented 4 years ago

Fabric API actually does have an api for adding tooltips, but I chose not to use it due to not having the ability to remove existing tooltips.

Here is my proposed change: https://github.com/kyrptonaught/EnchantedToolTips/commit/7982476bba2888b4c1298cea7f68715f16b5a239#diff-1c414cbc473a90635f3b9257332b99df The mixin is now just a redirect of a vanilla call, and without canceling the rest of the method. It is now possible to inject to the "TAIL" to add other tooltips

mainrs commented 4 years ago

Ah, now I understand the @Redirect annotation. This should work.