[1.21] Add an Attribute formatting API for better control of attribute tooltips #1551

Shadows-of-Fire · 2024-09-18T07:09:33Z

Description

This PR supplies an implementation of Apothic Attributes' IFormattibleAttribute API, which allows attributes to supply custom formatting logic for their attribute modifiers (and modifier values), as well as giving mods better control over what information is displayed when attribute modifier tooltips are added.

This PR supersedes #1371.

Features

Allows attributes to convert their own attribute modifiers and current values into MutableComponent(s), allowing attributes to map the raw numeric values to human-readable ones.
Abstracts the "Base Modifier ID" system that vanilla uses for Attack Damage and Attack Speed, allowing attributes to declare their own base modifier id which will be handled in tooltips.
Provides a single entrypoint for generating tooltips for attribute modifiers, which is also usable by mods that generate attribute modifiers for non-traditional slots.
Adds functionality to support merged attribute tooltips, which compresses multiple modifiers for a single attribute into one line.
a. This functionality is disabled by default, and must be enabled by a mod calling NeoForgeMod.enableMergedAttributeTooltips().
Shows advanced debug information when F3+H is enabled to clarify the underlying modifier, regardless of customizations done by the attribute.

Examples

Images from current implementation of various modifiers:

Default Settings:

With Advanced Tooltips:

With Merged Tooltips:

With Merged Tooltips Holding Shift:

Docs need work, and the thing still needs to be tested, but the impl is here and it compiles.

neoforged-pr-publishing · 2024-09-18T07:09:48Z

Publish PR to GitHub Packages

Last commit published: e9e5871b7dc033cf3894823f383b1a460923434f.

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 #1551' // https://github.com/neoforged/NeoForge/pull/1551
        url 'https://prmaven.neoforged.net/NeoForge/pr1551'
        content {
            includeModule('net.neoforged', 'neoforge')
            includeModule('net.neoforged', 'testframework')
        }
    }
}

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-pr1551.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1551
cd NeoForge-pr1551
curl -L https://prmaven.neoforged.net/NeoForge/pr1551/net/neoforged/neoforge/21.1.67-pr-1551-attr-tooltips/mdk-pr1551.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.

src/main/java/net/neoforged/neoforge/common/extensions/IAttributeExtension.java

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java

Shadows-of-Fire · 2024-09-19T04:51:44Z

cc'ing potentially relevant mod authors for comment:
@pufmat (Pufferfish Skills)
@amoooooo (Alembic)
@Daripher (Passive Skill Tree)
@RobertSkalko (Mine & Slash)

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java

amoooooo · 2024-09-19T16:57:08Z

cc'ing potentially relevant mod authors for comment: @pufmat (Pufferfish Skills) @amoooooo (Alembic) @Daripher (Passive Skill Tree) @RobertSkalko (Mine & Slash)

@dhyces also pulling in ratatosk, other main alembic dev

KnightMiner · 2024-09-19T17:13:10Z

In terms of how I use attributes, the main concern I have is the ability to conditionally suppress attributes from the tooltip. My mod has item tooltips where holding control or shift displays a different set of info, and want attributes to show in 2 out of 3 of those modes (one with full info like they normally show, one with just the stats when worn in the proper slot). I currently accomplish this using the item tooltip event to suppress tooltips based on their translation keys, though I suspect this PR would make that task much more difficult.

For my usecases, I ultimately need two things:

The ability to conditionally disable the attributes from showing in the tooltip at all.
- I believe the GatherSkippedAttributeTooltipsEvent would let me do this, if I just condition on my item type and the key that suppresses attributes. Mentioning this to ensure the functionality persists.
The ability to display a subset of tooltips in my other tooltip mode
- My current logic suppresses all slots except 1 (which is determined based on the item type) and suppresses certain attributes by UUID that I display in another form. The GatherSkippedAttributeTooltipsEvent lets me handle the latter, but I don't see a way at a glance to skip all attributes for a given equipment slot/that are not in a given equipment slot.
- Worst case I could always entirely suppress the Neo logic then manually reimplement it in my item logic. In that case I suppose I'd need to call TooltipUtil.applyTextFor, though I'd really want to suppress the groupName logic there for my usecases. I left a comment of a possible solution there.

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java

