Skip to content

feat(items): the tooltip's stat lines come from a tooltip layout - #824

Open
ezequielzacca wants to merge 3 commits into
sven-n:mainfrom
ezequielzacca:feat/item-tooltip-layouts
Open

ezequielzacca wants to merge 3 commits into
sven-n:mainfrom
ezequielzacca:feat/item-tooltip-layouts

Conversation

@ezequielzacca

@ezequielzacca ezequielzacca commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

This step's own changes: compare with #823. The diff of this PR also holds every earlier step of #815, since a PR from a fork can only target this repository's branches.

Summary

Step 5b-3 of #815 (data-driven item tooltips; design). Stacked on #823 (→ #822 → … → #813): only the last commit, feat(items): the tooltip's stat lines come from a tooltip layout, is this step.

The tooltip's stat and requirement lines are data now: Data/Items/Tooltips/Tooltip.json holds their texts, with translations, and a layout that says which lines show, in what order, color and style. RenderItemInfo() loses about 250 lines.

What moved

From RenderItemInfo() into the default layout:

  • damage (one-handed, two-handed, wizardry);
  • defense, magic defense, defense rate, attack and walk speed;
  • the seven resistances;
  • the six requirements with their "(lacking …)" lines;
  • the staves', books' and magic swords' wizardry or curse damage increase, and the sceptres' pet attack increase.

Durability stays code for now. Its rule has about 20 exceptions, and they become per-item layouts in a follow-up (5b-3b).

The file

"texts": {
  "requiredStrength": {"en": "Strength Requirement: {requiredStrength}", "de": "Benötigte Stärke: {requiredStrength}", …},
  "lacking": {"en": "(lacking {missing})", "de": "(fehlt {missing})", …}
},
"layouts": [{"name": "default", "lines": [
  {"section": "name"},
  {"text": "@defense", "when": ["defense"], "colors": [{"when": ["defenseHarmonyBonus"], "color": "yellow"}, {"when": ["armour", "excellent"], "color": "blue"}]},
  …
  {"section": "description"},
  {"text": "@requiredStrength", "when": ["requiredStrength", "!tag:luckyItem"], "colors": [{"when": ["missingStrength"], "color": "red"}, {"when": ["strengthReduced"], "color": "yellow"}]},
  {"text": "@lacking", "values": {"missing": "missingStrength"}, "when": ["requiredStrength", "!tag:luckyItem", "missingStrength"], "color": "red"},
  …
  {"section": "classes"},
  {"text": "", "when": ["tag:sceptre"]},
  {"text": "@petAttackIncrease", "when": ["tag:sceptre"], "color": "blue", "bold": true},
  {"section": "options"}
]}]
  • when: a list, and every entry must hold. An entry is one of:

    A ! negates an entry.

  • colors: rules where the first that holds wins; otherwise the line's color.

  • values: binds a text's placeholder for one line, so one "(lacking {missing})" serves all six requirements.

  • section: a part that's still code. The lines go between name, description, classes and options.

The full format, the states and the checks are in docs/item-data.md, "Tooltip layouts". Each check is an error when the data loads, and the message suggests the right name for a typo:

  • an unknown @text, condition, tag, color or placeholder;
  • a translation whose placeholders differ from the English text's;
  • misplaced sections.

Item checks that became tags

Two hardcoded item checks are item tags now, so the layout can name them:

  • noDamageLine: the Scrolls of Teleport, Teleport Ally and Soul Barrier. They have damage in their data but show no damage line.
  • noLevelRequirement: Loch's Feather.

The luckyItem, sceptre and summonerBook tags already existed.

Tooltips that changed, on purpose

