Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TooltipDataCallback to allow modders add their own tooltips to existing items #3403

Open
wants to merge 10 commits into
base: 1.20.2
Choose a base branch
from

Conversation

JumperOnJava
Copy link

@JumperOnJava JumperOnJava commented Nov 5, 2023

I recently was developing feature that added "sell" and "purchase" tooltips for blocks with that functionality. The problem is that these toolips were added by different mods, one for selling, one for purchasing. And to make these mods compatible i had to develop small separate library, and package it to both mods. And i haven't even tested compatibility with other mods that may change tooltips.
The only tooltip related events I found are for adding text lines to tooltip and registering tooltip component.
So I propose new event the Fabric API - TooltipDataCallback(ItemStack, List).
Also i added three example mods to showcase what this can add (Fabric discord message link)

@modmuss50 modmuss50 added the enhancement New feature or request label Nov 5, 2023
@modmuss50 modmuss50 requested a review from a team November 5, 2023 14:02
@Juuxel Juuxel added the priority:low Low priority PRs that don't immediately need to be resolved label Nov 6, 2023
JumperOnJava and others added 5 commits November 6, 2023 19:52
…ient/rendering/v1/TooltipComponentCallback.java

Co-authored-by: Juuz <6596629+Juuxel@users.noreply.github.com>
…ient/rendering/v1/TooltipDataCallback.java

Co-authored-by: Juuz <6596629+Juuxel@users.noreply.github.com>
It happened when only one tooltip data is added by event handlers
I for some reason assumed that there is always original data in item
…client/rendering/HandledScreenMixin.java

Co-authored-by: Juuz <6596629+Juuxel@users.noreply.github.com>
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, could possibly use Mixin extras in the Mixin, but its not a major issue.


@Mixin(HandledScreen.class)
class HandledScreenMixin {
@Redirect(method = "drawMouseoverTooltip", at = @At(value = "INVOKE", target = "Lnet/minecraft/item/ItemStack;getTooltipData()Ljava/util/Optional;"))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A WrapOperation would be better.

Copy link
Author

@JumperOnJava JumperOnJava Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixin extras avaliable only on loader 0.15+, this version is 0.14 compatible
Changing it for one small feature doesn't make much sense (for me)
Here is ME version for 1.20.4 #3486

@modmuss50 modmuss50 added the last call If you care, make yourself heard right away! label Dec 28, 2023
Comment on lines +42 to +44
if (multiData.tooltipData().isEmpty()) {
return original;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect people to be allowed to remove the original?

@Redirect(method = "drawMouseoverTooltip", at = @At(value = "INVOKE", target = "Lnet/minecraft/item/ItemStack;getTooltipData()Ljava/util/Optional;"))
Optional<TooltipData> addMultiData(ItemStack stack) {
Optional<TooltipData> original = stack.getTooltipData();
var multiData = new MultiTooltipData(new ArrayList<>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: No need to create MultiTooltipData here, just start with the list, and then create it later if the list > 1

import net.fabricmc.api.ClientModInitializer;
import net.fabricmc.fabric.api.client.rendering.v1.TooltipComponentCallback;

public class MultiTooltipComponentRegister implements ClientModInitializer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MultiTooltipComponentInitializer maybe, this class name is a little weird.

@modmuss50 modmuss50 removed the last call If you care, make yourself heard right away! label Jan 5, 2024
@modmuss50
Copy link
Member

Remvoed from last call, there are breaking changes between the two PRs, it might just be best to have one and make sure that all the behaviour is tested/defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:low Low priority PRs that don't immediately need to be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants