[20.6] Add more item tooltip event entry points #1019

ApexModder · 2024-05-27T07:39:15Z

Expands the ItemTooltipEvent to have more entry points, allowing modders to place their tooltips in more locations other than the bottom of the tooltip stack.

neoforged-pr-publishing · 2024-05-27T07:39:28Z

Publish PR to GitHub Packages

Last commit published: 0a6113851dae89089bb7531bea1fca2a3fddb8a9.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1019' // https://github.com/neoforged/NeoForge/pull/1019
        url 'https://prmaven.neoforged.net/NeoForge/pr1019'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1019.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1019
cd NeoForge-pr1019
curl -L https://prmaven.neoforged.net/NeoForge/pr1019/net/neoforged/neoforge/20.6.109-beta-pr-1019-pr-tooltip-ordering/mdk-pr1019.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

patches/net/minecraft/world/item/ItemStack.java.patch

src/main/java/net/neoforged/neoforge/event/entity/player/ItemTooltipEvent.java

patches/net/minecraft/world/item/ItemStack.java.patch

Shadows-of-Fire · 2024-05-28T02:24:37Z

Looking further at this, I believe sub events will promote poor user experience as far as consuming the event goes. If you want to adjust two tooltip segments based on any common logic, you have to replicate that logic in both calls. What would produce a better event surface is pre-dividing the relevant tooltip segments into sublists, passing those sublists to a single event, and then composing them in-order after the event's resolution. At the patch level, this can be accomplished by creating the relevant sub-lists and redirecting the local Consumer<Component> consumer = list::add; to the sublist relevant to that segment. The sublists can then be passed to the ItemTooltipEvent where they can be modified individually, and then composed into the single tooltip list after the event is completed.

This preserves the goal of being able to edit the segments, without requiring either multiple events or multiple firings of the event.

robotgryphon · 2024-05-28T02:45:35Z

Shadows, I disagree with creating multiple lists. Given the hot nature of this event, you're now adding overhead of multiple lists and combination logic to something that runs every frame.

If modders have shared logic, it's easily split out into a small private method that can be called from both event listeners -- but at that point, you're adding info to multiple spots in the tooltip. Probably a code smell on the modders' part anyway.

Shadows-of-Fire · 2024-05-28T02:52:23Z

A feature that this implementation lacks is being able to replace sections. Doing so will require cross-event communication via sentinel values, which is something I was hoping to be able to replace after the feature expansion.

The cost of the list creation and recombination is not that substantial. In general, at most one tooltip is drawn at a time.

robotgryphon · 2024-05-28T02:58:01Z

You're expanding the scope of this PR a lot by turning it into a full tooltip management event. What happens when a modder accidentally clears out all the sections, trying to render their own info? Now you're losing info.

The original purpose of the event was to provide extra info; this PR simply expands that to give more spots to hook in to.

Why are we making this way more complicated than it needs to be? ESPECIALLY as Mojang is moving data components and attributes around, so it might be short lived anyway?

Shadows-of-Fire · 2024-05-28T05:07:52Z

You're expanding the scope of this PR a lot by turning it into a full tooltip management event.

That's not a reasonable claim. Representing the stages as sub lists is simply a more expressive means of recording the multiple injection points. A fully-fledged tooltip management system would retain far more features.

What happens when a modder accidentally clears out all the sections, trying to render their own info? Now you're losing info.

Its lost; as is the case with incompatible modifications. That is still the case with the current (pre-PR) system, and the system proposed by this PR. No solutions thus far make any attempt to retain the vanilla tooltip data in the event that a mod wants to clear a section.

The original purpose of the event was to provide extra info; this PR simply expands that to give more spots to hook in to.

Yes, and it must do so in a meaningful enough way to warrant a breaking change. The current change would not satisfy the use cases of Apothic Attributes, Quark, or Obscuria Tooltips (and potentially others) that rely on adjusting specific tooltip sections.

ESPECIALLY as Mojang is moving data components and attributes around, so it might be short lived anyway?

I'm not really sure what this refers to. Nothing about tooltips has changed substantially at the itemstack / organizational level. The transition from NBT to components meant the code there changed a bit, but the ordering (and the result) is still the same.

ChampionAsh5357 · 2024-05-28T15:43:05Z

Since we're not modifying anything, why not just set this up like how we do GUI layers? Everything is sorted after registry so there should be no major overhead.

ApexModder · 2024-05-30T00:35:24Z

Fully reimplemented PR into a TooltipManager similar to GuiLayerManager

Mods now have to register tooltip providers via RegisterTooltipProvidersEvent.
Providers can be marked as above/below another provider or 'all' using one of the register methods in the event.

With this change mods can now pick and choose where to inject their tootltips to.
See VanillaTooltipProviders for vanilla tooltip exampls.

This system is also made in a generic fashion so modders could easily reuse for say fluid tooltips

ChampionAsh5357

Just some quick comments at a glance. Will do a more thorough review later.

ChampionAsh5357 · 2024-05-30T01:00:18Z

patches/net/minecraft/world/item/ItemStack.java.patch

