[ezequielzacca]

This step's own changes: compare with #820. 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 of #815 (data-driven item tooltips; design). Stacked on #820 (→ #819 → … → #813): only the last commit, refactor(items): build the item tooltip's lines apart from drawing them, is this step.

Before the stat lines move into data (5b-3), there must be a way to check that a tooltip didn't change, for every item rather than a few. This PR adds it.

The split

RenderItemInfo() (ZzzInventory.cpp) is two functions now:

• BuildItemTooltipText() writes the lines into TextList / TextListColor / TextBold, as before.
• RenderItemInfo() draws them. The Dark Lord's pets still draw their own tooltip.

The code only moves, and in the same order:

• the early returns of the item, the pet check, and the set and socket lines stay in the builder;
• the height and position of the box go to the drawing.

Nothing changes in game.

The snapshot

BuildItemTooltipLines() returns the built lines without drawing them. Two control socket commands use it; they exist only in builds with the control socket (docs/control-socket.md):

• tooltip: the lines of one item made from the fields the server sends (group, number, level, luck, skill, option, excellent, ancient, harmony, …), for the hero as he is.
• tooltip-snapshot: the lines of every item at every level 0–15, as plain, full (luck, skill, +4 option, every excellent option), ancient and harmony items, written to a text file. One request per item group (group) keeps the frame short.

A part of the file:

0/0 full +7
  0 <blank>
  4 bold Excellent Kris +7
  0 <blank>
  1 One-Handed Damage: 57 ~ 62
  0 Attack speed: 50
  0 Durability: [20/45]
  0 Strength Requirement: 102
  0 Agility Requirement: 82
  0 <blank>
  0 <blank>
  1 Luck (success rate of Jewel of Soul +25%)
  1 Luck (critical damage rate +5%)
  1 Additional Dmg +16
  1 Excellent Damage rate +10%
  1 Increase Damage +level/20
  1 Increase Damage +2%
  1 Increase Attacking(Wizardry)speed +7
  1 Increases acquisition rate of Life after hunting monsters +life/8
  1 Increases acquisition rate of Mana after hunting monsters +Mana/8
  0 <blank>

On the local OpenMU:

• the 16 groups give 60,736 tooltips in about one second;
• two runs give the same file (byte for byte);
• they agree with the same tooltips asked one by one through tooltip.

So the steps after this one record a snapshot before and after their change, with a hero that meets the requirements and one that doesn't, and compare them with any diff tool.

Next (5b-1, continued)

The builder still reads the hero and the UI directly (CharacterAttribute, g_pNewUISystem, g_pMyInventory, the pet, socket and harmony managers). Moving those reads behind a TooltipContext, one subsystem at a time and each checked with the snapshot, lets the same loop run in ctest with a made-up hero. Then the snapshot check runs in CI too.

Screenshots

