Repository navigation
refactor(items): the wings', capes' and pets' tooltip values come from their data - #818
Conversation
…m their data
The damage increase and absorption the tooltip code computed for the
wings, capes and pets are their descriptions now, with placeholders over
11 new level bonus tables of Stats/ItemStats.json (wingDamageIncrease1st
is 12 + 2 per level, ...): 24 wings and capes, plus Horn of Dinorant,
Guardian Angel and Imp, whose fixed numbers are written in the text.
Their cases leave RenderItemInfo; the two .resx texts no code uses any
more go too (the shared "Increase %d%% of Damage" and "Increase speed"
stay, other code prints them).
The printf texts become data as the game showed them: %d is the
placeholder or the number, %% is %. Two translations of the Imp's text
had broken escapes ("% %", shown as %; Portuguese "% o", read as an
octal number); they read as plain text now.
A table no stat uses is no longer a warning of the stat file alone: it
can be one for descriptions, so ValidateDescriptionPlaceholders warns
about a table that neither a stat nor a description uses.
A test compares every one of the 27 items at every level from +0 to +15
with the old formulas.
…at/item-tooltip-values-to-data # Conflicts: # docs/item-data.md # src/Localization/Game.pl.resx # src/source/Data/GameData/ItemData/ItemStatProfiles.cpp # src/source/Data/GameData/ItemData/ItemStatsJson.cpp
…at/item-tooltip-values-to-data # Conflicts: # docs/item-data.md
…at/item-tooltip-values-to-data
…at/item-tooltip-values-to-data # Conflicts: # docs/item-data.md
…at/item-tooltip-values-to-data # Conflicts: # docs/item-data.md
…at/item-tooltip-values-to-data # Conflicts: # docs/item-data.md # src/source/Data/GameData/ItemData/ItemDataValidation.cpp # src/source/Data/GameData/ItemData/ItemDataValidation.h
Mosch0512
left a comment
Mosch0512
left a comment
There was a problem hiding this comment.
Verdict: changes requested for two items; the rest is non-blocking. The move itself is right:
- Values: the tables give exactly what the old formulas printed, for all 27 items at every level from +0 to +15. The comparison test covers every shipped item with a wing tier.
- Tooltip order: unchanged. The Cape of Fighter and Cape of Lord lines were the last link of the same chain, and the rest of that chain only matches other items.
- Translations: apart from the Polish speed text below, every language matches what printf showed, including the Imp's Spanish and Russian
% %. The Portuguese% ois the deliberate fix you describe. - Removed texts: nothing reads ids 576 and 739 any more; server command 4's
725 + Cmd2only covers the Rena texts 725–727. - Warning routing: the "unused table" warning still reaches Item Formulas from Item Checks, because its source is the stat file.
Required
- Wing tiers. The second-tier, third-tier and small wings' lines used to follow the item's
wingTierin the data. Now they come only from each item's description, so a custom or re-tiered wing loses them or keeps the old tier's values (inline, ZzzInventory.cpp:3805). - Polish "Increase speed". It changed in all 22 wings, so the game now spells this line two ways in Polish (inline, Group12_Wing.json:52).
Non-blocking
- The comparison test checks English only, though the remaining
.resxkeys would let it check every language (inline, test_item_database_files.cpp:303). - The Imp's Portuguese text now shows its broken "de % of" (inline, Group13_Helper.json:82).
- The unused-table warning lives inside the placeholder check (inline, ItemDataValidation.cpp:423).
- The same three lines are copied into 22 items until the shared texts of #827 (inline, Group12_Wing.json:123).
Before merging (not code)
- The PR description is outdated. It still shows the placeholders without
table:({wingDamageIncrease1st}, and the Wing of Storm's JSON), while the second commit writes them as{table:wingDamageIncrease1st}. - The branch needs updating. It conflicts with main, and it doesn't have #817's three newest commits yet (0a8ac43, 4022b75, 1b899b1). The hint fix touches the same lines in
ItemDataValidation.cpp, which this branch has unchanged, so it should merge cleanly.
What I checked:
- this step's two commits against #817, and the chain around the removed cases;
- the wing tier helpers the old code used;
- every item with a wing tier in the shipped data, against the comparison test;
- each language of every moved text, against printf of the old
.resxformats; - every
Lookup()base near the removed ids; - how Item Checks routes the moved warning.
I didn't build this branch or run the game.
| TextListColor[TextNum] = TEXT_COLOR_WHITE; TextBold[TextNum] = false; TextNum++; | ||
| } | ||
| else if (ip->Type == ITEM_SPIRIT) | ||
| if (ip->Type == ITEM_SPIRIT) |
There was a problem hiding this comment.
Required. The removed branches for the second-tier, third-tier and small wings weren't hard-coded: IsSecondTierWingExceptCape, IsThirdTierWing and IsSmallWing read the item's wingTier (and its cape tag) from the item data. Now the lines come only from each item's own description, so the tier no longer decides them. That stays so through the rest of the stack: at #836 each wing still names its own tables.
Two cases break:
- A custom wing. A server adds a wing with
"wingTier": "third". Before, its tooltip showed "Increase 39+2L% of Damage", "Absorb 39+2L% of Damage" and "Increase speed" with no extra data. Now it shows none of them unless its description repeats the three lines. - A re-tiered wing. Changing a wing's tier in the data moves its stat profile, since the profile rules follow the tier, but leaves its tooltip on the old tier's tables. The two then disagree without any message.
Either of these would close the gap:
- a check that an item with a wing tier names that tier's tables, as a warning or an error;
- tooltip lines conditioned on the wing tier, once the layout steps can express that.
| "es": "aumentar la velocidad", | ||
| "id": "Tingkatkan kecepatan", | ||
| "ja": "移動速度向上", | ||
| "pl": "Zwiększ prędkość", |
There was a problem hiding this comment.
Required. All 22 "Increase speed" lines say "Zwiększ prędkość" in Polish. But the .resx key the old code printed, "Increase speed" (I18N::Game::IncreaseSpeed), is "Zwiększa prędkość". That key stays in the game: the tooltip code still prints it at ZzzInventory.cpp:4640 and 4650. So Polish players now see the line in two spellings, "Zwiększ" ("Increase!") on wings and "Zwiększa" ("Increases") elsewhere. The description also says the translations are otherwise unchanged, including their wording. Using "Zwiększa prędkość" in the 22 items keeps it as it was.
| return {}; | ||
| } | ||
|
|
||
| TEST_CASE("The wings', capes' and pets' tooltip values come from their data as from the old formulas [data][items]") |
There was a problem hiding this comment.
Non-blocking. This compares the English text only. Three of the formats stay in .resx, because other code prints them: "Increase %d%% of Damage", "Absorb %d%% of Damage" and "Increase speed". So the test can compare every language: printf the locale's format with the expected value, and compare it with the filled description line. That would have caught the Polish speed text, and it keeps guarding the translations as later steps copy them.
| "id": "Meningkatkan 30% of serangan & Dmg Sihir", | ||
| "ja": "攻撃力、力 30%増加", | ||
| "pl": "Zwiększa obrażenia fizyczne i magiczne o 30%", | ||
| "pt": "Aumenta em 30% o dano de ataque e magia de % of", |
There was a problem hiding this comment.
Non-blocking, follow-up. The Portuguese Imp text is now shown as written: "… de ataque e magia de % of". Before, printf read % o as an octal conversion with no argument and printed garbage, so it was broken either way. As data, it's now a one-line fix ("Aumenta em 30% o dano de ataque e magia"). The Imp text keeps an English "% of" in es, ru, id, tl, uk and zh-TW too, so it could join the "of" fixes of #828.
| if (!profiles.IsTableUsedByStats(table) && !named.contains(table)) | ||
| { | ||
| issues.push_back({ItemDataIssueSeverity::Warning, statFile, ItemDataIssue::NoItem, ItemDataIssue::NoItem, | ||
| "tables." + table, "neither a stat nor an item description uses this table"}); |
There was a problem hiding this comment.
Non-blocking. The stat file's "unused table" warning now lives in ValidateDescriptionPlaceholders. The function name no longer says what it does, and callers that only want the name check get stat file warnings too: the unit tests now filter out NoItem issues for that reason. A separate ValidateTableUses(items, profiles, statFile, issues), called where this one is (Load, Save Items, bmd import), keeps both jobs visible.
| "es": "aumentar la velocidad", | ||
| "id": "Tingkatkan kecepatan", | ||
| "ja": "移動速度向上", | ||
| "pl": "Zwiększ prędkość", |
There was a problem hiding this comment.
Non-blocking. The same three lines, each with 11 languages, are written out in all 22 wings and capes: about 1,000 lines of JSON, where only the table names differ by tier. A translation fix, like the Polish one above, has to be made in 22 places per language until #827's shared texts land. That's fine as a step; a line about it in the description would help.
…ip-values-to-data # Conflicts: # tests/data/test_item_description_placeholders.cpp
… names its tier's tables The unused-table warning moves out of the placeholder check into ValidateTableUses, run where that check is (Load, Save Items, bmd import). It also warns about a wing whose description names other tables than most of the other wings of its tier, cape tag and group: the tooltip code chose the wings' damage and absorption lines by the tier, so a new wing without them, or a re-tiered wing that keeps its old tier's tables, is pointed out.
…t are the game's "Zwiększa prędkość" as the game's "Increase speed" text has it, and the Imp's Portuguese text without its broken "de % of".
…he gate's lines are formatted
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
Reviewed the wing/cape/pet tooltip value migration, including src/source/Engine/Object/ZzzInventory.cpp and ItemDescriptionPlaceholders.cpp. No independently actionable defect identified in the inspected code. The value tables and localized placeholder output are especially worth checking in-game for level 0 and 15, and for the distinct cape absorption formulas. I did not run the build or gameplay tests.
Reviewed with AGENTS.md and docs/CODING_RULES.md as the quality baseline.
|
Thanks! All addressed in this PR, and main is merged in (with your three #817 commits): Required
Non-blocking
The description shows |
…s own item table Run in one process after a test that loaded the item data (the MU Helper pickup filter tests do), the comparison test crashed: it switches the UI language for every language, the item data follows each switch and refills ItemAttribute, which is null outside a ClientDataScope. CTest runs each test case in its own process, so CI did not show it. The test now runs inside a ClientDataScope, as ClientDataScope's comment asks of a test that switches the language.
|
Thanks, the fixes look good: the Polish speed text, the every-language comparison (it fails on the old Polish text) and the Imp's Portuguese text. The new wing check catches both cases from the review, and the shipped data gives no warnings. A test crash, fixed in 9980fdb. Please pull before your next push.
The wing check: remaining gaps, all non-blocking
|
… a tie, and checks a lone wing against the other groups Following the review: a wing is compared by its lines (their texts, tables included), so one that loses or gains a line without a table is pointed out too; wings that split evenly get one warning that they don't agree, so neither side is blamed; wings without lines count as a set of lines; and a wing alone in its tier, cape tag and group is compared with its tier's wings in the other groups that have more than one (Cape of Fighter and Cape of Lord, each alone, aren't compared with each other). The warning is in the docs, and the stat file test's comment names ValidateTableUses.
|
Thanks! Pulled 9980fdb, and the wing check now covers the gaps (b706374):
Tests cover the speed line, both ties, the lines-less tie, a lone group-13 wing, and Fighter/Lord staying apart. The whole test binary passes, and CI's quality gate passes locally. Carried through the stack to #836. |
The wing check took the lines most wings of a tier have as the ones the others are measured by, and wings without lines counted as one set of lines. New wings imported without their lines (a bmd import gives new items none), more of them than the wings with lines, then made "no lines" the majority: the wings with the right lines were pointed out and the new ones were not. Wings without lines no longer set the lines the others are compared with, so the new ones are the ones pointed out; a tie among the wings with lines is still reported as such.
Summary
Step 4 of #815 (data-driven item tooltips). #817, #816 and #813 are merged, so this PR's diff is this step only.
The damage increase and absorption that
RenderItemInfo()computed for wings, capes and pets come from the item data now. They are descriptions with placeholders (#817) over new level bonus tables inStats/ItemStats.json:wingDamageIncrease1st12 + 2/levelwingDamageAbsorb1st12 + 2/levelwingDamageIncrease2nd32 + 1/levelwingDamageAbsorb2nd25 + 2/levelwingDamageIncrease3rd39 + 2/levelwingDamageAbsorb3rd39 + 2/levelwingDamageIncrease3rdcapeDamageAbsorb3rd24 + 2/levelcapeDamageIncrease20 + 2/levelsmallCapeDamageAbsorb20 + 2/levelcapeDamageIncreasecapeOfFighterDamageAbsorb10 + 2/levelcapeDamageIncreasecapeOfLordDamageAbsorb10 + 1/level{"values": <+0 value>, "perLevel": <per level>}.DamageIncrease2ndWingsand similar hold only the per-level factor (0.01 × level), so I used names OpenMU doesn't have, to avoid mixing the two up in a later item exchange.%dis the placeholder (or the fixed number),%%is%.% %, which printf showed as%, and Portuguese% o, which printf read as an octal number with no argument. They read as plain text now.ZzzInventory.cpp(88 lines);.resxtexts no code uses any more, "Max HP +%d increased" (legacy id 739) and "Increase 30%% of attacking & Wizardry Dmg" (576). NoLookup()range reaches those ids.ValidateTableUses, run at Load, in Save Items and in the bmd import, next to the placeholder check):wingTier(andcapetag). Now each wing names its tables, so a wing whose description names other tables than most of the other wings of its tier, cape tag and group is a warning: a new third-tier wing without the lines, or a wing re-tiered in the data that keeps its old tier's tables, gets "names no table for its wing lines; the other third-tier wings of group 12 name {table:wingDamageAbsorb3rd}, {table:wingDamageIncrease3rd}". Cape of Fighter (12) and Cape of Lord (13) are apart by their group, as the old code had them..resxtexts other code still prints ("Increase %d%% of Damage", "Absorb %d%% of Damage", "Increase speed"), filled as printf fills them.Nothing changes in game.
Not in this PR
The other cases of that chain are fixed texts that depend on the item level: Town Portal scroll, Spirit, Loch's Feather, Fruits, Siege Potion, Contract Summon, Life Stone, Rena. Three plain ones are in the same chain too: Scroll of Archangel, Blood Bone and Invisibility Cloak. They need a per-line level condition and come next, as step 4b. The Armor of Guardsman's Chaos Castle table and the Lost Map / Symbol of Kundun stay code (they read the hero and other tables).
Screenshots
Before (#817) and after, the same. The values come from the data now.
Wing of Storm +15 (69 = 39 + 2 × 15):
The item data of the "after" images (12,36)
src/bin/Data/Items/Group12_Wing.json:{ "number": 36, "name": { "en": "Wing of Storm", "de": "Sturmflügel", "es": "Ala de Tormenta", "pt": "Asa da Tempestade" }, "tags": ["valuable"], "description": [ { "text": { "en": "Increase {table:wingDamageIncrease3rd}% of Damage", "de": "Erhöht den Schaden um {table:wingDamageIncrease3rd}%", "es": "Aumenta el daño de {table:wingDamageIncrease3rd}% of", "id": "Meningkatkan Kerusakan {table:wingDamageIncrease3rd}% of", "ja": "ダメージ {table:wingDamageIncrease3rd}%増加", "pl": "Zwiększa obrażenia o {table:wingDamageIncrease3rd}%", "pt": "Aumentar o dano de {table:wingDamageIncrease3rd}% of", "ru": "Увеличение урона {table:wingDamageIncrease3rd}% of", "tl": "Dagdagan ang {table:wingDamageIncrease3rd}% of Pinsala", "uk": "Збільшення шкоди на {table:wingDamageIncrease3rd}% of", "zh-TW": "增加 {table:wingDamageIncrease3rd}% of 傷害" } }, { "text": { "en": "Absorb {table:wingDamageAbsorb3rd}% of Damage", "de": "Absorbiert {table:wingDamageAbsorb3rd}% des Schadens", "es": "Absorber daño {table:wingDamageAbsorb3rd}% of", "id": "Menyerap Kerusakan {table:wingDamageAbsorb3rd}% of", "ja": "ダメージ {table:wingDamageAbsorb3rd}%吸収", "pl": "Pochłania {table:wingDamageAbsorb3rd}% obrażeń", "pt": "Absorver Dano {table:wingDamageAbsorb3rd}% of", "ru": "Поглощение {table:wingDamageAbsorb3rd}% of урона", "tl": "Isipsip ang {table:wingDamageAbsorb3rd}% of Pinsala", "uk": "Поглинання шкоди {table:wingDamageAbsorb3rd}% of", "zh-TW": "吸收{table:wingDamageAbsorb3rd}% of傷害" } }, { "text": { "en": "Increase speed", "de": "Erhöht die Geschwindigkeit", "es": "aumentar la velocidad", "id": "Tingkatkan kecepatan", "ja": "移動速度向上", "pl": "Zwiększa prędkość", "pt": "Aumentar a velocidade", "ru": "Увеличить скорость", "tl": "Dagdagan ang bilis", "uk": "Збільште швидкість", "zh-TW": "提高速度" } } ] … }The tables of
src/bin/Data/Items/Stats/ItemStats.jsonit names:[ { "name": "wingDamageAbsorb3rd", "caption": {"en": "Damage Absorption in % (3rd Wings)"}, "values": 39, "perLevel": 2 }, { "name": "wingDamageIncrease3rd", "caption": {"en": "Damage Increase in % (3rd Wings and Capes)"}, "values": 39, "perLevel": 2 } ]Guardian Angel:
The item data of the "after" images (13,0)
src/bin/Data/Items/Group13_Helper.json:{ "number": 0, "name": { "en": "Guardian Angel", "de": "Schutzengel", "es": "Angel Guardián", "pt": "Anjo guardião" }, "description": [ { "text": { "en": "Absorb 20% of Damage", "de": "Absorbiert 20% des Schadens", "es": "Absorber daño 20% of", "id": "Menyerap Kerusakan 20% of", "ja": "ダメージ 20%吸収", "pl": "Pochłania 20% obrażeń", "pt": "Absorver Dano 20% of", "ru": "Поглощение 20% of урона", "tl": "Isipsip ang 20% of Pinsala", "uk": "Поглинання шкоди 20% of", "zh-TW": "吸收20% of傷害" } }, { "text": { "en": "Max HP +50 increased", "de": "Max. HP +50 erhöht", "es": "HP máximo +50 aumentado", "id": "HP Maks +50 meningkat", "ja": "最大生命力 +50増加", "pl": "Zwiększono maksymalne HP +50", "pt": "HP máximo +50 aumentado", "ru": "Макс. HP +50 увеличен.", "tl": "Tumaas ang Max HP +50", "uk": "Макс HP +50 збільшено", "zh-TW": "最大生命值+50增加" } } ] … }Related issues
Step 4 of #815. Builds on #817, #816 and #813.
Checklist
docs/CODING_RULES.md.docs/build/README.md).