feat: Overwrite .json for individual feature setting changes from external mods#1388
Conversation
WalkthroughAdds a singleton SettingsOverrideManager and a JSON-based per-mod/per-feature override system, integrates it into the load sequence (Default → Overrides → User), wires per-feature reapplication and UI controls, and adds documentation and UI surfaces for override failures and management. Changes
Sequence Diagram(s)sequenceDiagram
participant Game
participant State
participant OverrideMgr as SettingsOverrideManager
participant FS as Filesystem
Game->>State: Initialize()
State->>FS: Load default config
State->>OverrideMgr: GetSingleton() / DiscoverOverrides()
OverrideMgr->>FS: Scan Overrides dir (*.json)
OverrideMgr-->>State: Discovered overrides
State->>OverrideMgr: LoadAppliedOverridesTracking()
State->>OverrideMgr: ApplyNewOverrides(defaultSettings, appliedTracking)
OverrideMgr-->>State: Merged global overrides
State->>FS: Optionally load user config
loop For each Feature
State->>OverrideMgr: ApplyNewFeatureOverrides(featureName, featureJson, appliedTracking)
OverrideMgr-->>State: Feature JSON merged (if new/changed)
State->>State: Load feature with merged settings
end
State->>OverrideMgr: SaveAppliedOverridesTracking(appliedTracking)
State-->>Game: Initialization complete
sequenceDiagram
participant UI as In-game UI
participant OverrideMgr as SettingsOverrideManager
participant Feature
UI->>OverrideMgr: Toggle override enabled/disabled
OverrideMgr-->>UI: Updated state (requires restart to apply)
UI->>Feature: User presses "Apply Override"
Feature->>OverrideMgr: ReapplyFeatureOverrides(featureName, featureJson)
OverrideMgr-->>Feature: Applied overrides (count)
Feature->>Feature: Reload settings if applied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
davo0411
changed the title
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: 1
🧹 Nitpick comments (3)
src/SettingsOverrideManager.h (1)
179-179: Consider documenting index stability requirementThe
featureOverrideMapstores indices into theoverridesvector. This works correctly as long as the vector isn't modified after building the map (which the current implementation ensures). Consider adding a comment to document this invariant.- std::unordered_map<std::string, std::vector<size_t>> featureOverrideMap; // Maps feature name to override indices + std::unordered_map<std::string, std::vector<size_t>> featureOverrideMap; // Maps feature name to override indices (indices must remain stable after discovery)src/SettingsOverrideManager.cpp (2)
324-325: Consider memory usage for large override filesReading the entire file content into memory could be problematic for very large JSON files. However, for typical configuration override files, this should be acceptable.
Consider adding a file size check before reading if this becomes an issue:
// Check file size before reading auto fileSize = std::filesystem::file_size(filePath); if (fileSize > 10 * 1024 * 1024) { // 10MB limit logger::warn("Override file too large: {} ({} bytes)", filePath.string(), fileSize); return nullptr; }
390-396: Document underscore handling in filename parsingThe parser uses the last underscore as the delimiter between modName and featureName. This works correctly but mod authors should be aware that their mod names can contain underscores, and only the last one will be used as the separator.
Example:
My_Cool_Mod_Skylighting.json→ modName: "My_Cool_Mod", featureName: "Skylighting"Consider adding a comment to clarify this behavior:
// Parse ModName_FeatureName format + // Uses the last underscore as delimiter, so mod names can contain underscores size_t lastUnderscore = nameWithoutExt.find_last_of('_');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package/SKSE/Plugins/CommunityShaders/Overrides/README.md(1 hunks)src/Feature.cpp(1 hunks)src/SettingsOverrideManager.cpp(1 hunks)src/SettingsOverrideManager.h(1 hunks)src/State.cpp(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (5)
package/SKSE/Plugins/CommunityShaders/Overrides/README.md (1)
1-164: Well-documented feature with clear examples!The README provides comprehensive documentation for the override system with clear naming conventions, JSON format examples, and troubleshooting guidance. This will help mod authors use the feature correctly.
src/Feature.cpp (1)
33-33: LGTM!The include directive is properly placed among other header includes.
src/State.cpp (2)
111-114: Loss of detailed performance instrumentationThe detailed performance event formatting has been commented out and replaced with generic strings. This reduces the debugging information available in performance traces.
Was this change intentional? The original formatting provided more context about shader types and defines which could be valuable for performance analysis.
203-252: Well-structured three-stage configuration loading!The implementation properly handles the loading sequence (Default → Overrides → User) with appropriate error handling and logging at each stage. The override system integration is clean and maintains backward compatibility.
src/SettingsOverrideManager.cpp (1)
424-427: Good defensive programming with metadata field handlingThe check for underscore-prefixed keys to skip metadata fields during merge is a clean approach. This prevents metadata from accidentally overwriting actual settings.
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/Feature.h (1)
101-106: Consider marking the new API [[nodiscard]] and documenting side-effectsThe bool return indicates success; make it harder to ignore by annotating with [[nodiscard]]. Also clarify in the brief that the method mutates in-memory settings and does not persist to disk.
- virtual bool ReapplyOverrideSettings(); + [[nodiscard]] virtual bool ReapplyOverrideSettings();src/FeatureIssues.cpp (1)
365-381: Nice addition: dedicated Override Failures section
- Properly segregates override issues and uses error palette.
- The collection and rendering mirror other sections; consistent.
Minor suggestion: consider ordering this section above “Unknown Features” if override failures are more actionable for users.
Also applies to: 418-425
src/SettingsOverrideManager.h (3)
48-61: Doc/return-type mismatch for ApplyNewOverridesThe comment implies the function returns modified settings JSON; signature returns size_t. Update the brief to reflect the actual return of “number of overrides applied.”
- * @brief Applies overrides to settings JSON, respecting applied override tracking + * @brief Applies overrides to settings JSON, respecting applied override tracking * @param baseSettings The default settings to start with * @param appliedOverrides Reference to tracking data for applied overrides - * @return Modified settings with overrides applied (only new/changed overrides) + * @return Number of overrides applied (only new/changed overrides)
212-216: Rename parameter ‘override’ to avoid confusion with the C++ contextual keywordNot a functional bug, but using ‘override’ as a parameter name can be confusing when scanning code. Rename to overrideJson for clarity.
- void MergeJson(json& target, const json& override); + void MergeJson(json& target, const json& overrideJson);
90-105: Annotate frequently-used return values with [[nodiscard]]These APIs signal important outcomes; encourage callers to check results.
- const std::vector<OverrideInfo>& GetOverrides() const { return overrides; } + [[nodiscard]] const std::vector<OverrideInfo>& GetOverrides() const { return overrides; } - std::vector<const OverrideInfo*> GetFeatureOverrides(const std::string& featureName) const; + [[nodiscard]] std::vector<const OverrideInfo*> GetFeatureOverrides(const std::string& featureName) const; - bool HasFeatureOverrides(const std::string& featureName) const; + [[nodiscard]] bool HasFeatureOverrides(const std::string& featureName) const; - size_t ReapplyFeatureOverrides(const std::string& featureName, json& featureJson); + [[nodiscard]] size_t ReapplyFeatureOverrides(const std::string& featureName, json& featureJson); - std::filesystem::path GetOverridesDirectory() const; + [[nodiscard]] std::filesystem::path GetOverridesDirectory() const; - json LoadAppliedOverridesTracking() const; + [[nodiscard]] json LoadAppliedOverridesTracking() const; - std::filesystem::path GetAppliedOverridesTrackingPath() const; + [[nodiscard]] std::filesystem::path GetAppliedOverridesTrackingPath() const; - bool IsEnabled() const { return enabled; } + [[nodiscard]] bool IsEnabled() const { return enabled; }Also applies to: 112-121, 149-158
src/Menu/FeatureListRenderer.cpp (1)
526-536: Apply Override UX: good gating; consider exposing result detailThe button is correctly gated behind hasOverrides and feature readiness. Since the call chain can compute how many entries were applied, consider surfacing the count (in logs or a small toast) for clarity.
If you’d like, I can refactor Feature::ReapplyOverrideSettings() to return a count and wire that here without breaking callers.
Also applies to: 587-605
src/State.cpp (1)
213-227: Enable overrides only when discovered — consider honoring IsEnabled()You already respect discovery; additionally guard with IsEnabled() so a disabled override system does no work.
- auto overrideManager = SettingsOverrideManager::GetSingleton(); - size_t overridesDiscovered = overrideManager->DiscoverOverrides(); + auto overrideManager = SettingsOverrideManager::GetSingleton(); + size_t overridesDiscovered = overrideManager->IsEnabled() ? overrideManager->DiscoverOverrides() : 0;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
package/SKSE/Plugins/CommunityShaders/Overrides/README.md(1 hunks)src/Feature.cpp(2 hunks)src/Feature.h(1 hunks)src/FeatureIssues.cpp(3 hunks)src/FeatureIssues.h(3 hunks)src/Menu/FeatureListRenderer.cpp(3 hunks)src/SettingsOverrideManager.cpp(1 hunks)src/SettingsOverrideManager.h(1 hunks)src/State.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package/SKSE/Plugins/CommunityShaders/Overrides/README.md
- src/SettingsOverrideManager.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (7)
src/FeatureIssues.h (1)
54-60: New issue type looks good; ensure downstream UI/classifiers handle it consistentlyAdding OVERRIDE_FAILED and IsOverrideFailed() is clear and self-contained. Verify any switch/if chains elsewhere (e.g., sorting, counters, telemetry) include this type where appropriate.
Would you like me to scan the codebase for switches on IssueType and propose patches if any skip OVERRIDE_FAILED?
Also applies to: 64-68
src/Feature.cpp (1)
269-291: Harden ReapplyOverrideSettings() with exception safety and loggingWrap the
LoadSettings(featureJson)call in a try/catch so malformed overrides won’t leave the feature in a bad state, and log any errors for troubleshooting.• File:
src/Feature.cpp, inFeature::ReapplyOverrideSettings()around theLoadSettings(featureJson)call.
• Ensure that your codebase provides alogger::warn(or equivalent) and thatLoadSettingsthrowsstd::exception–derived errors.Suggested diff:
if (appliedCount > 0) { - LoadSettings(featureJson); - return true; + try { + LoadSettings(featureJson); + return true; + } catch (const std::exception& e) { + logger::warn("Invalid override settings for {}: {}. Keeping original settings.", GetName(), e.what()); + } catch (...) { + logger::warn("Invalid override settings for {}. Keeping original settings.", GetName()); + } + return false; }Please verify that the logging call and exception types match your project’s conventions.
src/Menu/FeatureListRenderer.cpp (1)
530-536: Right-alignment math accounts for the new button — goodThe dynamic width calc and conditional inclusion of overrideButtonWidth is correct and keeps alignment stable.
src/State.cpp (4)
228-249: User merge is a shallow top-level overwrite — verify this matches intended semanticsThe loop replaces whole sections (e.g., “General”, “Menu”) rather than deep-merging nested keys. That’s fine if intended, but it can drop defaults/overrides subkeys absent in user.json.
If deep-merge is desired, I can provide a small JSON merge helper to recursively overlay user settings onto defaults+overrides.
352-365: Per-feature override application: solid; good tracking save
- Saving current feature JSON, applying new overrides, and reloading is correct.
- You already catch exceptions on LoadSettings — nice.
- Persisting AppliedOverrides after the loop ensures idempotence on subsequent runs.
Also applies to: 377-381
384-385: Config upgrade path preserves mode — goodSaving with the original a_configMode upon version bump keeps behavior consistent.
168-176: Manual verification needed: confirm and guard directory creation in both Load and SaveI couldn’t locate
src/State.cppor anycreate_directoriescalls in the repo, so please:
- Ensure the file and methods under review actually exist in the expected path.
- In the Load path, guard against an empty
parent_path():- std::filesystem::create_directories(configFolderPath);
- if (!configFolderPath.empty()) {
std::filesystem::create_directories(configFolderPath);- }
- In the Save(ConfigMode) implementation, derive and guard the save directory the same way: ```cpp auto configPath = GetConfigPath(a_configMode); auto dir = std::filesystem::path(configPath).parent_path().string(); if (!dir.empty()) { std::filesystem::create_directories(dir); }Please verify file locations and update both code paths accordingly.
| logger::info("Loading default settings from: {}", defaultConfigFilePath); | ||
| if (!tryLoadConfig(defaultConfigFilePath)) { | ||
| logger::info("No default config ({}), generating new one", defaultConfigFilePath); | ||
| std::fill(enabledClasses, enabledClasses + magic_enum::enum_integer(RE::BSShader::Type::Total) - 1, true); | ||
| Save(ConfigMode::DEFAULT); | ||
| // Attempt to load the newly created config | ||
| if (!tryLoadConfig(defaultConfigFilePath)) { | ||
| logger::error("Error opening newly created default config file ({})\n", defaultConfigFilePath); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential off-by-one when initializing enabledClasses
Filling up to Total - 1 risks leaving the last slot uninitialized. Prefer std::begin/std::end to fill the entire array.
- std::fill(enabledClasses, enabledClasses + magic_enum::enum_integer(RE::BSShader::Type::Total) - 1, true);
+ std::fill(std::begin(enabledClasses), std::end(enabledClasses), true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger::info("Loading default settings from: {}", defaultConfigFilePath); | |
| if (!tryLoadConfig(defaultConfigFilePath)) { | |
| logger::info("No default config ({}), generating new one", defaultConfigFilePath); | |
| std::fill(enabledClasses, enabledClasses + magic_enum::enum_integer(RE::BSShader::Type::Total) - 1, true); | |
| Save(ConfigMode::DEFAULT); | |
| // Attempt to load the newly created config | |
| if (!tryLoadConfig(defaultConfigFilePath)) { | |
| logger::error("Error opening newly created default config file ({})\n", defaultConfigFilePath); | |
| return; | |
| } | |
| } | |
| logger::info("Loading default settings from: {}", defaultConfigFilePath); | |
| if (!tryLoadConfig(defaultConfigFilePath)) { | |
| logger::info("No default config ({}), generating new one", defaultConfigFilePath); | |
| - std::fill(enabledClasses, enabledClasses + magic_enum::enum_integer(RE::BSShader::Type::Total) - 1, true); | |
| + std::fill(std::begin(enabledClasses), std::end(enabledClasses), true); | |
| Save(ConfigMode::DEFAULT); | |
| // Attempt to load the newly created config | |
| if (!tryLoadConfig(defaultConfigFilePath)) { | |
| logger::error("Error opening newly created default config file ({})\n", defaultConfigFilePath); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/State.cpp around lines 201 to 211, the std::fill call uses
magic_enum::enum_integer(RE::BSShader::Type::Total) - 1 which likely causes an
off-by-one and leaves the last enabledClasses element uninitialized; replace
that fill with a full-range fill using the array bounds (e.g.,
std::fill(std::begin(enabledClasses), std::end(enabledClasses), true)) or
compute the exact size with magic_enum::enum_integer(RE::BSShader::Type::Total)
without subtracting one so every element is initialized.
2 participants
adds a system for automatic overwriting of specific feature settings by loading additional .json files, named according to {modname}_{featureshortname}.json. No in game UI changes.
Loads the settings once when the overwrite.json is first installed, overwriting the user settings for the given feature. After this, the settings are not loaded again. The user is able to make changes and save them as per normal.
The settings are essentially loaded like this:
defaults.json -> overrides.json -> user.json
Summary by CodeRabbit
New Features
Improvements
Documentation