None: nothing changes in game. I checked the tooltips by hand in both themes after the split (the Wing of Storm +15 and the Jewel of Bless, as in #818 and #816).

Related issues

Step 5b-1 of #815. Builds on #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 989 tests pass.
  • The editor build with the control socket (windows-x64-mueditor) builds, and both commands were run against a local OpenMU as above.
  • Changed lines pass clang-format 21.1.8 as CI checks them.
• Changes are focused on one concern; the diff is as small as it reasonably can be.

[sven-n]

Bug: the static debounced pet-info callback captures the first tooltip item forever. In src/source/Engine/Object/ZzzInventory.cpp, RenderItemInfo, the new static DebouncedAction debouncedPetInfoRequest([ip, Inventype] { ... }) initializes its lambda only on the first Dark Lord pet tooltip. Subsequent tooltips reuse the first ip and Inventype, potentially after the item was destroyed, and can request the wrong pet information or dereference a dangling pointer. Please make the callback use current item/context at invocation time rather than capturing a transient ITEM* in static storage; add a test that alternates raven/horse items and inventory types.

Input validation: src/source/App/Control/ControlCommandsQuery.cpp, ItemTooltip: the level, durability, option, excellent, ancient, harmony, and harmony_level integers are cast directly to BYTE without bounds checks. Values outside 0..255 wrap and may create unintended item states. Reject invalid ranges with bad_request and cover boundaries. Also check output stream state after ItemTooltipSnapshot writes.

Reviewed against AGENTS.md and docs/CODING_RULES.md; build not run.

[ezequielzacca]

Thanks! Both addressed:

Pet info callback. Right, and it predates this PR (main has the same static DebouncedAction capturing [ip, Inventype]); this step only moved it, so I fixed it here. The callback no longer captures anything: each call stores the item type and storage of the tooltip being drawn, and the callback reads those when it fires. Which request it makes is now GetPetInfoRequest(itemType, pointedIndex, inventype), tested with the raven in the left hand, then the horse as the helper, then the raven again from other storages, and with pets pointed at anywhere else (no request).

Control socket input. tooltip checks every value before it makes the item and refuses what the item's fields can't hold with bad_request: level, option, harmony and harmony_level 0–15 (the item packet's four bits), excellent 0–63, durability and ancient 0–255. tooltip-snapshot checks the stream after writing, so a short file is failed. New test_control_tooltip_commands covers each boundary and the write failure.

Built and tested locally (all tests pass), and CI's quality gate (clang-format 21.1.8 on the changed lines, cppcheck 2.13 with CI's flags) passes locally. Carried through #822–#836.

[Mosch0512]

Verdict: changes requested for one item; the rest is non-blocking. The split itself is right:

• Nothing changes in game. The builder runs the old code up to the same point, and the code just before the split is identical. The old block that added the set and socket lines and then measured the box is now two blocks under the same !bItemTextListBoxUse condition. The drawing half only needs p, which it looks up again.
• Every early exit is covered: Nothing for the two returns, DarkLordPet for the pets, which still draw their own tooltip, in the same order as before.
• The pet request fix is right. DebouncedAction runs the callback inside invoke(), right after the two statics are set, so it always reads the item being drawn. GetPetInfoRequest keeps the old slot rules.
• The snapshot is safe to run in game:
  • The builder only reads game state: no server requests, effects or statics.
  • ShowLegacyTextList copies the lines when it is called, so the commands rewriting the shared text list between frames can't touch a shown tooltip.
  • The temporary items are freed again (ref count 1 → 0).

Required

• tooltip-snapshot empties out before it checks group (inline, ControlCommandsQuery.cpp:638).

Non-blocking

• tooltip answers with a tooltip of nothing for an item number no item uses (inline, ControlCommandsQuery.cpp:555).
• The snapshot's out isn't resolved like screenshot's (inline, ControlCommandsQuery.cpp:684).
• harmony_level without harmony is silently ignored (inline, ControlCommandsQuery.cpp:610).
• The docs don't say that per-group requests overwrite out (inline, control-socket.md:85).
• The pet request passes its item through two statics (inline, ZzzInventory.cpp:4407).

Before merging (not code)

• The branch needs main. In a random-order run of test_item_database_files, "Item Requirements does not store a +0 value that needs a weight picked when its window is hidden" fails. That's the order-dependent editor bug fixed by #854, which is on main but not on this branch yet.
• CI doesn't compile the new commands. The jobs that ran for this PR use presets with ENABLE_CONTROL_SOCKET off, and the editor-ON jobs were skipped. So ControlCommandsQuery.cpp's new code and test_control_tooltip_commands are only checked by local editor builds so far.

What I checked:

• the builder and the drawing half against the old function;
• the pet request against the old capture and against DebouncedAction;
• the builder for side effects, since the snapshot runs it on 60,000 made-up items;
• the item manager's create and delete, and how the tooltip copies its lines;
• the input checks and their test.

Built on Windows x64 Debug with the editor and the control socket: test_item_database, test_item_database_files, test_control_tooltip_commands and test_control_protocol pass in file order. I didn't run the two commands against a server.

[Mosch0512]

Required. The ofstream is constructed, and so the file truncated, before group is checked. A request with a bad group therefore empties out and only then answers bad_request.

For example, record {"cmd":"tooltip-snapshot","out":"before.txt","group":0}, change the code, then mistype {"out":"before.txt","group":16} while meaning after.txt. The reply is bad_request, but before.txt is now empty, and the "before" snapshot the whole step relies on is gone. Checking group (and anything else that can fail) before the ofstream is constructed fixes it.

[ezequielzacca]

Fixed in 78e5861: group is checked before out is opened, so a refused request leaves the file as it was. A test writes a file, sends group: 16 and checks the file still holds what it did.

[Mosch0512]

Non-blocking. This only range-checks group and number. For a number no item uses, CreateItemByParameters still makes an item, and the reply is a tooltip of an empty item rather than an error. That looks like a valid answer to a script with a typo'd number. The "the item could not be made" branch can't trigger either, because CreateItemByParameters returns null only for a null pointer. Refusing with bad_request when g_ItemDatabase.Find(type) is null, as the snapshot skips items that don't Exists(), would make that branch real.

[ezequielzacca]

Done in 78e5861: tooltip answers bad_request ("no item has group G and number N") when g_ItemDatabase.Find(group, number) is null. Since CreateItemByParameters can't fail for an item that exists, I dropped the unreachable "could not be made" branch and TooltipOf returns the lines directly. Tested.

[Mosch0512]

Non-blocking. screenshot resolves its out with ResolveScreenshotPath (UTF-8 to a wide, absolute path) and reports the path it wrote. This command opens out relative to the client's working directory and replies with the path as given. So {"out":"before.txt"} lands in whatever folder the client runs in, and the reply only says "before.txt". Using ResolveScreenshotPath for both opening and the reply would keep the two file-writing commands alike.

[ezequielzacca]

Done in 78e5861: the snapshot opens ResolveScreenshotPath(out) and replies with that absolute path, as screenshot does. A test sends a relative out and checks the reply names the absolute path.

[Mosch0512]

Non-blocking. harmony_level only counts inside if (harmony > 0). So {"group":0,"number":0,"harmony_level":13} replies with a plain tooltip and no error, and harmony without harmony_level silently uses level 0. A bad_request for harmony_level without harmony would catch the forgotten field.

[ezequielzacca]

Done in 78e5861: harmony_level without harmony is a bad_request ("harmony_level needs harmony"), with a test. harmony alone still uses level 0, as documented.

[Mosch0512]

Non-blocking. The row recommends one group per request, but each request truncates out. A full snapshot by groups therefore needs 16 files; with one out it ends up holding the last group only. A note ("use one file per group"), or an append option, avoids that. It could also say that the Dark Horse and the Dark Raven get a header but no lines, since the pet manager draws their tooltip.

[ezequielzacca]

Done in 78e5861: the row now says each request writes out anew, so a snapshot by groups takes one file per group, and that the Dark Horse and the Dark Raven get their header line but no lines.

[Mosch0512]

Non-blocking. This is correct, since invoke() runs the callback synchronously right after these assignments, but it takes the comment to see why. A throttle that keeps only the last request time would need no shared state: it calls GetPetInfoRequest(ip->Type, g_pMyInventory->GetPointedItemIndex(), Inventype) directly when a second has passed.

[ezequielzacca]

Done in 78e5861: it keeps only the last request time and calls GetPetInfoRequest(ip->Type, GetPointedItemIndex(), Inventype) directly when a second has passed. No statics for the item any more, and Debouncer.h had no other user, so it's gone.

[ezequielzacca]

Thanks for the review. Pushed 78e5861 on top of a merge of main:

• Required: tooltip-snapshot checks group before opening out.
• All five non-blocking points done: unknown item and harmony_level without harmony are refused, out is resolved like screenshot's, the docs note one file per group and the pets, and the pet request keeps only its last request time.
• main is merged in, so fix(editor): a +0 value that needs the weight list is no longer stored with a guessed weight #854's fix is on the branch: test_item_database_files passes in a random-order run.
• CI doesn't build the control socket: right. I built the editor preset (control socket on) locally on Windows x64 Release: all 1283 tests pass, including test_control_tooltip_commands (6 cases, 4 new) and test_control_protocol. Turning on an editor-ON job for PRs is a CI change I'd leave to a separate PR, if you want it.

I didn't run the commands against a server for this round; the request validation and path handling are covered by the new tests; the pet throttle has no test and wasn't tried in game.