fix(settings-override): rework override .json order #1729
Conversation
Changes flow of overwrite system to Default → User → Overrides → User Overrides (.user files) It is now very unlikely that the overwrite system will fail. It allows for user customization that is preserved and takes priority, while retaining the original overwrite files so they can be reverted if needed.
📝 WalkthroughWalkthroughIntroduces a User Override System and changes override application flow: adds per-feature Changes
Sequence Diagram(s)sequenceDiagram
participant State
participant OverrideManager as SettingsOverrideManager
participant Feature
participant FS as FileSystem
rect rgba(100, 200, 150, 0.5)
Note over State: State::Load() — new flow with user overrides
State->>State: Load Default Settings
State->>State: Load User Settings
State->>OverrideManager: DiscoverOverrides()
OverrideManager->>FS: Scan Overrides + User Overrides dirs
FS-->>OverrideManager: Override file list
OverrideManager-->>State: Overrides metadata
State->>OverrideManager: CleanupStaleUserOverrides()
OverrideManager->>OverrideManager: Remove stale .user.json
loop per loaded feature
State->>OverrideManager: ApplyOverrides(featureName, baseSettings)
OverrideManager->>OverrideManager: Merge global & feature override JSON
OverrideManager-->>State: appliedOverrides JSON
State->>OverrideManager: LoadUserOverride(featureName)
OverrideManager->>FS: Read feature.user.json (if exists)
FS-->>OverrideManager: User override JSON
OverrideManager-->>State: Merged settings (base + overrides + user)
State->>Feature: Reload feature with merged settings
Feature-->>State: Feature ready
end
end
sequenceDiagram
participant State
participant Feature
participant OverrideManager as SettingsOverrideManager
participant FS as FileSystem
rect rgba(150, 150, 200, 0.5)
Note over State: State::Save() — persisting user override delta
State->>Feature: Get current feature settings
Feature-->>State: currentSettings JSON
alt feature has overrides
State->>OverrideManager: GetMergedOverrideSettings(featureName, baseSettings)
OverrideManager->>OverrideManager: Compute combined hash & merged overrides
OverrideManager-->>State: mergedOverrides JSON
State->>OverrideManager: SaveUserOverride(featureName, currentSettings, mergedOverrides)
OverrideManager->>OverrideManager: Compute user-delta (current vs merged)
OverrideManager->>FS: Write/Update feature.user.json
FS-->>OverrideManager: Write OK
OverrideManager-->>State: Tracking updated
end
State->>State: Continue saving other features
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/SettingsOverrideManager.cpp`:
- Around line 1181-1198: The code calls tracking[trackingKey].get<std::string>()
without validating the JSON type, which can throw if the stored value is
corrupted; before calling get<std::string>(), check that
tracking[trackingKey].is_string() (or otherwise validate the type) and only
assign to storedHash when true; if the value is missing or not a string, log a
warning (using logger::warn/info) about the corrupted tracking value for
featureName, replace tracking[trackingKey] with currentHash and continue, and
then proceed with the existing comparison and update logic so
GetCombinedOverrideHash, trackingKey, storedHash and the std::filesystem::remove
path are only used when the stored hash is a valid string.
- Around line 1046-1062: The code updates the applied-overrides tracking even if
writing the user override file may have failed; after writing currentSettings to
the ofstream (variable file) and before calling
LoadAppliedOverridesTracking/SaveAppliedOverridesTracking/GetCombinedOverrideHash,
ensure the write succeeded by flushing and checking the stream state (e.g.,
file.flush() and if (file.fail() || !file.good()) ), log an error including
userFilePath and return false instead of updating tracking; only proceed to call
LoadAppliedOverridesTracking, set tracking[featureName + "_hash"] via
GetCombinedOverrideHash(featureName), and call
SaveAppliedOverridesTracking(tracking) when the file write was confirmed
successful.
🧹 Nitpick comments (1)
src/SettingsOverrideManager.cpp (1)
1016-1032: Consider extracting tolerance constants for numeric comparison.The float comparison logic with relative/absolute tolerance is well-implemented. For better maintainability, consider extracting the magic numbers to named constants.
♻️ Optional improvement
+namespace { + constexpr double FLOAT_ABS_TOLERANCE = 1e-6; + constexpr double FLOAT_REL_TOLERANCE = 1e-5; +} + // For numeric values, compare with tolerance to handle float32/float64 precision differences // JSON stores floats as float64, but C++ features often use float32, causing precision loss if (currentValue.is_number() && overrideValue.is_number()) { double current = currentValue.get<double>(); double override = overrideValue.get<double>(); double diff = std::abs(current - override); // Use relative tolerance for larger values, absolute for small values - double tolerance = std::max(1e-6, std::abs(override) * 1e-5); + double tolerance = std::max(FLOAT_ABS_TOLERANCE, std::abs(override) * FLOAT_REL_TOLERANCE); if (diff > tolerance) {
|
✅ A pre-release build is available for this PR: |
davo0411
left a comment
davo0411
left a comment
There was a problem hiding this comment.
my approach was to load the overrides into the user settings once on initial startup
but it led to issues of them not getting applied i guess
so like default -> override -> saved into user, then gets modified
i still think that approach is cleaner
my only design concern with this pr change is we now have 4 levels of settings
default -> user -> override -> user overriding the override
its clunky
all that being said, just give me some reasoning toward the new tiered system and I am happy to move to this. Just generally, the less files the better.
| appliedOverrides = json::object(); | ||
| } else { | ||
| // Validate each tracking entry | ||
| // Validate each tracking entry - allow both old format (objects) and new format (strings) |
There was a problem hiding this comment.
if you are changing the format, don't have backwards compatibility. AI likes to do this, to "Let the old system work". we should not be supporting an arbitrary legacy format under any circumstances. old overrides will need to be changed to fit one system.
Will likely need (Human) assessment more than just AI. Use AI to understand logic flow of code segments if you need.
There was a problem hiding this comment.
Was initiated by me. Only refers to the tracking file which is generated by the user. Wouldn't want settings to break for the end user when they update CS.
|
|
||
| // Invalid entry | ||
| logger::info("Invalid tracking entry for '{}', removing", key); | ||
| it = appliedOverrides.erase(it); |
There was a problem hiding this comment.
this we should not support, just force authors to use new system.
until we have lots of presets on nexus, we should not be supporting legacy systems.
There was a problem hiding this comment.
Just applies to tracking file. The actual overwrite files authors use stay unchanged.
| const std::string jsonExt = ".json"; | ||
| if (nameWithoutExt.length() >= jsonExt.length() && | ||
| nameWithoutExt.substr(nameWithoutExt.length() - jsonExt.length()) == jsonExt) { | ||
| if (nameWithoutExt.ends_with(jsonExt)) { |
|
|
||
| if (!userJson.is_object()) { | ||
| logger::info("User override file is not a JSON object: {}", userFilePath.string()); | ||
| return false; |
There was a problem hiding this comment.
we need handling for junk .jsons -- if a json is correctly named but has bad values, fake sliders/settings, malformed layout, etc.
it checks the object (that the file is a correctly named .json) but doesn't check the contents from what i can see
There was a problem hiding this comment.
I don't think this is needed from the user generated overwrite files?
Tested and it looks like normal overwrite files with junk .jsons will not apply.
| logger::info("Error deleting user override for {}: {}", featureName, e.what()); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
check this only occurs once and not every time the override is overwritten by the user. we do not want to be writing/deleting from the disk every time a user changes a value. this would be significant concern for performance. some form of caching/doing it only after all settings are iterated, etc, would be needed.
Claude seems to think so.
Per Claude:
Current Behavior:
Every time a user adjusts a slider, SaveUserOverride likely gets called, which:
Iterates through all override settings to check for differences
Performs numeric comparisons with tolerance calculations for every number value
Either writes a complete JSON file (with directory creation, file I/O, flush, hash computation, tracking update) OR deletes the file if values match defaults
Does full filesystem operations each time
There was a problem hiding this comment.
It only runs when the user intentionally saves their settings.
| */ | ||
| void ReportOverrideFailure(const std::string& modName, const std::string& featureName, const std::string& errorMessage); | ||
|
|
||
| // === NEW: User Override System === |
There was a problem hiding this comment.
also the whole thing should just be consolidated, not arbitrarily split (in terms of these new functions being below the old ones) should be integrated in an order that makes sense, ideally by functionality.
There was a problem hiding this comment.
should consolation come when this system is overhauled with the GUI stuff? seems complex
davo0411
changed the title
davo0411
changed the title
davo0411
changed the title
davo0411
changed the title
davo0411
changed the title
davo0411
changed the title
2 participants
Changes flow of overwrite system to Default → User → Overrides → User Overrides (.user files)
It is now very unlikely that the overwrite system will fail. It allows for user customization that is preserved and takes priority, while retaining the original overwrite files so they can be reverted if needed.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.