-             }
-
-+            net.neoforged.neoforge.event.EventHooks.onItemTooltip(this, p_41652_, list, p_41653_, p_339637_);
+-            if (this.has(DataComponents.CUSTOM_NAME)) {


Why? Just return early. Don't delete so much.

ChampionAsh5357 · 2024-05-30T01:01:51Z

src/main/java/net/neoforged/neoforge/tooltip/VanillaTooltipProviders.java

+        if (operation == AttributeModifier.Operation.ADD_MULTIPLIED_BASE || operation == AttributeModifier.Operation.ADD_MULTIPLIED_TOTAL)
+            d1 = d0 * 100D;
+        else if (attribute.is(Attributes.KNOCKBACK_RESISTANCE))
+            d1 = d0 * 10D;
+        else
+            d1 = d0;


Please no. Use brackets for statements since it just reads so weird

ChampionAsh5357 · 2024-05-30T01:02:36Z

src/main/java/net/neoforged/neoforge/tooltip/VanillaTooltipProviders.java

+
+    private static void addModifierTooltip(ItemStack stack, Consumer<Component> adder, @Nullable Player player, Holder<Attribute> attribute, AttributeModifier attributeModifier) {
+        // Copied from ItemStack | private in vanilla
+        var d0 = attributeModifier.amount();


I don't mind copy paste, but it should generally be updated with actual meaningful names

Shadows-of-Fire · 2024-05-30T01:15:21Z

Why this approach? It doesn't solve the issue I outlined earlier (section replacement is still not possible) but now requires we maintain a copy of the tooltip flow.

robotgryphon · 2024-05-31T14:30:33Z

See #1039 for rationale on this PR, and why we wanted the original hook implementation.

ApexModder · 2024-06-13T16:45:54Z

Closing as PR went stale and overall seems very controversial

ApexModder added enhancement New (or improvement to existing) feature or request breaking change Breaks binary compatibility 1.20.6 Targeted at Minecraft 1.20.6 labels May 27, 2024

FiniteReality requested changes May 27, 2024

View reviewed changes

patches/net/minecraft/world/item/ItemStack.java.patch Outdated Show resolved Hide resolved

patches/net/minecraft/world/item/ItemStack.java.patch Outdated Show resolved Hide resolved

src/main/java/net/neoforged/neoforge/event/entity/player/ItemTooltipEvent.java Outdated Show resolved Hide resolved

Shadows-of-Fire reviewed May 27, 2024

View reviewed changes

patches/net/minecraft/world/item/ItemStack.java.patch Outdated Show resolved Hide resolved

Shadows-of-Fire self-requested a review May 27, 2024 07:57

ApexModder added 6 commits May 29, 2024 23:49

Add more item tooltip event entry points

df33f15

Fix white space in ItemStack patch

6a796f9

Remove Before/After attributes sub event

c48f939

Remove old tooltip test code

f183698

Fix item tooltip tests failing

e91ff4f

Reimplement PR via TooltipManager

06787d6

ApexModder force-pushed the pr/tooltip-ordering branch from 797447a to 06787d6 Compare May 30, 2024 00:31

ApexModder added 2 commits May 30, 2024 01:51

Add javadocs

a25128f

Formatting

2028a65

ChampionAsh5357 requested changes May 30, 2024

View reviewed changes

ApexModder added 2 commits May 30, 2024 02:13

Redo ItemStack patch to quit out early rather than deleting everything

4105d1b

Slight reformatting to VanillaTooltipProviders

0a61138

ApexModder closed this Jun 13, 2024

ApexModder deleted the pr/tooltip-ordering branch July 29, 2024 08:12

ApexModder restored the pr/tooltip-ordering branch July 29, 2024 08:13

ApexModder deleted the pr/tooltip-ordering branch July 31, 2024 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[20.6] Add more item tooltip event entry points #1019

[20.6] Add more item tooltip event entry points #1019

ApexModder commented May 27, 2024

neoforged-pr-publishing bot commented May 27, 2024 •

edited

Loading

The artifacts published by this PR:

Repository Declaration

MDK installation

Shadows-of-Fire commented May 28, 2024

robotgryphon commented May 28, 2024

Shadows-of-Fire commented May 28, 2024

robotgryphon commented May 28, 2024

Shadows-of-Fire commented May 28, 2024

ChampionAsh5357 commented May 28, 2024

ApexModder commented May 30, 2024 •

edited

Loading

ChampionAsh5357 left a comment

ChampionAsh5357 May 30, 2024

ChampionAsh5357 May 30, 2024

ChampionAsh5357 May 30, 2024

Shadows-of-Fire commented May 30, 2024

robotgryphon commented May 31, 2024

ApexModder commented Jun 13, 2024

[20.6] Add more item tooltip event entry points #1019

[20.6] Add more item tooltip event entry points #1019

Conversation

ApexModder commented May 27, 2024

neoforged-pr-publishing bot commented May 27, 2024 • edited Loading

The artifacts published by this PR:

Repository Declaration

MDK installation

Shadows-of-Fire commented May 28, 2024

robotgryphon commented May 28, 2024

Shadows-of-Fire commented May 28, 2024

robotgryphon commented May 28, 2024

Shadows-of-Fire commented May 28, 2024

ChampionAsh5357 commented May 28, 2024

ApexModder commented May 30, 2024 • edited Loading

ChampionAsh5357 left a comment

Choose a reason for hiding this comment

ChampionAsh5357 May 30, 2024

Choose a reason for hiding this comment

ChampionAsh5357 May 30, 2024

Choose a reason for hiding this comment

ChampionAsh5357 May 30, 2024

Choose a reason for hiding this comment

Shadows-of-Fire commented May 30, 2024

robotgryphon commented May 31, 2024

ApexModder commented Jun 13, 2024

neoforged-pr-publishing bot commented May 27, 2024 •

edited

Loading

ApexModder commented May 30, 2024 •

edited

Loading