Repository navigation
feat(items): Item Sync with OpenMU, the exchange file with Import and Export (phase 7, step 1) - #129
Conversation
3e5e78e to
2cad7aa
Compare
Mosch0512
left a comment
Mosch0512
left a comment
There was a problem hiding this comment.
Verdict: changes requested. Two items are required; the other 13 are non-blocking.
Required
- An added item loses the stat profile the file gives it when its rules give another one (inline, ItemSyncApply.cpp:114).
- A formula profile or table that only the client has starts checked, so the default Apply removes it (inline, ItemSyncDiff.cpp:453).
Non-blocking
- A refusal caused by the formulas unchecks no row, so the next Apply fails the same way (inline, MuItemSyncUI.cpp:608).
ApplyItemSyncDiffthrows on a Changed row whose item the client no longer has (inline, ItemSyncDiff.cpp:724)."server": ""refuses the whole file (inline, ItemExchangeFile.cpp:81).- The file dialog's filter label stays in the first UI language (inline, MuItemSyncUI.cpp:381).
- The bmd weight conversion goes by group and slot, not by the item's profile (inline, ItemBmdImport.cpp:391).
GrowWeighthard-codes the requirement formula (inline, ItemBmdImport.cpp:386).- The shared fields and tags are listed twice (inline, ItemSyncApply.cpp:68).
- The export preview builds a whole item diff to count formula entries (inline, ItemSyncApply.cpp:323).
FindItemscans the items once per row (inline, ItemSyncApply.cpp:171).- The field toggle builds its list again every frame (inline, MuItemSyncUI.cpp:785).
takenLocalesis never read (inline, ItemSyncDiff.h:97).ApplyItemSyncImportis about 135 lines (inline, ItemSyncApply.cpp:137).KnownDifferencescould have a file of its own (inline, ItemSyncDiff.h:30).
I read the diff with the enclosing functions and followed the import through ItemEdits::SetItems and CItemDataHandler::WithDerivedFields, the save checks it runs first, the export, the bmd conversion and the window. I didn't build or run it. CI: Quality Gates and the Linux and macOS builds (editor OFF) passed; the Windows build (editor OFF) was still running when this was posted. The editor-ON jobs are skipped, so CI compiles neither the Item Sync window nor its tests.
| } | ||
| // "" stores the item by its rules (CItemDataHandler::SetItems). | ||
| const std::string ruled = Data::Items::DeriveStatProfile(profiles, item); | ||
| item.statProfile = RuleName(exchange.statProfile) == ruled ? std::string() : exchange.statProfile; |
There was a problem hiding this comment.
Required. The pick made here doesn't survive the store for an added item. ItemEdits::SetItems stores through CItemDataHandler::WithDerivedFields, and for an item without a previous version (previous == nullptr) a non-empty statProfile goes to FollowStatProfile(profiles, nullptr, …), which returns DeriveStatProfile: the rules' profile.
For example, the file adds a wing with "statProfile": "secondTierWing". The exchange file has no wing tier, so the rules give the default ({"profile": "default", "slot": ["wings"]}), and the row's note says "Its profile rules give default, the file secondTierWing". Apply then stores the default profile, so the wing gets the default defense and damage formulas, and the comparison afterwards lists statProfile again. The checks before the apply validated secondTierWing, not what is stored.
An existing item has the same gap when the file's profile equals its old derived one while the imported fields change what the rules give: definition.statProfile == previous->statProfile makes it follow the new data.
The new-item test only covers a profile the rules give (newSword->statProfile == "weapon"). Storing the import's pick as it is (for example by telling WithDerivedFields that the profile is picked), with a test of an added item whose file profile differs from its rules, would cover it.
| } | ||
| } | ||
| row.known = options.known != nullptr && options.known->Contains(row.entry); | ||
| row.checked = !row.known; |
There was a problem hiding this comment.
Required. Every formula row starts checked, also a profile or table that only the client has, so the default Apply removes it. The comment of BuildItemSyncDiff and docs/item-data.md say that removals start unchecked.
For example, a server file without this PR's orb profile (or without one of the client's tables) lists profiles.orb checked. Apply removes the profile; the 12 orbs still name orb, so with the default checks the save checks refuse the whole import. Starting a row unchecked when the file lacks the entry (incoming is empty), as for removed items, would match the docs.
| { | ||
| if (row.group == issue.group && row.number == issue.number) | ||
| { | ||
| SetRowChecked(row, false); |
There was a problem hiding this comment.
Non-blocking. After a refused Apply only item rows are unchecked, by the issue's group and number. Errors of the formulas carry NoItem, so the formula row behind them stays checked and the next Apply fails the same way. For example, a checked profiles.armor.defense.table that names a table the import doesn't take (the case of the test "Formulas the item stat file's checks refuse are not taken"). Unchecking the formula rows on such a refusal, or saying which formula rows caused it, would help.
| targetItems[key] = UnflattenItem(row.group, row.number, fields, issues); | ||
| continue; | ||
| } | ||
| Fields fields = FlattenItem(targetItems.at(key)); |
There was a problem hiding this comment.
Non-blocking. targetItems.at(key) throws std::out_of_range for a Changed row whose item the client no longer has. In the window the "data changed" blocker prevents that, but ApplyItemSyncDiff and ApplyItemSyncImport are public, and an exception there ends the editor. A find that skips the row, as the New and Removed branches skip theirs, keeps them safe on their own.
| } | ||
| return false; | ||
| } | ||
| if (!field->is_string() || field->get_ref<const std::string&>().empty()) |
There was a problem hiding this comment.
Non-blocking. An empty text is an error also for the optional server, so a file with "server": "" can be neither imported nor compared, although the writer itself leaves an empty server out. Reading an empty optional text as absent would let such a file through.
| } | ||
| continue; | ||
| } | ||
| const ItemDefinition* exchange = FindItem(applied, row.group, row.number); |
There was a problem hiding this comment.
Non-blocking. FindItem scans applied.items once for each row. ItemSyncDiff.cpp already has IndexItems; indexing once would do, and the lookup wouldn't be written twice.
|
|
||
| void CMuItemSyncUI::RenderFieldToggle() | ||
| { | ||
| std::set<std::string> names; |
There was a problem hiding this comment.
Non-blocking. This builds a std::set of every changed field's name on each frame while the window is open. With a large diff that is thousands of strings per frame for a list that only changes with the diff; Compare could build it once.
| std::vector<SyncItem> items; | ||
| std::vector<SyncFormula> formulas; | ||
| // The languages names are taken in (the client's UI languages). | ||
| std::vector<std::string> takenLocales; |
There was a problem hiding this comment.
Non-blocking. takenLocales is filled in BuildItemSyncDiff and never read, by neither the window, the apply nor the tests. It could go, or the window could show it where it says which languages are taken.
| return notes; | ||
| } | ||
|
|
||
| bool ApplyItemSyncImport(const ItemSyncDiff& diff, const ItemExchangeFile& file, std::vector<ItemDataIssue>& issues) |
There was a problem hiding this comment.
Non-blocking. ApplyItemSyncImport is about 135 lines: it applies the diff, picks the stat file, collects the items and removals, checks the held items, runs the save checks and records the undo step. Rule 1 of docs/CODING_RULES.md: "A function does one thing. If you can describe it with "and", split it." and "Aim for functions that fit on one screen (~40 lines)." The held check, the checks of the data as it would be and the undo step could each be a function of their own.
| { | ||
| // The items' fields and formula entries meant to differ between the client and a server | ||
| // (Data/Items/Sync/KnownDifferences.json): the diff shows them unchecked. | ||
| struct KnownDifferences |
There was a problem hiding this comment.
Non-blocking. KnownDifferences has its own file, format version, loader and writer, but lives in the diff's header and source. Rule 6 of docs/CODING_RULES.md: "Each new class or significant struct/type gets its own file, named after the class." A KnownDifferences.h and .cpp would match.
a503155 to
fede166
Compare
Phase 7 moves the item data both sides have, and the item formulas, between the client and OpenMU: MuEditor exports and imports an exchange file and updates from the server, each with a diff the user checks by hand. It has five steps, one PR each: the exchange file with Export and Import (MuMain), the item formulas as data, a read-only export endpoint and an import page (OpenMU), and Update from server (MuMain). The design replaces section 6 and settles the decisions taken for it: the exchange file's fields in the item files' names and meanings, the formulas part, the orbs, crystals and scrolls keeping their requirements as values as OpenMU does, the known differences kept in the repo, the endpoint behind an API key with rate limits and a cached export, and the key from an environment variable. The OpenMU notes are brought up to date: the rule flags of PR A, the configuration download that exists, and the fields its item definitions have.
…s values The 44 items of groups 12 and 15 that have stat requirements and no slot stored a weight that the drop level formula grew into the requirement. OpenMU stores the value the game asks for instead, so they do too now, for the item exchange with OpenMU (phase 7): the scroll profile and a new orb profile, with a rule of its own for group 12, keep the stat requirements as stored. A test pins the 44 values at +0, where the game asks for exactly what it did before; it shows these items at no other level (above +0 the value now stays as it is, as on OpenMU). The Orb of Summoning keeps its energy by level. The bmd files of the original client hold weights for these items: Import from bmd turns them into the values with the original client's formula, and Export as bmd and Export as S6E3 write the smallest weight that gives each value, with a note where none gives it exactly. The tests of the "levelRequirement" basis, which no shipped profile uses any more, install the old scroll profile for their scope.
The file of phase 7: a header (format version, source, the server of a file from OpenMU, game version), the item formulas and the items, with the item fields OpenMU has too in the names and meanings of the item files. The formulas are the item stat file without its profile rules, which stay with the client; each item names the profile it uses. The file is written in the layout of the item files and the item stat file, so the same data always gives the same text. The reader refuses another game version or source and a newer format, reads no item whose (group, number) is there twice (OpenMU does not keep it unique), and leaves out what stays with the client with a warning. The item files and the item stat file give their item and formulas JSON and their text layout to it; their own files stay as they were.
The diff the Item Sync window shows: the client's items and item formulas against an exchange file, field by field, named by their keys in the item files and the item stat file, and the side that changes (the client for an import, the file for an export) with the checked rows taken from the other. New and changed rows are checked by default, removals are not, nor are the known differences (Data/Items/Sync/KnownDifferences.json), a drop level OpenMU cannot keep, or the size and slot of an item the game holds, which stay as they are also when checked. A language one side lacks stays as it is, and an import takes names only in the client's UI languages. A profile one side lacks is one row as a whole; formulas the item stat file's checks refuse are not taken.
…hange file The window lists the differences between the client's items and item formulas and an exchange file, per item and per field. Only the checked rows are applied: an import changes the client's data as one step of the undo history, after the checks of a save pass on the data as it would be; an export writes the client's checked values into the file. Known differences stay unchecked and are kept in Data/Items/Sync/KnownDifferences.json.
…peated one A removed row of an import lists what Remove item lists: what goes with the item and what uses it at its number. An item a file has more than once (OpenMU does not keep the group and number unique) no longer refuses the whole file: it is a locked row the import leaves as it is, while the rest of the file can be imported. An export still refuses such a file, as writing it would lose both copies.
The window, the exchange file, the diff and the known differences in docs/item-data.md; the window in the editor README. The design doc now says that an export does not write into a file that has an item twice.
Right after an export the file has the client's data, so the window showed only "No differences" with Check all, Check none and the write button, which do nothing then. It now says that the file has the same items and formulas as the client and how to see differences, and hides the buttons.
…y it cannot
The window lists only what differs, which left it empty and unclear
right after an export. Now:
- A file that matches the client says so in green, and the lists say
how much of what was compared differs ("Items that differ (3 of
911)", "Items: all 911 are the same in the client and the file").
- What the last Apply wrote or applied shows above what it left; the
rows that were not checked stay unchecked.
- The apply and write button is off, with the reason, while nothing is
checked, while the path field holds another file than the compared
one (it wrote into the typed path before), and while the client's
items or formulas changed since the comparison.
- An export into a file that is there but cannot be read is refused; a
new file says it is not there yet.
- A new item of a type the character holds is a locked row, and an
import is refused when an item it would add, resize, give another
slot or remove is held by then (ItemEdits refused it silently, and
the import reported success).
- A new row whose client data lost a field since the comparison no
longer throws.
…row 0 A table header takes the ID of its label under PushID(column), so the column named "##Checked" had the ID of row 0's "##Checked" checkbox under PushID(0), and ImGui reported the conflict. The column has a name of its own now, in all three tables of the window.
…lies or replaces An export compared the client with the file it was written into, which listed every item as new for a new file and was not what an export is expected to do; OpenMU's import page and the client's import each show the diff of the side that changes. Now: - Export writes the client's items and formulas: a new file at once, a file that is there after a confirmation with what changes in it (items added, changed and removed, formula entries; or that it cannot be read as an exchange file), and nothing when the file already has the client's data. - Apply the checked changes asks first, with what it adds, changes and removes in the client; what is not checked or not listed stays. - The import table says what happens in the client: Added, Changed, Removed, under "In the client". The diff is only of a file into the client now: SyncDirection, the row of all formulas for a file without them and the export of checked rows are gone.
…gs keep their action - The import and export confirmations open again while they wait for an answer: a click after F12 hid the editor closed them, and nothing was applied or written without a word. - Each file dialog keeps its own action (its userdata): with an import and an export dialog open at once, the file picked to import became the export target. A dialog's answer waits while a confirmation is open, so the confirmation keeps its diff. - After an export the window compares again when the import's file is the same file (also written differently, or not readable before). - The "In the client" column takes the width of its widest text. - The help text says that only Import lists differences; the design doc no longer says that an export refuses a file with an item twice, and the docs say "added" where the window does.
…nge file OpenMU's items have no description, so the exchange file leaves it out like the other client-only fields: a file with one is read with a warning, and an import keeps the client's.
Compare two files (folded under the path field) lists what changes from file A to file B, e.g. a client export and a server export, without the client's data: in every language, also one only one file has, with the formulas. Nothing can be applied; a row can be marked as a known difference. An import and a comparison of two files replace each other in the window. Only the client has it: OpenMU's import page compares a file with its live data.
…emoved items right An item file A has twice was listed as added, without a name, as the file leaves it out of its items; its row now says changed or removed by what file B has, takes the name the files give, and names the file that has it twice instead of the import's text about the client. An item only file A has lists file A's values.
…hecked Every formula row started checked, also a profile or table the file lacks, so the default Apply removed it; with the orb profile gone the orbs named a profile that was not there, and the checks refused the whole import. Such a row starts unchecked now, as a removed item does.
… for an item
Storing an item lets its profile follow the rules when the item is new
or its profile field does not change, so an added wing the file gives
secondTierWing got the default profile its rules give without a wing
tier. The import decides each item's profile itself ("" follows the
rules, another one is a pick), so it stores them as given
(StatProfileStore::AsGiven); the item tools keep following the data.
…fuse an import An issue of the formulas has no item, so a refusal they caused unchecked no row and the next Apply failed the same way. Such a refusal unchecks all formula rows now, and the message says so.
…nt no longer has targetItems.at threw for a Changed row whose item the client lost since the diff; the window's data-changed blocker prevented it, but ApplyItemSyncDiff and ApplyItemSyncImport are public. The row is left out, as the New and Removed branches leave theirs.
An empty text was an error also for the optional server, so a file with "server": "" could be neither imported nor compared, though the writer leaves an empty server out.
…'s language The static filter kept the label of the first dialog, so after the editor's language changed the dialog still showed the old one; the label is set again before each dialog.
…code has names The add, the percents and the item groups of GrowWeight and HasRequirementWeightsInBmd were bare numbers. They stay constants of the code rather than the item stat file's formulas: they describe what the original client's bmd files hold, which does not change with the data.
HasRequirementWeightsInBmd decided by group 12 or 15 and no slot whether an item keeps its requirements as values; in the game its profile decides. An orb given the default profile, or the orb profile changed back to a weight formula, held weights again, and Export as bmd and S6E3 turned them into other weights. A requirement converts now when the item's profile (the one it names, or the one its rules give) keeps its base value while the default grows it; with the shipped formulas these are the same 44 orbs, crystals and scrolls. Import from bmd converts in the handler, once each item's profile is known.
ToClientItem listed by hand the fields ToExchangeItem copies, and both files had their own list of the shared tags; a field added to one and missed in the other would show in the diff but never be applied. CopyExchangeFields and ExchangeTags in ItemExchangeFile.h serve both directions.
…them PreviewItemSyncExport built a whole diff, flattening and comparing all 911 items of both sides, only to count the formula rows right after it had compared the items itself; CountFormulaDifferences compares the formulas alone.
ApplyItemSyncImport scanned the applied items once per row, and the diff and the export preview each built their own index; IndexExchangeItems in ItemExchangeFile.h serves all three.
…s" with the diff It built a set of every changed field's name on each frame while the window was open; the list only changes with the diff, so Compare builds it once.
… takes takenLocales was filled and never read; the options say which languages are taken.
…each It was about 135 lines: it applied the diff, picked the stat file, collected the items and removals, checked the held items, ran the save checks and recorded the undo step. FindChangedStatFile, CollectImportChange, CheckHeldItems, CheckImportedData and RecordImport do those now; the behaviour stays.
It has its own file, format version, loader and writer, but lived in the diff's header and source; KnownDifferences.h and .cpp hold it now, as rule 6 of docs/CODING_RULES.md asks.
fede166 to
99c4203
Compare
Summary
Step 1 of phase 7 of the data-driven items plan (
docs/superpowers/specs/2026-09-25-data-driven-items-design.md, section 6): the item sync with OpenMU, client side. The items and item formulas move between the client and an OpenMU server through one JSON file, the item exchange file. MuEditor gets an Item Sync window: Import shows how a file differs from the client and takes the checked rows, Export writes the client's items and formulas into a file. The server stays authoritative; the client never writes to it.The later steps are separate PRs: 2 the item formulas as data in OpenMU, 3 OpenMU's read-only export endpoint, 4 Update from server in MuEditor, 5 OpenMU's import page.
The exchange file
formatVersion1,source(MuMainorOpenMU),server(only from OpenMU),gameVersion(season6only; another one, or an unknownformatVersion, is refused).formulas: the item stat file withoutformatVersionand the profile rules, which stay with the client.items: the fields both sides have, in the keys and meanings of the item files, sorted by group and number. What only the client has (model and display data, the description, the other tags, wing tier, weapon kind, two-handed, potion priority, attack type, magic defense, the rental flags) is not in it; a file with such a field is read with a warning.Orbs, crystals and scrolls keep their requirements as values
The 44 items of groups 12 and 15 with stat requirements and no slot keep the values the game asks for, as OpenMU stores them, instead of a weight the drop level formula grows. Their profiles write
"base"for the stat requirements (thescrollprofile, and a neworbprofile with a rule of its own for group 12). The values were converted once; a test pins them and checks that the game asks for the same at +0 (the game shows these items only at +0). Import from bmd turns the original bmd weights into values with the original client's formulas; Export as bmd and Export as S6E3 turn the values back into the smallest weight that gives them (the nearest one, with a note, when none gives a value exactly).The Item Sync window (Windows → Items → Item Sync)
Data/Items/Sync/KnownDifferences.json, ticked in the diff), and fields with a note: the size and slot of an item the character wears or an item window shows (such an item is not removed, nor a new one of its type added), and a drop level above 255 (a byte on OpenMU).Under the window
ItemExchangeFile(reader, writer,ToExchangeItem,BuildItemExchange), withWriteItemJson/ReadItemJsonand the formula parts of the stat file JSON exposed from the item file and stat file code.ItemSyncDiff: the diff by flattened field paths (width,requirements.strength,name.de,tags.ammunition,default.damage.table,profiles.scroll.requirements.energy,profiles.<name>for a profile as a whole), with counts of what was compared, andApplyItemSyncDiff.ItemSyncApply:ToClientItem,ApplyItemSyncImport(item stat file first, then the items, one undo group),PreviewItemSyncExport,WriteItemSyncExport.WriteItemTextFilewrites one file the way Save Items does (temporary file, then rename).Tests and checks
test_item_database_files186,test_item_database159,test_effect_types74, all passing (see the note on one test of main below); the build without the editor and itstest_item_databasepass too. New: the exchange file round trip of all 911 items and the formulas, the reader's refusals, the diff (changes, names, held items, known differences, drop level, formulas, repeated items, counts), the window's import as one undo step, a refused import, the confirmations and blockers, the plain export with its counts, the pinned orb and scroll values and the bmd weight round trip.main(Translate item names, Server List editor, item descriptions, the tooltip texts as data). The S6E3 and bmd exports build their records from main'sBuildItemAttributes(the item names of a language) and turn the requirements into weights on top of it (BuildBmdItemAttributes); the CSV export keeps the values.test_item_database_filesrun (it passes alone). After any test that loaded the item data in aClientDataScope, the scope setsItemAttributeback to null while the item data handler still listens to locale changes; the test'sI18N::SetLocale("de")then refills a null table. The editor-ON test jobs are skipped in PR CI, so CI does not show it. A fix belongs in its own PR.