feat(save-editor): group and explain the attribute list - #77
Conversation
The attribute list carried MagicianLevel as "Magic Circle" — the exact name the Skills list already uses for the same thing, in the same tab. Only one of the two did anything: the game re-derives the attribute from the skill's GameplayEffect class when the save is loaded, so a value typed into the attribute field never survives. Proven in game: a save whose effect class said circle 6 while MagicianLevel still said -1 let the hero use a circle 4 rune, and rune usability is stated against MagicianLevel. MagicianLevel therefore joins the four Critical_* attributes in heroHiddenAttributeIds, for the same reason they are there. It stays editable in the All-data browser. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0db4ff1421
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When the typed attribute search fails or comes back empty, the hero pane falls back to the legacy private-player editor, which listed every row the core's summary carries — without consulting heroHiddenAttributeIds. The core includes MagicianLevel in that summary, so on this path the two identically named Magic Circle controls were still both there, the ineffective one included. The four Critical_* ids never surfaced the problem because the core's private-player summary does not carry them at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8df9e48. Configure here.
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The advanced attributes were one long list of raw engine ids with machine-generated labels — "Fill ratio (AttributeSet_Thirst)", "Critical level (%)" — that told a player nothing, and several of them do nothing at all. Grouped into Combat & movement, Diving, Sleep & rest and Intoxication, with the leftovers still under Advanced. A group's sidebar entry only appears when the hero actually carries its attributes. Every remaining value now has a real name and a tooltip on that name saying what it does in the game, in all twelve languages. Both live in one ICU select message per locale, so the whole table is two keys. Ids that exist in several attribute sets are addressed by a composite key (`Health_RecoveryRatePerHourOfSleep`), which is what lets the Health and Mana sleep rates carry different wording — and what retires the "(AttributeSet_X)" suffix the rows used to grow. The separator is an underscore because ICU rejects a dot in a select arm. Dropped from the curated view, because the game does not act on them: Toughness and its three curve coefficients (encumbrance was cut, and carrying is unlimited), and the sixteen hunger/thirst/fatigue values of the survival mode that cannot be switched on. All stay editable in the All-data browser. See docs/reference/survival-mode.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Gothic 1 Remake contains a complete hunger/thirst/fatigue system that no player can reach: the abilities are granted, the attribute sets are seeded, the per-stage effects are configured — and the difficulty UI row that would switch it on ships hidden behind m_IsShown = false, with the remaining gate in native code no script ever calls. Writing it down because the save editor now hides sixteen attributes on the strength of it. If a patch switches the mode on, that decision has to be revisited, and nobody should have to redo the investigation to know what those values meant. Covers what it is, why it does not run, what was measured in game, the three places the flag is stored (and the permadeath field sitting next to it), the full mechanics including a mis-wired thirst threshold, the inventory of hidden values, and the steps to re-enable the group. Registered in the reference index and in the MCP page table, so the server can serve it as gore://reference/survival-mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dh0er
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b9ee3af. Configure here.
| // Hide the per-weapon critical values from the curated view (same as | ||
| // the player); they stay editable in the All-data browser. | ||
| .where((row) => !heroHiddenAttributeIds.contains(row.key)) | ||
| .where((row) => !heroAttributeHidden(row.key)) |
There was a problem hiding this comment.
NPC hide ignores set-qualified ids
Medium Severity
heroAttributeHidden and heroAttributeGroup only treat RecoveryRatePerHourOfSleep as Fatigue-inert or Health/Mana sleep stats when given setClass. The NPC filter calls heroAttributeHidden(row.key) with no set, and grouping/ranking do the same, even though _setClassFromPaths already recovers it for labels. Fatigue recovery therefore stays in the curated NPC list, and the Health/Mana rates stay in Advanced instead of Sleep. The legacy player fallback has the same hide gap.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit b9ee3af. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9ee3af21b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Hide the per-weapon critical values from the curated view (same as | ||
| // the player); they stay editable in the All-data browser. | ||
| .where((row) => !heroHiddenAttributeIds.contains(row.key)) | ||
| .where((row) => !heroAttributeHidden(row.key)) |
There was a problem hiding this comment.
When an NPC response contains RecoveryRatePerHourOfSleep from AttributeSet_Fatigue, this call omits the set class, so heroAttributeHidden cannot match the new set-qualified Fatigue_RecoveryRatePerHourOfSleep entry and the inert survival value remains visible. The same missing context in npc_attributes_panel.dart:186-193 also puts the live Health/Mana versions in Advanced rather than Sleep; extract the set class from basePath/currentPath and pass it consistently to the hidden, grouping, and ranking helpers.
Useful? React with 👍 / 👎.
…combat group "XP for executing" and "XP for killing" were nearly the same sentence twice, and neither said what the difference is. Gothic has a non-lethal defeat state: an opponent can be beaten down and left unconscious rather than killed, and finishing off someone who already lies defeated is a separate act the game even tracks as its own crime severity. So the pair is now "XP for defeating" (bringing the character down at all, dead or merely unconscious) against "XP for finishing off" (killing it while it lies defeated), with tooltips that name both outcomes. The combat group was called "Combat skills", which reads as the game's one-handed/two-handed/bow skills. It holds poise, damage taken and movement speed instead, so it is "Combat and movement" now, and the message key follows the meaning. Both across all twelve languages, each translated with its language's established term for a downed opponent and for the finishing blow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 564b68dc76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| showRoster: false, | ||
| ), | ||
| attributeLabel: attributeLabel, | ||
| attributeTooltip: attributeTooltipFor, |
There was a problem hiding this comment.
When an NPC carries attributes such as DamageMultiplier, SuperArmor, SpeedModifier, or Oxygen, this passes the player-oriented tooltip resolver into its panel unchanged. The localized messages explicitly describe what “the hero” takes, absorbs, or does, so hovering an NPC row misleadingly suggests that editing it affects the player rather than the selected NPC; provide NPC-specific wording or make these shared descriptions actor-neutral.
Useful? React with 👍 / 👎.
"Combat and movement" was too wide for the sidebar. It reads "Combat / movement" now, in every language, which is the same information in fewer pixels. The sidebar tile truncated with an ellipsis at one line, so a heading that still does not fit lost its second word entirely. It wraps onto a second line instead; the ellipsis only applies beyond that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
The attribute list was a dump of raw engine ids. Some had machine-generated labels that told a player nothing — "Fill ratio (AttributeSet_Thirst)", "Critical level (%)" — several duplicated a control that already existed elsewhere, and a number of them do nothing at all in the game.
Two controls that silently did nothing
MagicianLevelsat in the attribute list under the same name as the Magic Circle row under Skills, in the same tab. Only one of the two worked: the game re-derives the attribute from the skill's GameplayEffect class on load, so a value typed into the field never survived.Proven in game — a save whose effect class said circle 6 while
MagicianLevelstill said-1let the hero use a circle 4 rune, and rune usability is stated againstMagicianLevel(every rune writesm_RequiredStats[MagicianLevel] = RequiredMagicCircleLevel, the same machinery a sword uses for Strength).That makes it the same case as the four
Critical_*attributes, hidden earlier for exactly this reason. It joins them. The same exclusion now also applies on the fallback path, where the legacy private-player editor listed every row the core's summary carries without consulting the hidden set.The advanced list, reworked
Grouped into Combat & movement, Diving, Sleep & rest and Intoxication, with the leftovers under Advanced. A group's sidebar entry only appears when the hero actually carries its attributes.
Every remaining value has a real name and a tooltip on that name saying what it does — in all twelve languages. Both live in one ICU
selectmessage per locale, so the whole table is two keys rather than fifty.Ids that exist in several attribute sets are addressed by a composite key (
Health_RecoveryRatePerHourOfSleep). That is what lets the Health and Mana sleep rates carry different wording, and what retires the(AttributeSet_X)suffix the rows used to grow.Values dropped, and why
Toughness,ToughnessA/B/CBoth stay editable in the All-data browser — the editor hides what cannot work, it does not refuse access to the bytes.
The survival finding is written up in
docs/reference/survival-mode.md: the system is fully built and the six abilities are granted, but the difficulty UI row that would enable it ships hidden behindm_IsShown = falseand the remaining gate is native code no script ever calls. Forcing the runtime state true before the hero loaded, with hunger at 900/1000 — the stage that owes −15 % Strength and 1 HP per second — changed nothing. The page records the mechanics, the three places the flag is stored (and the permadeath field sitting next to it), the inventory of hidden values and the steps to re-enable the group, so the decision can be revisited without redoing the investigation.Testing
flutter test— 541 passed.flutter analyze lib test— no issues.cargo test -p gore-mcp— 310 passed.🤖 Generated with Claude Code
Note
Medium Risk
Changes which save fields appear in the main editor and how edits are filtered; wrong hiding or grouping could confuse users or hide real stats, but values remain reachable via All data and tests cover grouping/hiding.
Overview
The curated hero/NPC attribute UI is reorganized into Combat / movement, Diving, Sleep & rest, and Intoxication (plus existing core/resistances/skills/advanced), with grouping and sort order driven by set-aware keys via
heroAttributeKeyandheroAttributeHidden.Hidden from the curated list (still in All data): duplicate or load-overwritten values such as
MagicianLeveland skill-derived crits; unused Toughness and survival Hunger/Thirst/Fatigue fields, with set-qualified hiding for ambiguous ids likeRecoveryRatePerHourOfSleepon Fatigue vs Health/Mana.Labels and tooltips use localized ICU
attributeManualFallbackLabel/attributeManualTooltipkeys (composite ids where needed); player, NPC, and legacy attribute rows show tooltips on hover. Combat sidebar copy becomes “Combat / movement”; duplicate(AttributeSet_*)suffixes on labels are removed. CHANGELOG documents the UX change and related fixes (duplicate Magic Circle, faster saves from another change in the same release notes).Reviewed by Cursor Bugbot for commit d02c899. Bugbot is set up for automated code reviews on this repo. Configure here.