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

Add tooltip related extension methods for Attribute & Attribute related tooltip fix #1371

Open
wants to merge 2 commits into
base: 1.21.x
Choose a base branch
from

Conversation

RaymondBlaze
Copy link
Contributor

@RaymondBlaze RaymondBlaze commented Jul 25, 2024

Context

  • Vanilla has some hardcoded special case for building tooltip for attribute modifiers in ItemStack, like the weapon attack damage and attack speed modifier and the Attributes.KNOCKBACK_RESISTANCE.
  • PotionContents#addPotionTooltip does not respect Attribute#getStyle (This is a vanilla bug, haven't search about it on Mojira yet).

Features

  • A extension for mods to achieve the similar behavior, allowing them to modify the displayed amount or provide custom the tooltip line. NeoForge's BooleanAttribute also benefits from this extension as now it is showed in tooltip in the style of Enables <Attribute> and Disables <Attribute> instead of +1 <Attribute> and -100% <Attribute>, which is more user friendly.
  • A useful overload of the PotionContents#addPotionTooltip, so it may use a custom consuming description other than the potion.whenDrank, which is useful for mods who wish to utilize the method in their non-potion item (like foods) tooltip.

Fixes

  • PotionContents#addPotionTooltip does not respect Attribute#getStyle(boolean).
  • Set NAMETAG_DISTANCE's sentiment to NEUTRAL, replacing the old BENEFICIAL default.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@Shadows-of-Fire
Copy link
Contributor

This looks like a more minimal version of the IFormattableAttribute API supplied by Apothic Attributes, which is a good thing; though I would want to nudge something in a direction that makes more sense for the usecases I've seen:

Your solution supplies double getAmountForDisplay(AttributeModifier modifier), but then immediately contradicts itself by having BooleanAttribute exist - for this reason, the "display value" should be a Component instead of a double, since there may be non-numeric attributes afloat.

Apothic Attributes uses MutableComponent toValueComponent(@Nullable Operation op, double value, TooltipFlag flag) [link] for this purpose, which can express the value of boolean attributes, numeric attributes, and dynamic attributes (which is a thing used by some mods where the attribute is a "stack" of modifiers and the default value is NaN)

The other method, Component getModifierDescription(@Nullable Player player, AttributeModifier modifier), is mostly fine, though I'm curious as to why you want the player as context. My version is MutableComponent toComponent(AttributeModifier modif, TooltipFlag flag) [link] since some additional debug data is displayed when the flag is raised.

@RaymondBlaze
Copy link
Contributor Author

Your solution supplies double getAmountForDisplay(AttributeModifier modifier), but then immediately contradicts itself by having BooleanAttribute exist - for this reason, the "display value" should be a Component instead of a double, since there may be non-numeric attributes afloat.

The double getAmountForDisplay(AttributeModifier modifier) is more of providing a low level control over the attribute tooltip with just the amount being modified, which is basically the vanilla use case. With advanced control needs like BooleanAttribute, mods should override Component getModifierDescription(@Nullable Player player, AttributeModifier modifier) instead. There's no use case between "only modify the numeric amount" and "take over the whole description as it should not be displayed in numeric", and patching in a Component just for the amount display is simply not quite feasible due to how the vanilla method is structured.

The other method, Component getModifierDescription(@Nullable Player player, AttributeModifier modifier), is mostly fine, though I'm curious as to why you want the player as context.

The player context is for cases like providing tooltips in the style how vanilla displays the attack damage and attack speed for items, they need a player context to get the attribute base value. Tooltip flag is not natively available for the PotionContents method, but I can patch it in anyways as the vanilla callsite do have the context. And since tooltip flag is brought up, it maybe useful if we allow some attribute modifiers to be hidden from the description?

@Shadows-of-Fire
Copy link
Contributor

Shadows-of-Fire commented Jul 26, 2024

allow some attribute modifiers to be hidden from the description

We could do that, though it depends what level of control we want to express over the tooltips - I have an event for this functionality in AA here

But it depends on AA's tooltip handling rules, which entirely replace the vanilla ones to provide more control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21 Targeted at Minecraft 1.21 bug A bug or error enhancement New (or improvement to existing) feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants