Add tooltip related extension methods for Attribute & Attribute related tooltip fix
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 theAttributes.KNOCKBACK_RESISTANCE. -
PotionContents#addPotionTooltipdoes not respectAttribute#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
BooleanAttributealso benefits from this extension as now it is showed in tooltip in the style ofEnables <Attribute>andDisables <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 thepotion.whenDrank, which is useful for mods who wish to utilize the method in their non-potion item (like foods) tooltip.
Fixes
-
PotionContents#addPotionTooltipdoes not respectAttribute#getStyle(boolean). - Set
NAMETAG_DISTANCE's sentiment toNEUTRAL, replacing the oldBENEFICIALdefault.
- [ ] Publish PR to GitHub Packages
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.
Your solution supplies
double getAmountForDisplay(AttributeModifier modifier), but then immediately contradicts itself by havingBooleanAttributeexist - 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?
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.
@RaymondBlaze, this pull request has conflicts, please resolve them for this PR to move forward.
Superseded by #1551