Repository navigation
feat(items): the tooltip's stat lines come from a tooltip layout - #824
ezequielzacca wants to merge 3 commits into
Conversation
sven-n
left a comment
There was a problem hiding this comment.
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.
|
Thanks. Most of these were covered: "A tooltip file's typos are errors with the name meant" tests a mistyped
|
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.
7b36538 to
27dd2b9
Compare
|
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.
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 |
Mosch0512
left a comment
There was a problem hiding this comment.
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
defaultlayout with theifit 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
defenseholds exactly whenip->Defensedid. - 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:
luckyItemandsceptrewere already whatIsLuckyItemTypeandIsSceptreTyperead.summonerBookis staff 21 to 23, all of the old 21 to 29 range that exists.noLevelRequirementis 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
.resxstring (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). IsValidLocaleCodeandContainsSeparatorare back in the item validation (inline, ItemDataValidation.cpp:277).- The
noDamageLinetag changes nothing (inline, Tooltip.json:337). - The layout's names are looked up as strings on every frame (inline, ZzzInventory.cpp:1722).
ValidateTooltipTextLocalesnames 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 (
Lookupreturns "" 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
RenderItemInfobranch; - the texts of all 11 languages against main's
.resxstrings; - the new tags against the item checks they replace, for every shipped item;
- every
Lookupby 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}", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"}]}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| - **`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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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:
ContainsSeparatoris unused. GCC and Clang warn about an unused function in an anonymous namespace.ItemTooltipLayouts.cpp:132calls this copy.
Dropping both and calling LocalizedString::IsValidLocaleCode there keeps one version.
There was a problem hiding this comment.
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"}]}, |
There was a problem hiding this comment.
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
hasDamagecomment 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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thanks for the review. Pushed 1e96041 as a new commit on top (no history rewrite now that the PR has review threads):
|
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.jsonholds 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 thedefaultlayout: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
when: a list, and every entry must hold. An entry is one of:excellent,twoHanded,skillBook, …);tag:sceptre).A
!negates an entry.colors: rules where the first that holds wins; otherwise the line'scolor.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 betweenname,description,classesandoptions.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:@text, condition, tag, color or placeholder;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,sceptreandsummonerBooktags 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:.resx
The texts only these lines used are gone from every language:
Spell resistance: %dMoving speed: %d%s Resistance: %dStamina Requirement: %dIncrease 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 (Lookupreturns "" 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 itshasDamagedoesn't hold.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):
Legendary Staff +9
Damage, attack speed, a requirement, then the bold blue wizardry damage increase after the classes.
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):
Book of Samut
A summoner book: a red requirement with its "(lacking …)" line, and the curse increase (
tag:summonerBook) instead of the wizardry one.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):
Great Scepter +9
The sceptre's pet attack increase, from
tag:sceptre.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):
Blade +11
A plain weapon: damage, speed and two requirements.
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):
Ring of Ice +2
A resistance line and a level requirement.
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):
Related issues
Step 5b-3 of #815. Builds on #823, #822, #821, #820, #819, #818, #817, #816 and #813.
Checklist
docs/CODING_RULES.md.docs/build/README.md).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.tooltip-snapshotof two heroes before and after, and the screenshots above in both themes.