Shadows-of-Fire · 2024-09-19T17:18:35Z

The ability to conditionally disable the attributes from showing in the tooltip at all

The GatherSkippedAttributeTooltipsEvent should have you covered fully there via GatherSkippedAttributeTooltipsEvent#setSkipAll

The ability to display a subset of tooltips in my other tooltip mode

I may be able to add per-slot logic to the event, though I worry about imposing a limitation on mods supplying attributes for non-EquipmentSlot-backed attribute groups. I guess this functionality would just be unused in that case, rather than being a true limiting factor.

RobertSkalko · 2024-09-19T19:02:30Z

I'll skip commenting a lot here because I don't actually use vanilla attributes ever since the mod was created. My tooltips are so large I just hide the vanilla tooltip and give a keybind to not hide, to well show the vanilla tooltip without my mod replacing it.

I think this is more important for the other less overhaul-y rpg mod devs like Apotheosis.. Yeah you're the OP heh. I don't actaully know any other good rpg mods.

What my mod also does is cut blank lines in tooltip if the tooltip gets too big.. which it does, even when i cut off the entire vanilla tooltip. I think forge was missing a scrollable tooltip mod last time I checked. Just problems that happen when you have: base, implicit, prefix, suffix, corruption stats on one item.. and a few other things

I also have a system where pressing ALT, SHIFT, CTRL provide different functions to the tooltips. One makes my tooltips show stat ranges from the affix, one shows descriptions for stats, and the last one hides the tooltip and lets vanilla and mods do whatever.

That's it from me. I initially unfollowed the post but got pinged by the quote.. Not sure how helpful this will be

dhyces · 2024-09-21T15:00:27Z

Not a huge fan of the changes to vanilla handling, but eh. Why isn't the merged tooltip option a config setting? It feels like it would fit better in a config than a "forge milk" method call.

lcy0x1 · 2024-09-21T15:16:58Z

I think it’s better to allow mods who need this feature to have an option to enable it from code instead of having it fully controlled by config.

For example, I have mods that need this merged tooltip to make my weapon tooltip look better. I think Apoth wants it as well.

What about making config to be a tri-state (on/default/off), while default allows mods to turn them on?

pupnewfster · 2024-09-21T14:45:01Z

patches/net/minecraft/world/entity/ai/attributes/Attribute.java.patch

+    protected static final net.minecraft.network.chat.TextColor MERGED_RED = net.minecraft.network.chat.TextColor.fromRgb(0xF93131);
+    protected static final net.minecraft.network.chat.TextColor MERGED_BLUE = net.minecraft.network.chat.TextColor.fromRgb(0x7A7AF9);
+    protected static final net.minecraft.network.chat.TextColor MERGED_GRAY = net.minecraft.network.chat.TextColor.fromRgb(0xCCCCCC);


What is the reasoning for using slightly different colors for the merged variants than vanilla uses for normal attributes?

That's the only user-facing indicator that a modifier line is hiding a merged tooltip. It needs some kind of indicator, and I found colors to work well.

pupnewfster · 2024-09-21T14:45:29Z

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