"Every item's tooltip is as recorded" (#822) compares every item's tooltip at every level, as four variants, for two heroes. It's re-recorded here for 11 items, and every other tooltip is unchanged. Both changes come from the old damage block's TextNum-- when it skipped the line:

  • Eight skill scrolls whose skill has no damage (Nova, Berserker, Sleep, Weakness, Wizardry Enhance, Chain Drive, Dark Side, Dragon Roar) lost the blank line under their name. They keep it now, like every other item.
  • The three teleport scrolls, when excellent, recolored that blank line blue. A blank line shows no color, so nothing visible changes.

.resx

The texts only these lines used are gone from every language:

  • Spell resistance: %d
  • Moving speed: %d
  • %s Resistance: %d
  • Stamina Requirement: %d
  • Increase pet attack as %d%%
  • Curse Spell increment %d%%

The others stay, because other windows use them: the character and pet info windows, the pet manager, the skill tooltip, and another tooltip case.

Found while doing this

The map name fallback Lookup(30 + iMap) (MapManager.cpp) reaches the stat line ids 66–80 for the maps with no name of their own. That's an existing bug, separate from this step, but this PR changes what it shows: map 42's name was the text %s Resistance: %d, and with ids 66, 68 and 72 removed it becomes an empty text (Lookup returns "" for a missing id). Maps 36, 38 and 42 are affected the same way.

Screenshots

Each pair is the same tooltip before (#823) and after this PR, in the legacy and modern themes, for a level 400 Dark Knight. Only the Scroll of Nova changes. In the others, the stat lines now come from the layout and look the same as before.

Scroll of Nova: the one visible change

Before, its skill has no damage, so the old damage block's TextNum-- deleted the blank line under the name. Every other item has that blank line, and now this scroll does too. Its wizardry damage line is skipped, since its hasDamage doesn't hold.

Legacy theme Modern theme
Before
After
The layout lines behind it (Tooltip.json)
{"text": "@wizardryDamage", "when": ["hasDamage", "!tag:noDamageLine", "skillBook"], "colors": [{"when": ["damageHarmonyBonus"], "color": "yellow"}, {"when": ["excellent"], "color": "blue"}]}
{"text": "@requiredEnergy", "when": ["requiredEnergy", "!tag:luckyItem"], "colors": [{"when": ["missingEnergy"], "color": "red"}]}
{"text": "@lacking", "values": {"missing": "missingEnergy"}, "when": ["requiredEnergy", "!tag:luckyItem", "missingEnergy"], "color": "red"}

Their texts (English):

"lacking": "(lacking {missing})"
"requiredEnergy": "Energy Requirement: {requiredEnergy}"
"wizardryDamage": "Wizardry Damage: {damageMin} ~ {damageMax}"

Legendary Staff +9

Damage, attack speed, a requirement, then the bold blue wizardry damage increase after the classes.

Legacy theme Modern theme
Before
After
The layout lines behind it (Tooltip.json)
{"text": "@oneHandedDamage", "when": ["hasDamage", "!tag:noDamageLine", "!skillBook", "!twoHanded"], "colors": [{"when": ["damageHarmonyBonus"], "color": "yellow"}, {"when": ["excellent"], "color": "blue"}]}
{"text": "@attackSpeed", "when": ["attackSpeed"]}
{"text": "@requiredStrength", "when": ["requiredStrength", "!tag:luckyItem"], "colors": [{"when": ["missingStrength"], "color": "red"}, {"when": ["strengthReduced"], "color": "yellow"}]}
{"text": "@wizardryDamageIncrease", "when": ["wizardryWeapon", "!tag:summonerBook"], "color": "blue", "bold": true}

Their texts (English):

"attackSpeed": "Attack speed: {attackSpeed}"
"oneHandedDamage": "One-Handed Damage: {damageMin} ~ {damageMax}"
"requiredStrength": "Strength Requirement: {requiredStrength}"
"wizardryDamageIncrease": "Wizardry Dmg {magicPower}% rise"

Book of Samut

A summoner book: a red requirement with its "(lacking …)" line, and the curse increase (tag:summonerBook) instead of the wizardry one.

Legacy theme Modern theme
Before
After
The layout lines behind it (Tooltip.json)
{"text": "@attackSpeed", "when": ["attackSpeed"]}
{"text": "@requiredDexterity", "when": ["requiredDexterity", "!tag:luckyItem"], "colors": [{"when": ["missingDexterity"], "color": "red"}, {"when": ["dexterityReduced"], "color": "yellow"}]}
{"text": "@requiredEnergy", "when": ["requiredEnergy", "!tag:luckyItem"], "colors": [{"when": ["missingEnergy"], "color": "red"}]}
{"text": "@lacking", "values": {"missing": "missingEnergy"}, "when": ["requiredEnergy", "!tag:luckyItem", "missingEnergy"], "color": "red"}
{"text": "@curseDamageIncrease", "when": ["wizardryWeapon", "tag:summonerBook"], "color": "blue", "bold": true}

Their texts (English):

"attackSpeed": "Attack speed: {attackSpeed}"
"curseDamageIncrease": "Curse Spell increment {magicPower}%"
"lacking": "(lacking {missing})"
"requiredDexterity": "Agility Requirement: {requiredDexterity}"
"requiredEnergy": "Energy Requirement: {requiredEnergy}"

Great Scepter +9

The sceptre's pet attack increase, from tag:sceptre.

Legacy theme Modern theme
Before
After
The layout lines behind it (Tooltip.json)
{"text": "@oneHandedDamage", "when": ["hasDamage", "!tag:noDamageLine", "!skillBook", "!twoHanded"], "colors": [{"when": ["damageHarmonyBonus"], "color": "yellow"}, {"when": ["excellent"], "color": "blue"}]}
{"text": "@attackSpeed", "when": ["attackSpeed"]}
{"text": "@requiredStrength", "when": ["requiredStrength", "!tag:luckyItem"], "colors": [{"when": ["missingStrength"], "color": "red"}, {"when": ["strengthReduced"], "color": "yellow"}]}
{"text": "@requiredDexterity", "when": ["requiredDexterity", "!tag:luckyItem"], "colors": [{"when": ["missingDexterity"], "color": "red"}, {"when": ["dexterityReduced"], "color": "yellow"}]}
{"text": "@petAttackIncrease", "when": ["tag:sceptre"], "color": "blue", "bold": true}

Their texts (English):

"attackSpeed": "Attack speed: {attackSpeed}"
"oneHandedDamage": "One-Handed Damage: {damageMin} ~ {damageMax}"
"petAttackIncrease": "Increase pet attack as {magicPower}%"
"requiredDexterity": "Agility Requirement: {requiredDexterity}"
"requiredStrength": "Strength Requirement: {requiredStrength}"

Blade +11

A plain weapon: damage, speed and two requirements.

Legacy theme Modern theme
Before
After
The layout lines behind it (Tooltip.json)
{"text": "@oneHandedDamage", "when": ["hasDamage", "!tag:noDamageLine", "!skillBook", "!twoHanded"], "colors": [{"when": ["damageHarmonyBonus"], "color": "yellow"}, {"when": ["excellent"], "color": "blue"}]}
{"text": "@attackSpeed", "when": ["attackSpeed"]}
{"text": "@requiredStrength", "when": ["requiredStrength", "!tag:luckyItem"], "colors": [{"when": ["missingStrength"], "color": "red"}, {"when": ["strengthReduced"], "color": "yellow"}]}
{"text": "@requiredDexterity", "when": ["requiredDexterity", "!tag:luckyItem"], "colors": [{"when": ["missingDexterity"], "color": "red"}, {"when": ["dexterityReduced"], "color": "yellow"}]}

Their texts (English):

"attackSpeed": "Attack speed: {attackSpeed}"
"oneHandedDamage": "One-Handed Damage: {damageMin} ~ {damageMax}"
"requiredDexterity": "Agility Requirement: {requiredDexterity}"
"requiredStrength": "Strength Requirement: {requiredStrength}"

Ring of Ice +2

A resistance line and a level requirement.

Legacy theme Modern theme
Before
After
The layout lines behind it (Tooltip.json)
{"text": "@iceResistance", "when": ["iceResistance"]}
{"text": "@requiredLevel", "when": ["requiredLevel", "!tag:noLevelRequirement"], "colors": [{"when": ["missingLevel"], "color": "red"}]}

Their texts (English):

"iceResistance": "Ice Resistance: {iceResistance}"
"requiredLevel": "Minimum Level Requirement: {requiredLevel}"

Related issues

Step 5b-3 of #815. Builds on #823, #822, #821, #820, #819, #818, #817, #816 and #813.

Checklist

  • I have read and followed docs/CODING_RULES.md.
  • I have built the project locally (see docs/build/README.md).
    • Windows x64 Release: all 1002 tests pass. The new ones are in tests/data/test_item_tooltip_layouts.cpp: the shipped file loads without problems, the reader, its checks, the conditions and the colors. The recorded tooltip test is re-recorded for the 11 items above only.
    • Changed lines pass clang-format 21.1.8 as CI checks them.
    • In game: tooltip-snapshot of two heroes before and after, and the screenshots above in both themes.
  • Changes are focused on one concern; the diff is as small as it reasonably can be.

@sven-n sven-n left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed the new src/source/Data/GameData/ItemData/ItemTooltipLayouts.cpp parser and its JSON layout integration. No confirmed blocking defect identified in the inspected code. Please ensure the tests explicitly exercise invalid references, unknown conditions, missing default layouts, and translated placeholder mismatches, as those are the failure modes introduced by externalizing the stat-line rules. Build not run.

Reviewed with AGENTS.md and docs/CODING_RULES.md as the quality baseline.

@ezequielzacca

Copy link
Copy Markdown
Contributor Author

Thanks. Most of these were covered: "A tooltip file's typos are errors with the name meant" tests a mistyped @reference, unknown and mistyped conditions, an unknown tag and a translation with a mistyped placeholder; "A tooltip file needs the default layout" tests a missing default layout. The gaps are added:

  • A reference that's close to no text names it without a suggestion.
  • A translation that drops the English text's placeholder, adds another, or has a } without its {.
  • A file whose only layout has another name: still the "must have the layout "default"" error, plus the "only the default layout is used" warning.

Tooltips/Tooltip.json holds the item tooltip's shared texts (by name, with
their translations) and its layouts: which damage, defense, speed,
resistance and requirement lines a tooltip shows, each with the conditions
it needs (a built-in value or an item stat table not 0, a state, a tag) and
its colour rules. The loader reads it with the item data, and the checks
report unknown texts, values, states and tags, a translation whose
placeholders differ, and a layout without its sections.

The Scrolls of Teleport, Teleport Ally and Soul Barrier get the new
noDamageLine tag, and the Horn of Fenrir its profile; docs/item-data.md
describes the file. Nothing reads the layouts yet.
RenderItemInfo's damage, defense, magic defense, block, speed, resistance and
requirement lines, and the wizardry, curse and pet attack increases, are now
written from the default layout of Tooltips/Tooltip.json instead of code:
each line's conditions are checked against the item's tooltip values
(Engine::Items::TooltipValues: the values, the states the conditions name,
the conditions and the line colours), and its text, filled with the values,
comes in the UI's language. The 24 texts those lines used move from the
.resx files into the tooltip file.

Every tooltip of both snapshot heroes stays as it was except 11 skill
scrolls, re-recorded on purpose:
- Scroll of Nova and the Berserker, Sleep, Weakness, Wizardry Enhance, Chain
  Drive, Dark Side and Dragon Roar parchments now keep the blank line under
  their name that every other item has. The old code wrote a wizardry damage
  line for them, found their skill does no damage, and stepped back one line
  too many, deleting that blank.
- The Scrolls of Teleport, Teleport Ally and Soul Barrier no longer colour
  the blank line under their name blue when excellent, a side effect of the
  same step back; a blank line's colour doesn't show.
@ezequielzacca
ezequielzacca force-pushed the feat/item-tooltip-layouts branch from 7b36538 to 27dd2b9 Compare October 25, 2026 01:32
@ezequielzacca

Copy link
Copy Markdown
Contributor Author

Caught up with main now that #823 is in, and cleaned up the history: the branch is now two commits on top of main (the layout file with its loader and checks, then the stat lines from the layout) instead of nine commits and 28 merges of the stack.

One deliberate output change, called out in the second commit: 11 skill scrolls in the recorded snapshot.

  • Scroll of Nova and the Berserker, Sleep, Weakness, Wizardry Enhance, Chain Drive, Dark Side and Dragon Roar parchments now keep the blank line under their name that every other item has. The old code wrote a wizardry damage line for them, found their skill does no damage, and stepped back one line too many, deleting that blank.
  • The Scrolls of Teleport, Teleport Ally and Soul Barrier no longer colour that blank line blue when excellent, a side effect of the same step back (a blank line's colour doesn't show).

Every other tooltip of both heroes matches main's recording. Checked: Windows x64 Release with the editor and the control socket, each commit builds, and all 1479 tests pass on the second, including test_item_database_files in random orders.

@Mosch0512 Mosch0512 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: changes requested for one item; the rest is non-blocking. The move to the layout is right:

  • The lines and conditions match the old code. I compared each line of the default layout with the if it replaces:
    • Damage: the harmony colour before the excellent colour, wizardry damage for group 15, and one- or two-handed damage otherwise.
    • Defense: the harmony defense bonus only applies to armour, so defense holds exactly when ip->Defense did.
    • Speeds, magic defense, resistances: same values, same colours.
    • Requirements: level, strength, agility, stamina, energy and charisma, with the "(lacking …)" lines and the yellow reduced colour.
    • Tags for the old item checks: luckyItem and sceptre were already what IsLuckyItemType and IsSceptreType read. summonerBook is staff 21 to 23, all of the old 21 to 29 range that exists. noLevelRequirement is only Loch's Feather.
  • The recorded snapshot agrees. Exactly the 11 scrolls you list changed, and every other tooltip of both heroes is the same.
  • The texts match the old ones in 10 of the 11 languages. I rendered every old .resx string (with the element names for the resistances) and compared it with the layout's text. Only Polish differs (below).
  • The removed ids are safe to remove. Nothing else looks up 66, 68, 72, 1234, 1691 or 1930, by name or by number, apart from the map name fallback you found.

Required

  • Three Polish texts undo main's corrections (inline, Tooltip.json:270).

Non-blocking

  • A Jewel of Harmony reduction to exactly 0 now hides the requirement line (inline, Tooltip.json:355).
  • The docs allow {table:…} in a layout text, but the check refuses it (inline, docs/item-data.md:219).
  • IsValidLocaleCode and ContainsSeparator are back in the item validation (inline, ItemDataValidation.cpp:277).
  • The noDamageLine tag changes nothing (inline, Tooltip.json:337).
  • The layout's names are looked up as strings on every frame (inline, ZzzInventory.cpp:1722).
  • ValidateTooltipTextLocales names its fields differently from the reader (inline, ItemTooltipLayouts.cpp:620).
  • Three untouched blocks are reformatted (inline, ZzzInventory.cpp:3218).

Before merging (not code)

  • The map name note. With ids 66, 68 and 72 removed, map 42's name changes from "%s Resistance: %d" to an empty text (Lookup returns "" for a missing id). The PR text says it's still the resistance text; it's worth saying that this PR changes it.

What I checked:

  • every line of the layout against the old RenderItemInfo branch;
  • the texts of all 11 languages against main's .resx strings;
  • the new tags against the item checks they replace, for every shipped item;
  • every Lookup by number or computed id for the removed ids.

I also added two local tests (not pushed):

  • a layout text with {table:damageIncreaseWeapons} and a "table:…" condition;
  • every weapon and staff at +0 to +15 with harmony options 2 to 4 at levels 0 to 13, looking for a reduced requirement of exactly 0, and the values of the three teleport scrolls.

Built on Windows x64 Debug with the editor and the control socket: test_item_database_files passes whole, 219 of 219, in file order and three random orders, and so do test_item_database, test_control_tooltip_commands and test_control_protocol. I didn't run the game.

"es": "Requisito de carisma: {requiredLeadership}",
"id": "Persyaratan Karisma: {requiredLeadership}",
"ja": "使用可能な統率: {requiredLeadership}",
"pl": "Wymagania dotyczące charyzmy: {requiredLeadership}",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Required. Three Polish texts are the old machine translations that main has since corrected:

Text This PR main's Game.pl.resx
requiredLeadership (line 270) Wymagania dotyczące charyzmy: … Wymagane dowodzenie: … (17fc723)
wizardryDamage (line 36) Czarodziejskie obrażenia: … Obrażenia magiczne: … (a650df6)
wizardryDamageIncrease (line 296) Zwiększenie obrażeń Wizardry …%. Obrażenia magiczne +…% (a650df6)

The texts were probably copied before those two commits reached main. The three .resx entries stay, since other windows use them, so after this PR the tooltip shows the old Polish while those windows show the corrected text. The recorded snapshot runs in English, so no test sees it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1e96041: the three Polish texts are now main's corrected ones: "Wymagane dowodzenie: …", "Obrażenia magiczne: … ~ …" and "Obrażenia magiczne +…%".

{"text": "@lacking", "values": {"missing": "missingLevel"}, "when": ["requiredLevel", "!tag:noLevelRequirement", "missingLevel"], "color": "red"},
{"text": "@requiredStrength", "when": ["requiredStrength", "!tag:luckyItem"], "colors": [{"when": ["missingStrength"], "color": "red"}, {"when": ["strengthReduced"], "color": "yellow"}]},
{"text": "@lacking", "values": {"missing": "missingStrength"}, "when": ["requiredStrength", "!tag:luckyItem", "missingStrength"], "color": "red"},
{"text": "@requiredDexterity", "when": ["requiredDexterity", "!tag:luckyItem"], "colors": [{"when": ["missingDexterity"], "color": "red"}, {"when": ["dexterityReduced"], "color": "yellow"}]},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking. "when": ["requiredDexterity"] reads the requirement after the Jewel of Harmony's reduction. The old code read the item's own requirement (ip->RequireDexterity). So when a reduction brings the requirement to exactly 0, the old tooltip showed "Agility Requirement: 0" in yellow, and the new one shows no line.

A local test over every weapon and staff at +0 to +15, with harmony options 2 to 4 at levels 0 to 13, finds one such item: the Grand Viper Staff (5/10) at +13 to +15 with option 3 (agility) at level 12. Its requirement is 37, and the reduction at level 12 is 37. The snapshot's harmony variant is option 1, so it doesn't cover this.

Hiding a requirement of 0 is arguably better. But it's a visible change the PR says it doesn't make. Either the condition reads the item's own requirement, or the PR text lists it with the 11 scrolls. Strength has the same condition.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1e96041, to keep the old behaviour: the strength and agility lines now show by the item's own requirement (new states strengthRequired and dexterityRequired, set before the reductions). So your Grand Viper Staff case shows "Agility Requirement: 0" in yellow again. A test pins exactly that item: +13, option 3, level 12.

Comment thread docs/item-data.md Outdated
- **`text`**: `"@name"` for a shared text, a text written in place (with its
translations, as in a description), or `""` for a blank line. Its
placeholders are the [built-in values](#description-placeholders), `level`
and the stat file's tables.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking. The docs say a layout text's placeholders are "the built-in values, level and the stat file's tables". But CheckLinePlaceholders only knows the built-ins and the line's values. A local test with {"text": "Bonus {table:damageIncreaseWeapons}"} fails to load:

layouts[0].lines[1].text: has the unknown placeholder {table:damageIncreaseWeapons}; a placeholder is a built-in value (docs/item-data.md) or one the line's "values" names

Conditions are the same: "when": ["table:damageIncreaseWeapons"] is refused as "neither a built-in value, a state nor "tag:"".

The game side would handle both: the fill and AreTooltipConditionsMet fall back to GetPlaceholderValue. Two comments claim tables work as well: the header comment of AreTooltipConditionsMet (ItemTooltipValues.h:76) and the test comment "{level} and the stat file's tables are values too" (test_item_tooltip_layouts.cpp:212). The reader has no stat profiles to check a table name against. So either the docs and both comments drop the tables, or the check is made after the stat file loads, like ValidateDescriptionPlaceholders.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 1e96041: the docs now say a layout's placeholders are the built-ins and what the line's values names, and that the stat file's tables aren't checked here, so a layout can't use them yet. Both comments (AreTooltipConditionsMet's and the test's) no longer claim tables.


// Locale codes like "pt" or "zh-TW". Anything else (empty, "=", "||") would
// break the LocalizedString format used for the OpenMU exchange.
bool IsValidLocaleCode(const std::string& locale)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking. This commit adds IsValidLocaleCode here, and ContainsSeparator at line 29. Main moved both into LocalizedString in 96c8596 (LocalizedString::IsValidLocaleCode, LocalizedString::ContainsSeparator), and the rest of this file calls those. This looks like a leftover from squashing the history:

  • ContainsSeparator is unused. GCC and Clang warn about an unused function in an anonymous namespace.
  • ItemTooltipLayouts.cpp:132 calls this copy.

Dropping both and calling LocalizedString::IsValidLocaleCode there keeps one version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1e96041: both copies are gone, and ItemTooltipLayouts.cpp calls LocalizedString::IsValidLocaleCode. You're right that it was a leftover of the history cleanup.

{"section": "name"},
{"text": "@oneHandedDamage", "when": ["hasDamage", "!tag:noDamageLine", "!skillBook", "!twoHanded"], "colors": [{"when": ["damageHarmonyBonus"], "color": "yellow"}, {"when": ["excellent"], "color": "blue"}]},
{"text": "@twoHandedDamage", "when": ["hasDamage", "!tag:noDamageLine", "!skillBook", "twoHanded"], "colors": [{"when": ["damageHarmonyBonus"], "color": "yellow"}, {"when": ["excellent"], "color": "blue"}]},
{"text": "@wizardryDamage", "when": ["hasDamage", "!tag:noDamageLine", "skillBook"], "colors": [{"when": ["damageHarmonyBonus"], "color": "yellow"}, {"when": ["excellent"], "color": "blue"}]},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking. The noDamageLine tag doesn't hide anything. Since ComputeDamage now takes the skill book path for all of group 15, the three scrolls get their skill's damage, and Teleport, Teleport Ally and Soul Barrier do none. A local test gives hasDamage false for 15/5, 15/14 and 15/15. So "hasDamage" already hides the line, and the layout looks the same without !tag:noDamageLine.

Two comments describe the old state:

  • the hasDamage comment in ItemTooltipValues.h:29 ("the teleport scrolls have it, though their tag noDamageLine hides the line");
  • the tag's docs entry ("though the item has damage").

Either the tag goes (enum value, docs entry and the three data edits), or both comments say it only matters if those skills get damage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 1e96041: the tag is gone (enum value, name, docs entry, the three data edits and the layout's !tag:noDamageLine), and the hasDamage comment says the teleport scrolls' skills do no damage. The recorded snapshot is unchanged without it. The layout test's tag case now uses sceptre.

for (auto line = first == layout->lines.end() ? first : first + 1;
line != layout->lines.end() && line->section == Data::Items::TooltipSection::None; ++line)
{
if (!Engine::Items::TooltipValues::AreTooltipConditionsMet(line->when, item.Type, level, values))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking. RenderItemInfo rebuilds the tooltip every frame while it's shown. For each of the layout's 32 lines, every condition is now found by name: string comparisons through GetItemTooltipValue's tables, then GetPlaceholderValue. Every shown line also builds a std::function, a std::string and a std::wstring. CODING_RULES.md §12 asks to avoid heap allocations and string work per frame. Resolving the condition and placeholder names to indexes when the file loads would leave the per-frame work as it was before.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed it should be resolved at load. Turning the condition and placeholder names into indexes touches the reader, the conditions and the fill, so I'd rather do it as a follow-up PR right after this one than grow this diff. The per-frame work is still bounded by the default layout's 32 lines; I'll open the follow-up once this merges.

{
for (size_t index = 0; index < layout.lines.size(); ++index)
{
warn(layout.lines[index].text, "layouts." + layout.name + ".lines[" + std::to_string(index) + "].text");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking. This names the field layouts.default.lines[3].text, while the reader's messages use layouts[0].lines[3].text. The same line then appears under two names in the log.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1e96041: the locale warnings now name the field like the reader does, layouts[0].lines[3].text.

}
else if (Level == 14)
{
mu_swprintf(TextList[TextNum], I18N::Game::FreeEntranceToKalima);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking. The Kalima ticket lines here and the jewel combination lines at 3249 aren't otherwise changed, but they're reformatted (one statement per line). It's harmless, but it adds unrelated lines to an already large diff.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1e96041: the Kalima ticket and jewel combination lines are back to main's formatting, so they drop out of the diff.

- Polish: the leadership requirement and the two wizardry damage texts are
  main's corrected ones (17fc723, a650df6), not the old translations.
- The strength and agility requirement lines show by the item's own
  requirement (new states strengthRequired, dexterityRequired), as the old
  code did: a Jewel of Harmony reduction to exactly 0 shows "... Requirement:
  0" in yellow again (the Grand Viper Staff at +13 with option 3 at level 12;
  a test pins it).
- The noDamageLine tag goes: the three scrolls' skills do no damage, so
  hasDamage already hides their line; the snapshot is unchanged.
- The docs and two comments no longer claim a layout can use the stat file's
  tables, which the checks refuse.
- IsValidLocaleCode and ContainsSeparator are main's LocalizedString ones.
- The locale warnings name layouts as the reader does (layouts[0]...).
- The Kalima ticket and jewel combination lines keep main's formatting.
@ezequielzacca

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Pushed 1e96041 as a new commit on top (no history rewrite now that the PR has review threads):

  • Required: the three Polish texts are main's corrected ones.
  • Non-blocking, done:
    • a requirement reduced to 0 shows again, as before, with a test;
    • the docs and comments no longer claim tables in layouts;
    • the duplicate locale helpers are gone;
    • the noDamageLine tag is gone (the snapshot is unchanged without it);
    • the field naming matches the reader;
    • the reformatted blocks are back to main's formatting.
  • Follow-up: resolving the layout's names at load, as a separate PR once this one merges.
  • Map names: the PR text now says that maps 36, 38 and 42 lose their resistance-text name and show an empty one.
  • Checked: Windows x64 Release with the editor and the control socket, all 1480 tests pass, and test_item_database_files passes in three random orders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants