[ezequielzacca]

This step's own changes: compare with #821. 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-1, continued, of #815 (data-driven item tooltips; design). Stacked on #821 (→ #820 → … → #813): only the last commit, test(items): every item's tooltip in a test, against recorded hashes, is this step.

#821 records every item's tooltip in the game through the control socket. This PR does the same in a test, so CI checks every tooltip before the stat lines move into data (5b-3).

What the test needed

I ran the tooltip builder in a test and gave it what it reached for. It needed less than a full context:

• The hero is the globals CharacterAttribute / Hero. A test points them at its own hero, as test_item_stats.cpp already does.
• The game data is loaded as the game loads it: skills, socket items, item sets, monster names (the Transformation Ring's names), the Jewel of Harmony and add option tables, and the buff system's time texts.
• Two seams in the game code:
  • Making an item: FillItemFromParameters is the part of the item manager's CreateItemByParameters that needs no item list or key, and the manager calls it.
  • The inventory window: without it, an item counts as worn. That's what an item the inventory grid doesn't hold always counted as.
• Text width: the description lines wrap by the font, which a test doesn't have, so SetItemTooltipTextMeasure lets a test use a fixed width instead.

The test

"Every item's tooltip is as recorded" builds the 60,736 tooltips of tooltip-snapshot for two heroes:

• a new Dark Wizard, who lacks every requirement;
• a level 400 Blade Master, a third class who meets most of them.

It compares one hash per item with tests/data/item_tooltip_snapshot.txt (1,898 lines, 66 KB). When an item changes, the test prints its new lines:

ERROR: CHECK( false ) is NOT correct!
  logged: bladeMaster 14/13 now:
14/13 plain +0
  0 <blank>
  3 bold Jewel of Bless
  0 <blank>
  0 It is used to increase your item level up to 7

(That's the Jewel of Bless's description changed by hand to check the test; the change isn't part of this PR.) After a change that's meant, MU_TOOLTIP_SNAPSHOT_UPDATE=1 records the new hashes, and the PR's diff of the file shows which items changed. The snapshot code is shared with tooltip-snapshot (Engine/Object/ItemTooltipSnapshot), so the game's lines are one request away for comparison.

Checked against the game

For the same hero (test0Dw on a local OpenMU), the test's snapshot equals the game's, apart from the wrapped description lines (the font). That comparison found one bug:

• Transformation Ring above +5: SommonTable has the six monsters of +0 to +5, and a higher level read past its end. The name was then whatever memory held there: "Bull Fighter" at +7 and +13 in the game, and other monsters in the test.
• The fix: IsTransformationRingMonster checks the level. Such a ring has no monster, so its name is empty, as it already was at the other levels above +5.

Apart from that fix, the game's snapshots of both heroes before and after this PR are identical.

Related issues

Step 5b-1 of #815. Builds on #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 990 tests pass, the new one included.
  • The editor build with the control socket builds, and its snapshots were compared as above.
  • Changed lines pass clang-format 21.1.8 as CI checks them.
  • Linux and macOS: not run here. The hashes are a stable hash of the lines, and the test doesn't read the font, so they should be the same, but CI will tell.
• Changes are focused on one concern; the diff is as small as it reasonably can be.

[sven-n]

Snapshot test gap: src/source/Engine/Object/ItemTooltipSnapshot.cpp, WriteItemTooltipSnapshot creates a Cartesian product of every item and plain/full/ancient/harmony variants at every level. Most combinations are not valid in normal gameplay (e.g. excellent options on items without excellent options), while important real-world variants such as different durability values, timed items, and individual option flags are not represented by the four default variants. Since this snapshot is intended to catch behavioral regressions while moving tooltip rules to JSON, please explicitly cover representative supported variants and add targeted assertions for conditional lines (not only a hash of the default combinations). In particular, a tooltip regression that only appears at low durability would not be detected by this baseline. Reviewed against AGENTS.md / CODING_RULES.md; tests not run; this is a coverage concern, not a demonstrated bug.

[ezequielzacca]

Thanks, agreed. Both parts are in 72eca0f (plus 6df23b2):

• More variants. The snapshot now records eleven variants instead of four: the four from before, plus luck, skill, the +1 option and one excellent option, each alone; durability 1 and 0 (a worn and a broken item, and a stack of one or none); and an expired item. Each item is still recorded at every level for both heroes. A timed item that isn't expired is left out, because it shows its end date in local time, which depends on the machine's time zone.

• Targeted assertions. A new test, "A tooltip's durability, count and expiry lines follow the item", checks those lines directly:

  • the Kris's durability line at full, 1 and 0, against the same maximum;
  • an Apple stack's count at 3 and 1;
  • an expired item's red "Expired Item" line, with no durability line;
  • a timed item's orange "Expiration Day" line and its blue end date (computed with localtime, as the game does), with no durability line;
  • no tooltip at all for a timed item without its end date.

  It checks the English texts written out, so it holds after later PRs in the stack move them out of the .resx.

On the combinations that can't happen in the game, such as excellent options on an item that has none: I kept them. They don't cost much, and they record what the tooltip does with whatever data the server sends.

Every later PR in the stack is re-recorded with the eleven variants. For each one, I compared which items change against the old four-variant recording, and the sets are the same. Where a whole snapshot changed (#825 added its own two variants, #829 changed how blank lines are written), I diffed the full tooltip text instead. Only the changes those PRs describe show up. #825's own count and timed variants are now covered by these eleven.

[ezequielzacca]

Caught up with main now that #821 is in (e9491df37):

• The merge: refactor(items): build the item tooltip's lines apart from drawing them #821's tooltip-snapshot fixes moved into this PR's WriteItemTooltipSnapshot structure: group is checked before out is opened, out is resolved like screenshot's, and the reply names the absolute path.
• A random-order crash fixed: test_item_database_files crashed in "Every item's tooltip is as recorded" when "A tooltip's durability, count and expiry lines follow the item" ran first. Both load the monster names, and OpenMonsterScript appended without resetting its count: the second load wrote 678 more names past the 1024-row MonsterScript table. The loader now starts at zero and stops at the table's end. The game loads the names once, so nothing changes in game.
• Checked: all 1285 tests pass (Windows x64 Release, editor and control socket on), and the binary passes whole in file order and in six random orders (seeds 7, 11, 23, 42, 99, 1234).

[Mosch0512]

Verdict: changes requested for one item; the rest is non-blocking. A test of every tooltip is the right safety net for 5b, and it holds up well:

• The recording is sound. I ran the snapshot test in a Windows Debug build, with a local guard for the item below. Every recorded hash matched your Release recording except 13/70, for both heroes, so the hashes don't depend on the build type.
• The set-file fix is right. I decoded the old and new itemsetoption_*.bmd. Only the 8 names changed, each is now valid UTF-8 and fits its 64-byte field, every other byte of the records is the same, and all six checksums are valid. That includes Portuguese and Spanish, which the test doesn't load.
• The Transformation Ring fix covers every use of SommonTable; all of them now go through IsTransformationRingMonster.
• Other fixes: the Weapon of Archangel's values are now initialized, and the stack items' key 0 never matches an inventory item, since keys start at 1.
• The monster names: I found the same OpenMonsterScript overflow, and e9491df fixes it: the count starts at zero and stops at the table's end. Thanks for catching it.
• #821's fixes survived the merge into WriteItemTooltipSnapshot.

Required

• The Talisman of Mobility's "durability 1" line asserts in Debug and records a leftover row in Release (inline, ItemTooltipSnapshot.cpp:59).

Non-blocking

• The expiry date is read through a cast that includes RefCount on Windows; the new test can't see it (inline, test_item_tooltips.cpp:356).
• TooltipDataScope leaves the item sets, socket items and monster names loaded (inline, test_item_tooltips.cpp:73).
• The new file's free functions aren't in a namespace, as CODING_RULES.md asks for new files (inline, ItemTooltipSnapshot.h:10).
• The snapshot test keeps every tooltip's text in memory (inline, test_item_tooltips.cpp:240).
• SetItemTooltipTextMeasure splits BuildItemTooltipLines from its comment (inline, ZzzInventory.h:239).
• GetItemName leaves raw buffers unwritten for a prize past the list (inline, ZzzInventory.cpp:1189).

What I checked:

• the snapshot and durability tests, alone and in whole-binary runs of test_item_database_files, in file order and in random orders (Debug);
• the set files, decoded byte by byte, with the game's checksum;
• every use of SommonTable, and how the builder reads the portal position and the expiry time;
• what TooltipDataScope loads and what it restores.

Built on Windows x64 Debug at f6896ff, before the merge of main and e9491df; I read e9491df but didn't rebuild after it.

[Mosch0512]

Required. With this variant, the Talisman of Mobility (13/70) at durability 1 reaches g_PortalMgr.GetPortalPositionText(TextList[TextNum]) (ZzzInventory.cpp:3550). That function asserts when no position is saved, and a test never has one (PortalMgr.cpp:97).

• Debug: the assert aborts the test binary. "Every item's tooltip is as recorded" ends with SIGABRT in every run, and every test after it is skipped.
• Release: the assert is compiled out and nothing is written, so the row keeps whatever the previous tooltip left there. The recorded hash for 13/70 contains another tooltip's line. With a local guard, my Debug snapshot matched every recorded hash except 13/70, for both heroes.

The Talisman of Resurrection's revive-position line (ZzzInventory.cpp:2621) checks g_PortalMgr.IsRevivePositionSaved() first. Guarding this line with g_PortalMgr.IsPortalPositionSaved() the same way and re-recording 13/70 fixes both.

[ezequielzacca]

Fixed in 3d3e7ec: the position line is now guarded by g_PortalMgr.IsPortalPositionSaved(), like the Talisman of Resurrection's. I re-recorded the snapshot: only 13/70 changed, for both heroes (two rows in item_tooltip_snapshot.txt), which matches your Debug run.

[Mosch0512]

Non-blocking, pre-existing. The builder reads this value as localtime((time_t*)&(ip->lExpireTime)) (ZzzInventory.cpp:1730). On Windows, long is 4 bytes and time_t 8, and ITEM is packed, so the cast also reads the next field, int RefCount, as the time's high half.

This test's item comes from FillItemFromParameters, which leaves RefCount at 0, so it sees the right date. An item from the item manager has RefCount 1, so in the game a timed item's "Expiration Day" would show a date about 136 years late (2^32 seconds). The test says it computes the date "as the game does", but it can't see the difference. const time_t expireTime = ip->lExpireTime; ExpireTime = localtime(&expireTime); fixes it.

[ezequielzacca]

Fixed in 3d3e7ec, in the builder: const time_t expireTime = ip->lExpireTime; ExpireTime = localtime(&expireTime);. The test's timed item now comes from a CItemMng (CreateItemByParameters), so it has RefCount 1 like a game item. I put the old cast back to check: the date line then fails.

[Mosch0512]

Non-blocking. The scope restores the harmony and add-option pointers, the buff system and the skill table it made, and the language. But the item sets (g_csItemOption), the socket items (g_SocketItemMgr) and the monster names stay loaded after the test. When another test had already set SkillAttribute, the skills are also loaded into that test's array.

Later tests then see data they didn't load; for example, the usage test's comment "No set file is loaded here, so the set numbers are all 0" no longer holds after these tests. It passed in my runs, but leftovers like these caused the order-dependent failures earlier in the stack (#847, #854). Restoring them, or noting in the comment that these tests load them for good, keeps it visible.

[ezequielzacca]

In 3d3e7ec I went with the comment: the scope's comment now lists what it restores and says the item sets, socket items and monster names stay loaded, since the game has no way to unload them and each load replaces what was there (with e9491df, the monster names too). I reworded the usage test's comment so it no longer assumes no set file is loaded; the Kris is in no ancient set either way.

[Mosch0512]

Non-blocking. These are free functions extracted from ControlCommandsQuery.cpp into a new file. CODING_RULES.md, "Namespaces and file names for extracted free functions (new files only)", asks for them to be wrapped in a <Layer>::<Concern> namespace, e.g. Engine::Items::TooltipSnapshot.

[ezequielzacca]

Done in 3d3e7ec: FullItemParameters, TooltipOfParameters, WriteItemTooltipSnapshot and the new VisitItemTooltipSnapshots are in Engine::Items::TooltipSnapshot; the control commands and the tests call them qualified.

[Mosch0512]

Non-blocking. tooltips keeps the full text of every item's tooltips for both heroes: 2 × 911 × 11 × 16 = 320,672 tooltips. Each hero's ostringstream holds them once more, and all of that is only to print up to five changed items on failure. Hashing each item as it is written, and building the text again only for the items that changed, would keep the memory flat. The test takes 16 to 20 s alone in a Debug build.

[ezequielzacca]

Done in 3d3e7ec: VisitItemTooltipSnapshots hands out one item's text at a time (WriteItemTooltipSnapshot is built on it). The test loads the recorded hashes first, hashes and compares each item as it comes, and keeps the text only for the first five changed items, so it holds one item's tooltips at a time.

[Mosch0512]

Non-blocking. This declaration was inserted between BuildItemTooltipLines' comment (line 235) and its declaration. That comment now reads as a second comment of SetItemTooltipTextMeasure, and BuildItemTooltipLines has none. In the .cpp, RenderItemInfo's comment "(sx, sy) in the screen's 640x480 stretch" also sits orphaned above s_itemTooltipTextMeasure.

[ezequielzacca]

Fixed in 3d3e7ec and f783329: SetItemTooltipTextMeasure and its comment come first, then BuildItemTooltipLines with its own. In the .cpp, the orphaned "(sx, sy) in the screen's 640x480 stretch" line was RenderItemInfo's comment; it's back above RenderItemInfo now.

[Mosch0512]

Non-blocking. For a prize past ChaosEventName, nothing is written now, and the comment counts on "the caller's fallback". Only the std::wstring overload has one, though. The raw-buffer callers (WSclient.cpp, MixMgr.cpp, QuestMng.cpp, NPCQuest.cpp) keep whatever their buffer held. The tooltip writes p->Name in the same case, and doing the same here keeps the two alike. The shipped 14/12 has durability 0, so this is only reachable with custom data.

[ezequielzacca]

Done in 3d3e7ec: past the list it writes p->Name, as the tooltip does, so the raw-buffer callers get the item's name too.

[ezequielzacca]

Thanks for the thorough review. Pushed 3d3e7ec:

• Required: the Talisman of Mobility's position line is guarded by IsPortalPositionSaved(); only 13/70 was re-recorded (both heroes).
• All six non-blocking points done: the expiry-time cast (with a test that now catches it), what TooltipDataScope leaves loaded, the Engine::Items::TooltipSnapshot namespace, the snapshot test holding one item at a time, the comment order, and the prize name.
• Checked: Windows x64 Release with the editor and the control socket: all 1285 tests pass, and test_item_database_files passes whole in file order and in six random orders. I didn't build Debug this round, so the assert path is covered by the guard and your analysis, not a Debug run of mine.

[ezequielzacca]

Merged main again (d3676c51b) for #858, #859 and #860:

• feat(monsters): the monster list, its names and kinds, and the monster editor tools (MN1) #860 replaced the monster name loader, so e9491df's OpenMonsterScript fix is gone with it; I took main's side. The tooltip test's data scope now loads the names through g_MonsterDataHandler (Monsters.json), as the game does.
• feat(monsters): the monster list, its names and kinds, and the monster editor tools (MN1) #860's TransformationRing::MonsterName replaces this PR's IsTransformationRingMonster, which I dropped.
• The tooltip snapshot still matches without re-recording, so the ring names are the same. All 1433 tests pass (Windows x64 Release, editor and control socket on), and test_item_database_files passes whole in file order and random orders.