+    /**
+     * @deprecated Neo: Use {@link net.neoforged.neoforge.client.util.TooltipUtil#addAttributeTooltips}
+     */
+    @Deprecated
     private void addAttributeTooltips(Consumer<Component> p_330796_, @Nullable Player p_330530_) {
         ItemAttributeModifiers itemattributemodifiers = this.getOrDefault(DataComponents.ATTRIBUTE_MODIFIERS, ItemAttributeModifiers.EMPTY);
 +        // Neo: We don't need to call IItemStackExtension#getAttributeModifiers here, since it will be done in forEachModifier.


Given we are deprecating this method now, should we just remove the comment inside it that we are patching in as well?

pupnewfster · 2024-09-21T15:09:47Z

src/main/java/net/neoforged/neoforge/common/BooleanAttribute.java

+            comp = Component.translatable("neoforge.modifier.bool", this.toValueComponent(modif.operation(), value, flag), Component.translatable(this.getDescriptionId())).withStyle(ChatFormatting.BLUE);
+        } else {
+            value *= -1.0D;
+            comp = Component.translatable("neoforge.modifier.bool", this.toValueComponent(modif.operation(), value, flag), Component.translatable(this.getDescriptionId())).withStyle(ChatFormatting.RED);


Instead of copying this long line twice, can we just set a ChatFormatting variable instead of one for MutableComponent?

src/main/java/net/neoforged/neoforge/common/NeoForgeMod.java

pupnewfster · 2024-09-21T15:17:08Z

src/main/java/net/neoforged/neoforge/common/extensions/IAttributeExtension.java

+     * @return The component form of the formatted value.
+     */
+    default MutableComponent toValueComponent(@Nullable Operation op, double value, TooltipFlag flag) {
+        // Knockback Resistance should be a PercentageAttribute, but we can't registry replace attributes, so we do this here.


I am not sure which is the better solution, but can't we just patch Attributes.java to have it be new PercentageAttribute instead of new RangedAttribute?

pupnewfster · 2024-09-21T15:51:49Z

src/main/java/net/neoforged/neoforge/common/util/AttributeUtil.java

+     * Client bouncer class to avoid class loading issues. Access to this class still needs a dist check.
+     */
+    private static class ClientAccess {
+        @SuppressWarnings("resource")


My IDE is telling me this is a redundant suppression. Is this needed for eclipse or was it due to code that you had while prototyping that no longer exists?

pupnewfster · 2024-09-21T15:54:50Z

src/main/java/net/neoforged/neoforge/common/util/AttributeUtil.java

+        modifierMap.forEach((attr, modif) -> {
+            BaseModifier base = baseModifs.get(attr);
+            if (base != null) {
+                base.children.add(modif);
+            }
+        });


Could we replace this with a loop to avoid the unnecessary capturing lambda?

for (Map.Entry<Holder<Attribute>, AttributeModifier> modifierEntry : modifierMap.entries()) { BaseModifier base = baseModifs.get(modifierEntry.getKey()); if (base != null) { base.children.add(modifierEntry.getValue()); } }

pupnewfster · 2024-09-21T15:57:32Z

src/main/java/net/neoforged/neoforge/common/util/AttributeUtil.java

+    /**
+     * Creates a mutable component starting with the char used to represent a drop-down list.
+     */
+    private static MutableComponent list() {


This could use a slightly more descriptive method name, given list gives the impression it is some sort of List<...>

pupnewfster · 2024-09-21T15:58:21Z

src/main/java/net/neoforged/neoforge/common/util/AttributeUtil.java

+            if (NeoForgeMod.shouldMergeAttributeTooltips() && modifs.size() > 1) {
+                double[] sums = new double[3];
+                boolean[] merged = new boolean[3];
+                Map<Operation, List<AttributeModifier>> shiftExpands = new HashMap<>();


Is there a reason for this being a HashMap as opposed to an EnumMap?

pupnewfster · 2024-09-21T15:59:03Z

src/main/java/net/neoforged/neoforge/common/util/AttributeUtil.java

+
+                for (AttributeModifier modifier : modifs) {
+                    if (modifier.amount() == 0) continue;
+                    if (sums[modifier.operation().ordinal()] != 0) merged[modifier.operation().ordinal()] = true;


Can we just retrieve the ordinal once and store it in a variable so that this code is a bit more readable?

Initial IFormattableAttribute implementation

90c20b0

Docs need work, and the thing still needs to be tested, but the impl is here and it compiles.

Shadows-of-Fire added enhancement New (or improvement to existing) feature or request 1.21.1 Targeted at Minecraft 1.21.1 labels Sep 18, 2024

Shadows-of-Fire self-assigned this Sep 18, 2024

Shadows-of-Fire marked this pull request as draft September 18, 2024 07:09

lcy0x1 reviewed Sep 18, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/common/extensions/IAttributeExtension.java Outdated Show resolved Hide resolved

lcy0x1 reviewed Sep 18, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java Outdated Show resolved Hide resolved

lcy0x1 reviewed Sep 18, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java Outdated Show resolved Hide resolved

lcy0x1 reviewed Sep 19, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java Outdated Show resolved Hide resolved

Finish API-ifying the implementation

610bee9

Shadows-of-Fire marked this pull request as ready for review September 19, 2024 04:28

Shadows-of-Fire requested a review from a team September 19, 2024 04:28

Shadows-of-Fire added 2 commits September 18, 2024 21:43

Fix sentiment on nametag distance

c1be879

Make potion tooltips use new extensions

f5e58c2

lcy0x1 reviewed Sep 19, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java Outdated Show resolved Hide resolved

Switch to TooltipFlag key modifier methods

0e265f9

KnightMiner reviewed Sep 19, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java Outdated Show resolved Hide resolved

KnightMiner reviewed Sep 19, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java Outdated Show resolved Hide resolved

KnightMiner reviewed Sep 19, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/client/util/TooltipUtil.java Outdated Show resolved Hide resolved

Shadows-of-Fire added 4 commits September 20, 2024 22:17

Move group name logic to applyModifierTooltips

8990ed5

Flattern TooltipUtil into AttributeUtil

82321aa

update event sidedness docs

2c95e34

Add the ability to skip entire groups to GatherSkipped

e9e5871

Shadows-of-Fire requested a review from KnightMiner September 21, 2024 07:30

Shadows-of-Fire requested a review from lcy0x1 September 21, 2024 07:30

pupnewfster requested changes Sep 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.21] Add an Attribute formatting API for better control of attribute tooltips #1551

[1.21] Add an Attribute formatting API for better control of attribute tooltips #1551

Shadows-of-Fire commented Sep 18, 2024 •

edited

Loading

neoforged-pr-publishing bot commented Sep 18, 2024 •

edited

Loading

The artifacts published by this PR:

Repository Declaration

MDK installation

Shadows-of-Fire commented Sep 19, 2024

amoooooo commented Sep 19, 2024

KnightMiner commented Sep 19, 2024

Shadows-of-Fire commented Sep 19, 2024

RobertSkalko commented Sep 19, 2024

dhyces commented Sep 21, 2024

lcy0x1 commented Sep 21, 2024

pupnewfster Sep 21, 2024

Shadows-of-Fire Sep 21, 2024

pupnewfster Sep 21, 2024

pupnewfster Sep 21, 2024

pupnewfster Sep 21, 2024

pupnewfster Sep 21, 2024

pupnewfster Sep 21, 2024

pupnewfster Sep 21, 2024

pupnewfster Sep 21, 2024

pupnewfster Sep 21, 2024

[1.21] Add an Attribute formatting API for better control of attribute tooltips #1551

Are you sure you want to change the base?

[1.21] Add an Attribute formatting API for better control of attribute tooltips #1551

Conversation

Shadows-of-Fire commented Sep 18, 2024 • edited Loading

Description

Features

Examples

Default Settings:

With Advanced Tooltips:

With Merged Tooltips:

With Merged Tooltips Holding Shift:

neoforged-pr-publishing bot commented Sep 18, 2024 • edited Loading

The artifacts published by this PR:

Repository Declaration

MDK installation

Shadows-of-Fire commented Sep 19, 2024

amoooooo commented Sep 19, 2024

KnightMiner commented Sep 19, 2024

Shadows-of-Fire commented Sep 19, 2024

RobertSkalko commented Sep 19, 2024

dhyces commented Sep 21, 2024

lcy0x1 commented Sep 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadows-of-Fire commented Sep 18, 2024 •

edited

Loading

neoforged-pr-publishing bot commented Sep 18, 2024 •

edited

Loading