feat(UI): Time of Day#1903
Closed
Dlizzio wants to merge 41 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughAdds Time of Day (TOD) support and blending: per-period SettingEntry, TOD lifecycle (activate/deactivate/update/blend), per-period persistence and overwrite discovery, new shared SceneSettings UI + TimeOfDayPanel, EM-based theme sizing, and calendar-preferred game-time retrieval. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as TimeOfDayPanel
participant UIHelpers as SceneSettingsUI
participant Manager as SceneSettingsManager
participant Persist as FileStorage
participant Feature as FeatureSystem
UI->>UIHelpers: User adds/edits setting (feature,key,value,period)
UIHelpers->>Manager: AddSetting(type, feature, key, value, period)
Manager->>Manager: Validate type, period, float invariants
Manager->>Persist: SaveUserSettings(type) [includes period metadata]
Persist-->>Manager: Persisted ack
UI->>Manager: Request current TOD state / dominant period
Manager->>Manager: UpdateTimeOfDay() -> GetTimeOfDayFactors()
Manager->>Manager: ApplyTimeOfDayBlended()
Manager->>Feature: Apply blended values to features
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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🧪 Generate unit tests (beta)
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 |
|
No actionable suggestions for changed features. |
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.
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/SceneSettingsManager.cpp (2)
239-312:⚠️ Potential issue | 🟡 MinorReject TimeOfDay entries without a valid period
AddSettingacceptsTimeOfDayPeriod::Countfor TOD, which can create invisible entries that still trigger TOD activation. Add a guard to prevent invalid period input. As per coding guidelines: "Validate user input and prevent DirectX crashes from malformed configurations".Suggested fix
void SceneSettingsManager::AddSetting(SceneType type, const std::string& featureShortName, const std::string& settingKey, const json& value, TimeOfDayPeriod period) { + if (type == SceneType::TimeOfDay && period == TimeOfDayPeriod::Count) { + logger::warn("[SceneSettings] Ignoring TOD entry without period: {}.{}", featureShortName, settingKey); + return; + } if (HasDuplicateEntry(type, featureShortName, settingKey, EntrySource::User, period)) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 239 - 312, AddSetting currently allows TimeOfDay entries with TimeOfDayPeriod::Count (an invalid sentinel) which creates invisible/invalid entries; change AddSetting to validate input early: if type == SceneType::TimeOfDay and period == TimeOfDayPeriod::Count then reject (return) and optionally log or report an error; ensure this check happens before calling HasDuplicateEntry/GetEntriesMut so no invalid entry is created or saved.
346-363:⚠️ Potential issue | 🟠 MajorDelete-all skips TimeOfDay overwrite files in period subfolders
DeleteAllOverwritesuses the base directory even for TOD, so overwrite files under period subfolders remain on disk and will be reloaded later. This breaks expected behavior.Suggested fix
for (const auto& entry : vec) { - if (entry.source == EntrySource::Overwrite && !entry.sourceFilename.empty()) - std::filesystem::remove(overwritesPath / entry.sourceFilename, ec); + if (entry.source == EntrySource::Overwrite && !entry.sourceFilename.empty()) { + auto filePath = overwritesPath / entry.sourceFilename; + if (type == SceneType::TimeOfDay && entry.period != TimeOfDayPeriod::Count) + filePath = overwritesPath / GetPeriodName(entry.period) / entry.sourceFilename; + std::filesystem::remove(filePath, ec); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 346 - 363, DeleteAllOverwrites currently only deletes files directly under GetOverwritesPath(type), so TimeOfDay overwrite files stored in period subfolders are left on disk; update DeleteAllOverwrites (which uses GetEntriesMut, EntrySource::Overwrite and entry.sourceFilename) to remove matching files recursively for the TOD scene type: obtain overwritesPath, and if type corresponds to TimeOfDay, iterate std::filesystem::recursive_directory_iterator(overwritesPath) and delete any file whose filename (or relative path if stored) matches entry.sourceFilename (fall back to overwritesPath / entry.sourceFilename for non-TOD types or if entry.sourceFilename already contains a subpath); leave the erase_if on the entries vector, preserve setting of allOverwritesPausedMap[type] and the ReapplyIfActive() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 793-799: The code uses lastAppliedTODFloats[shortName][key] which
default-inserts 0.0f, so the first blended value near zero gets skipped by the
kBlendEpsilon check; change the logic in the apply loop to detect whether an
entry already exists before doing the epsilon compare (e.g., use find/contains
on lastAppliedTODFloats for shortName and then for key, or initialize values to
a NaN sentinel and test std::isnan) and only perform the std::abs(cachedFloat -
result) < kBlendEpsilon comparison when a prior value is present; if there is no
prior value, set the cached value to result and push the key/result into
dirtyKeys unconditionally (or set sentinel/updated flag) so first-time ~0
results are applied.
- Around line 905-917: For TimeOfDay entries, ensure entries missing a period or
with an invalid period are skipped and a warning is logged: when type ==
SceneType::TimeOfDay, check item.contains("period") and call
GetPeriodFromName(item["period"].get<std::string>()); if the period is missing
or entry.period == TimeOfDayPeriod::Count, log a warning including
entry.featureShortName, entry.settingKey and the raw period value (so users can
fix malformed configs) and continue without pushing the entry; keep using
HasDuplicateEntry and EntrySource::User behavior unchanged for valid entries.
- Around line 779-811: Guard against NaN/Inf for all floats used in the TOD
blending path by adding std::isfinite checks (include <cmath>) after each
get<float>() call: in the block using baseline->get<float>() (baseVal) and for
pr.value->get<float>() and any factor values (f) read from
factors[pr.periodIdx]; if a value is not finite, replace it with a safe fallback
(e.g., 0.0f) before using it in the accumulation for result/coveredFactor so
shader-bound settings can't become NaN/Inf; keep existing comparisons with
lastAppliedTODFloats, kBlendEpsilon and dirtyKeys unchanged.
In `@src/WeatherEditor/TimeOfDayPanel.cpp`:
- Around line 172-201: The InputFloat handler for SettingType::Float must
validate the edited float before calling
manager->UpdateEntryValue/SaveUserSettings; currently a text edit can produce
NaN/Inf that gets persisted. After the ImGui::InputFloat returns true (or before
UpdateEntryValue is called), check the local float (val) with std::isfinite (or
!std::isnan && std::isfinite) and only call
manager->UpdateEntryValue(kSceneType, index, val, true) when the value is
finite; if it is not finite, reject the change (e.g., revert to a safe default
or skip the update) and do not call SaveUserSettings. Ensure the validation is
in the block handling SettingType::Float so malformed floats never reach
UpdateEntryValue/SaveUserSettings.
---
Outside diff comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 239-312: AddSetting currently allows TimeOfDay entries with
TimeOfDayPeriod::Count (an invalid sentinel) which creates invisible/invalid
entries; change AddSetting to validate input early: if type ==
SceneType::TimeOfDay and period == TimeOfDayPeriod::Count then reject (return)
and optionally log or report an error; ensure this check happens before calling
HasDuplicateEntry/GetEntriesMut so no invalid entry is created or saved.
- Around line 346-363: DeleteAllOverwrites currently only deletes files directly
under GetOverwritesPath(type), so TimeOfDay overwrite files stored in period
subfolders are left on disk; update DeleteAllOverwrites (which uses
GetEntriesMut, EntrySource::Overwrite and entry.sourceFilename) to remove
matching files recursively for the TOD scene type: obtain overwritesPath, and if
type corresponds to TimeOfDay, iterate
std::filesystem::recursive_directory_iterator(overwritesPath) and delete any
file whose filename (or relative path if stored) matches entry.sourceFilename
(fall back to overwritesPath / entry.sourceFilename for non-TOD types or if
entry.sourceFilename already contains a subpath); leave the erase_if on the
entries vector, preserve setting of allOverwritesPausedMap[type] and the
ReapplyIfActive() call.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/SceneSettingsManager.cppsrc/SceneSettingsManager.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/TimeOfDayPanel.cppsrc/WeatherEditor/TimeOfDayPanel.h
|
✅ A pre-release build is available for this PR: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SceneSettingsManager.cpp (1)
347-364:⚠️ Potential issue | 🟠 Major
DeleteAllOverwritesdoesn't account for TOD period subfolders — files won't be deleted from disk.
RemoveSetting(line 298) andDiscoverOverwritesInDir(line 959) both route TOD overwrite files through period subfolders (e.g.,TimeOfDay/Dawn/file.json), butDeleteAllOverwritesat line 355 always usesoverwritesPath / entry.sourceFilename, which resolves to the wrong path for TOD entries. The in-memory entries are erased correctly, but the files persist on disk and will be rediscovered on the next load.🐛 Proposed fix
auto& vec = GetEntriesMut(type); for (const auto& entry : vec) { - if (entry.source == EntrySource::Overwrite && !entry.sourceFilename.empty()) - std::filesystem::remove(overwritesPath / entry.sourceFilename, ec); + if (entry.source == EntrySource::Overwrite && !entry.sourceFilename.empty()) { + auto filepath = overwritesPath; + if (type == SceneType::TimeOfDay && entry.period != TimeOfDayPeriod::Count) + filepath /= GetPeriodName(entry.period); + filepath /= entry.sourceFilename; + std::filesystem::remove(filepath, ec); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 347 - 364, DeleteAllOverwrites removes in-memory overwrite entries but constructs the wrong filesystem path for TimeOfDay (TOD) entries because it always uses GetOverwritesPath(type) / entry.sourceFilename; update it to resolve the exact same on-disk path used by RemoveSetting and DiscoverOverwritesInDir for TOD period subfolders. Specifically, in SceneSettingsManager::DeleteAllOverwrites, when iterating GetEntriesMut(type) and deleting files for entries with source == EntrySource::Overwrite, compute the file path the same way RemoveSetting/DiscoverOverwritesInDir do for TOD (i.e., include the period subfolder for SceneType::TimeOfDay) or call the same helper used by those functions to build the path, then pass that resolved path to std::filesystem::remove; keep the rest of the cleanup (std::erase_if, allOverwritesPausedMap[type] = false, ReapplyIfActive()) unchanged.
🧹 Nitpick comments (1)
src/SceneSettingsManager.cpp (1)
296-298: Consider breaking up the long ternary for readability.Line 298 is hard to scan in one pass. A small helper or early
ifwould clarify intent.♻️ Suggested refactor
// For TimeOfDay overwrites, files are in period subfolders auto basePath = GetOverwritesPath(type); - auto filepath = (type == SceneType::TimeOfDay && entry.period != TimeOfDayPeriod::Count) ? basePath / GetPeriodName(entry.period) / entry.sourceFilename : basePath / entry.sourceFilename; + auto filepath = basePath; + if (type == SceneType::TimeOfDay && entry.period != TimeOfDayPeriod::Count) + filepath /= GetPeriodName(entry.period); + filepath /= entry.sourceFilename;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 296 - 298, The ternary that builds filepath is hard to read; refactor by computing basePath via GetOverwritesPath(type) then use a small conditional to set filepath: if (type == SceneType::TimeOfDay && entry.period != TimeOfDayPeriod::Count) build filepath by appending GetPeriodName(entry.period) and entry.sourceFilename to basePath, otherwise set filepath to basePath / entry.sourceFilename; you can also extract a helper like BuildOverwriteFilePath(type, entry) that encapsulates this logic to improve clarity and reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 404-408: UpdateEntryValue currently calls ApplyTimeOfDayBlended
but the hour-based throttle in ApplyTimeOfDayBlended (which compares current
hour to lastBlendedHour against kHourUpdateThreshold) prevents immediate
application when editing a TOD slider; before calling ApplyTimeOfDayBlended from
UpdateEntryValue (when type == SceneType::TimeOfDay and isTimeOfDayActive),
reset the sentinel lastBlendedHour to an out-of-range value (e.g., -1.0f) so the
throttle is bypassed and the new TOD value is applied immediately; locate the
lastBlendedHour variable and the call site in
UpdateEntryValue/ApplyTimeOfDayBlended to implement this change.
- Line 11: Remove the unused header include by deleting the line "#include
<numeric>" from SceneSettingsManager.cpp; ensure no other code in the file
references symbols from <numeric> so compilation is unaffected and run a build
to verify.
---
Outside diff comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 347-364: DeleteAllOverwrites removes in-memory overwrite entries
but constructs the wrong filesystem path for TimeOfDay (TOD) entries because it
always uses GetOverwritesPath(type) / entry.sourceFilename; update it to resolve
the exact same on-disk path used by RemoveSetting and DiscoverOverwritesInDir
for TOD period subfolders. Specifically, in
SceneSettingsManager::DeleteAllOverwrites, when iterating GetEntriesMut(type)
and deleting files for entries with source == EntrySource::Overwrite, compute
the file path the same way RemoveSetting/DiscoverOverwritesInDir do for TOD
(i.e., include the period subfolder for SceneType::TimeOfDay) or call the same
helper used by those functions to build the path, then pass that resolved path
to std::filesystem::remove; keep the rest of the cleanup (std::erase_if,
allOverwritesPausedMap[type] = false, ReapplyIfActive()) unchanged.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 296-298: The ternary that builds filepath is hard to read;
refactor by computing basePath via GetOverwritesPath(type) then use a small
conditional to set filepath: if (type == SceneType::TimeOfDay && entry.period !=
TimeOfDayPeriod::Count) build filepath by appending GetPeriodName(entry.period)
and entry.sourceFilename to basePath, otherwise set filepath to basePath /
entry.sourceFilename; you can also extract a helper like
BuildOverwriteFilePath(type, entry) that encapsulates this logic to improve
clarity and reuse.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/SceneSettingsManager.cppsrc/WeatherEditor/TimeOfDayPanel.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/WeatherEditor/TimeOfDayPanel.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SceneSettingsManager.cpp (1)
268-285:⚠️ Potential issue | 🟡 MinorGuard against TimeOfDay entries with invalid period.
If
period == Count, the entry becomes invisible and later discarded on save/load. Skip and log to avoid ghost entries.
As per coding guidelines: “Validate user input and prevent DirectX crashes from malformed configurations.”🛡️ Suggested fix
void SceneSettingsManager::AddSetting(SceneType type, const std::string& featureShortName, const std::string& settingKey, const json& value, TimeOfDayPeriod period) { + if (type == SceneType::TimeOfDay && period == TimeOfDayPeriod::Count) { + logger::warn("[SceneSettings] Skipping TOD entry with invalid period: {}.{}", featureShortName, settingKey); + return; + } if (HasDuplicateEntry(type, featureShortName, settingKey, EntrySource::User, period)) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 268 - 285, Add a guard in SceneSettingsManager::AddSetting to skip creating entries when period == TimeOfDayPeriod::Count: at the top of AddSetting (before calling HasDuplicateEntry/GetEntriesMut) check for that invalid enum value, log a warning using the project's logging facility, and return early so no SettingEntry is added, SaveUserSettings or ReapplyIfActive are invoked; keep references to HasDuplicateEntry, GetEntriesMut, SaveUserSettings, and ReapplyIfActive to help locate the correct spot to insert this validation.
🧹 Nitpick comments (2)
src/SceneSettingsManager.cpp (2)
39-44: Optional: lock period-name table tokPeriodCount.A tiny
static_assertguards against future mismatches if the enum changes.♻️ Suggested tweak
const char* SceneSettingsManager::GetPeriodName(TimeOfDayPeriod period) { static const char* names[] = { "Dawn", "Sunrise", "Day", "Sunset", "Dusk", "Night" }; + static_assert(kPeriodCount == (sizeof(names) / sizeof(names[0])), + "Update period name table when kPeriodCount changes"); int idx = static_cast<int>(period); return (idx >= 0 && idx < kPeriodCount) ? names[idx] : "Unknown"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 39 - 44, Add a compile-time check to guard the hard-coded names array length against changes to the TimeOfDayPeriod enum: inside SceneSettingsManager::GetPeriodName, add a static_assert that the number of entries in the local names array equals kPeriodCount (referencing names and kPeriodCount) so any future enum change will produce a build error if the table is not updated.
1-4: Conventional commit title + issue reference suggestion.Consider adjusting the title to lowercase description (e.g.,
feat(ui): time of day) and, if there’s a tracking issue, add an “Implements #”/“Addresses #” reference in the PR description.
As per coding guidelines: “Conventional Commit Titles … lowercase description … Issue References (Fixes/Implements/Addresses/Related to#123).”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 1 - 4, Update the PR/commit title to follow Conventional Commits (lowercase description, e.g., "feat(ui): time of day") and add an issue reference in the PR description like "Implements #<id>" or "Addresses #<id>"; locate the change related to SceneSettingsManager (file/class SceneSettingsManager or the commit that added/modified src/SceneSettingsManager.cpp) and amend the commit message or PR title accordingly and add the appropriate issue line in the PR description before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 946-960: DeleteAllOverwrites currently only removes files from the
base overwrites directory and therefore misses TimeOfDay period subfolders;
update DeleteAllOverwrites to mirror DiscoverOverwrites behavior: if scene type
== SceneType::TimeOfDay iterate for i in 0..kPeriodCount, build each periodPath
= GetOverwritesPath(type) / GetPeriodName(static_cast<TimeOfDayPeriod>(i)) and
delete entries in each periodPath (reuse the same deletion logic used for base
dir), otherwise delete from GetOverwritesPath(type) as before so TOD period
files are removed.
- Around line 734-847: The float extraction calls in ApplyTimeOfDayBlended
(baseline->get<float>() and pr.value->get<float()>) can throw on
malformed/non-numeric JSON; before calling get<float>() validate types (e.g.,
json::is_number() or reuse DetectSettingType) and if baseline is not numeric
skip treating the key as a float (log a warning) and if a period value is
non-numeric treat that period as absent (skip its contribution or use 0.0f per
existing behavior) instead of calling get<float>(); update the float path in
SceneSettingsManager::ApplyTimeOfDayBlended to perform these checks around
baseVal and periodVal extraction and ensure dirtyKeys/featureFloats logic
remains consistent when skipping invalid entries.
- Around line 917-930: The TimeOfDay parsing block in SceneSettingsManager calls
item["period"].get<std::string>() without validating the JSON type, which can
throw; update the TimeOfDay branch to first check item["period"].is_string() (or
equivalent) and if not, log a warn via logger::warn referencing
entry.featureShortName and entry.settingKey and skip the entry, otherwise call
GetPeriodFromName on the string and keep the existing invalid-period check
(TimeOfDayPeriod::Count) and continue behavior; ensure you reference the same
symbols (SceneType::TimeOfDay, item["period"], GetPeriodFromName, entry.period)
so the change is localized.
---
Outside diff comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 268-285: Add a guard in SceneSettingsManager::AddSetting to skip
creating entries when period == TimeOfDayPeriod::Count: at the top of AddSetting
(before calling HasDuplicateEntry/GetEntriesMut) check for that invalid enum
value, log a warning using the project's logging facility, and return early so
no SettingEntry is added, SaveUserSettings or ReapplyIfActive are invoked; keep
references to HasDuplicateEntry, GetEntriesMut, SaveUserSettings, and
ReapplyIfActive to help locate the correct spot to insert this validation.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 39-44: Add a compile-time check to guard the hard-coded names
array length against changes to the TimeOfDayPeriod enum: inside
SceneSettingsManager::GetPeriodName, add a static_assert that the number of
entries in the local names array equals kPeriodCount (referencing names and
kPeriodCount) so any future enum change will produce a build error if the table
is not updated.
- Around line 1-4: Update the PR/commit title to follow Conventional Commits
(lowercase description, e.g., "feat(ui): time of day") and add an issue
reference in the PR description like "Implements #<id>" or "Addresses #<id>";
locate the change related to SceneSettingsManager (file/class
SceneSettingsManager or the commit that added/modified
src/SceneSettingsManager.cpp) and amend the commit message or PR title
accordingly and add the appropriate issue line in the PR description before
merging.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/SceneSettingsManager.cpp (3)
239-285:⚠️ Potential issue | 🟡 MinorBlock TOD entries with invalid period to avoid ghost records.
AddSetting()allowsTimeOfDayPeriod::Countfor TOD entries, which creates entries that save but never apply. Guard it and log a warning.As per coding guidelines: “Validate user input and prevent DirectX crashes from malformed configurations.”🔧 Proposed fix
void SceneSettingsManager::AddSetting(SceneType type, const std::string& featureShortName, const std::string& settingKey, const json& value, TimeOfDayPeriod period) { + if (type == SceneType::TimeOfDay && period == TimeOfDayPeriod::Count) { + logger::warn("[SceneSettings] Skipping TOD entry with invalid period: {}.{}", featureShortName, settingKey); + return; + } if (HasDuplicateEntry(type, featureShortName, settingKey, EntrySource::User, period)) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 239 - 285, AddSetting currently allows creating TimeOfDay entries with an invalid period (TimeOfDayPeriod::Count), producing saved but never-applied "ghost" records; update SceneSettingsManager::AddSetting to validate the period when type == SceneType::TimeOfDay and reject TimeOfDayPeriod::Count (log a warning) before calling HasDuplicateEntry or pushing the entry, so invalid TOD entries are blocked and no ghost records are saved (use the existing logging facility and return early on invalid period).
15-24:⚠️ Potential issue | 🟡 MinorAdjust PR title to conventional commit style and add issue link if any.
The current title uses uppercase scope/description. Suggestfeat(ui): time of dayand, if there’s a tracking issue, add “Implements #…/Related to #…”.As per coding guidelines: “Conventional Commit Titles … Format: type(scope): description … lowercase description, no ending period” and “Issue References … ‘Implements
#123’/‘Related to#123’ for features.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 15 - 24, Change the PR title to follow Conventional Commits (e.g., use "feat(ui): time of day" — lowercase description, no trailing period) and, if there is a tracking issue, append an issue reference like "Implements `#123`" or "Related to `#123`"; this is independent of the code in SceneSettingsManager::GetSceneTypeName in src/SceneSettingsManager.cpp but do the PR title edit and add the issue link to the PR/commit message accordingly.
918-948:⚠️ Potential issue | 🟠 MajorGuard
featureandsettingfields with type checks to prevent JSON parse exceptions on malformed user config.
get<std::string>()throws on non-string values. Currently only existence is checked; add type validation and skip bad entries with warnings, matching the pattern already established forperiodfield parsing (lines 937–941).Proposed fix
- entry.featureShortName = item["feature"].get<std::string>(); - entry.settingKey = item["setting"].get<std::string>(); + if (!item["feature"].is_string() || !item["setting"].is_string()) { + logger::warn("SceneSettingsManager: Entry has non-string feature/setting (feature type: {}, setting type: {}) — skipping", + item["feature"].type_name(), item["setting"].type_name()); + continue; + } + entry.featureShortName = item["feature"].get<std::string>(); + entry.settingKey = item["setting"].get<std::string>();Without these checks, malformed JSON causes the entire settings file to fail loading instead of gracefully skipping the bad entry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 918 - 948, The code currently assumes item["feature"] and item["setting"] are strings and calls get<std::string>() which will throw on malformed JSON; update the loop that fills SettingEntry (in the GetEntriesMut/type handling) to first validate item.contains("feature") && item["feature"].is_string() and item.contains("setting") && item["setting"].is_string(), and if either check fails call logger::warn with context including the offending item type (use item["feature"].type_name() / item["setting"].type_name() where available) and continue to skip the entry; keep the existing TimeOfDay period parsing (GetPeriodFromName, TimeOfDayPeriod::Count) unchanged but ensure the earlier string checks prevent exceptions when creating SettingEntry.featureShortName and SettingEntry.settingKey.
🧹 Nitpick comments (2)
src/SceneSettingsManager.cpp (2)
55-65: Verify SE/AE/VR safety for calendar/sky hour access.
Please confirmRE::Calendar::gameHourandRE::Sky::currentGameHouroffsets are correct across SE/AE/VR and considerREL::RelocateMember()+ runtime detection for version-safe access.As per coding guidelines: “Warn about SE/AE/VR compatibility issues and suggest REL::RelocateMember() patterns for runtime compatibility” and “Ensure changes work across SE/AE/VR variants using runtime detection.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 55 - 65, SceneSettingsManager::GetCurrentGameHour directly dereferences calendar->gameHour and sky->currentGameHour which may have different offsets across SE/AE/VR; update the accessor to use runtime-safe member relocation and version detection (e.g., use REL::Module::IsEditor() / runtime variant checks or existing runtime detection utilities) and REL::RelocateMember() to compute the correct offsets for RE::Calendar::gameHour and RE::Sky::currentGameHour before reading them; ensure you check the relocated pointer/float for null/validity and fall back to the current 12.0f default if access fails, and keep the current clamping logic in GetCurrentGameHour.
39-53: Make period parsing resilient to case differences.
GetPeriodFromName()requires exact casing, so user-edited JSON like “dawn” will be rejected. Consider normalizing case to make configs more forgiving.As per coding guidelines: “Validate user input and prevent DirectX crashes from malformed configurations.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 39 - 53, GetPeriodFromName currently does a case-sensitive comparison against GetPeriodName, so names like "dawn" fail; make parsing case-insensitive by normalizing the input and the compared period names to a common case (e.g., lowercase) before comparing. Update SceneSettingsManager::GetPeriodFromName to transform the input std::string (and/or the GetPeriodName() result) using a locale-independent ASCII tolower conversion (or std::transform with ::tolower) and then compare against the lowercased GetPeriodName(static_cast<TimeOfDayPeriod>(i)) for i in [0, kPeriodCount), still returning TimeOfDayPeriod::Count on no match. Ensure you reference TimeOfDayPeriod, GetPeriodFromName, GetPeriodName, and kPeriodCount when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 738-859: The non-float branch in
SceneSettingsManager::ApplyTimeOfDayBlended copies period JSON values into
blendedValue without validating their types, which can cause json::type_error
when feature->LoadSettings() later calls .get<T>(); fix by checking each
pr.value's type against the baseline's type (from savedTimeOfDayBaseline) using
DetectSettingType(baseline) and the json type predicates (e.g.,
is_boolean/is_string/is_number_integer) before assigning blendedValue, fall back
to the baseline when the period value's type mismatches (and log a warning), and
keep lastAppliedTODOther/cache updates unchanged so only validated values are
applied to Feature::FindFeatureByShortName(...)->LoadSettings().
---
Outside diff comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 239-285: AddSetting currently allows creating TimeOfDay entries
with an invalid period (TimeOfDayPeriod::Count), producing saved but
never-applied "ghost" records; update SceneSettingsManager::AddSetting to
validate the period when type == SceneType::TimeOfDay and reject
TimeOfDayPeriod::Count (log a warning) before calling HasDuplicateEntry or
pushing the entry, so invalid TOD entries are blocked and no ghost records are
saved (use the existing logging facility and return early on invalid period).
- Around line 15-24: Change the PR title to follow Conventional Commits (e.g.,
use "feat(ui): time of day" — lowercase description, no trailing period) and, if
there is a tracking issue, append an issue reference like "Implements `#123`" or
"Related to `#123`"; this is independent of the code in
SceneSettingsManager::GetSceneTypeName in src/SceneSettingsManager.cpp but do
the PR title edit and add the issue link to the PR/commit message accordingly.
- Around line 918-948: The code currently assumes item["feature"] and
item["setting"] are strings and calls get<std::string>() which will throw on
malformed JSON; update the loop that fills SettingEntry (in the
GetEntriesMut/type handling) to first validate item.contains("feature") &&
item["feature"].is_string() and item.contains("setting") &&
item["setting"].is_string(), and if either check fails call logger::warn with
context including the offending item type (use item["feature"].type_name() /
item["setting"].type_name() where available) and continue to skip the entry;
keep the existing TimeOfDay period parsing (GetPeriodFromName,
TimeOfDayPeriod::Count) unchanged but ensure the earlier string checks prevent
exceptions when creating SettingEntry.featureShortName and
SettingEntry.settingKey.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 55-65: SceneSettingsManager::GetCurrentGameHour directly
dereferences calendar->gameHour and sky->currentGameHour which may have
different offsets across SE/AE/VR; update the accessor to use runtime-safe
member relocation and version detection (e.g., use REL::Module::IsEditor() /
runtime variant checks or existing runtime detection utilities) and
REL::RelocateMember() to compute the correct offsets for RE::Calendar::gameHour
and RE::Sky::currentGameHour before reading them; ensure you check the relocated
pointer/float for null/validity and fall back to the current 12.0f default if
access fails, and keep the current clamping logic in GetCurrentGameHour.
- Around line 39-53: GetPeriodFromName currently does a case-sensitive
comparison against GetPeriodName, so names like "dawn" fail; make parsing
case-insensitive by normalizing the input and the compared period names to a
common case (e.g., lowercase) before comparing. Update
SceneSettingsManager::GetPeriodFromName to transform the input std::string
(and/or the GetPeriodName() result) using a locale-independent ASCII tolower
conversion (or std::transform with ::tolower) and then compare against the
lowercased GetPeriodName(static_cast<TimeOfDayPeriod>(i)) for i in [0,
kPeriodCount), still returning TimeOfDayPeriod::Count on no match. Ensure you
reference TimeOfDayPeriod, GetPeriodFromName, GetPeriodName, and kPeriodCount
when making the change.
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.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SceneSettingsManager.cpp (1)
300-320:⚠️ Potential issue | 🟠 MajorValidate
TimeOfDayperiod before inserting entries
AddSettingacceptsperiod = TimeOfDayPeriod::Count(or other invalid cast values). Those entries are then skipped by blending, and they serialize without"period"(Line 957), creating ghost/inert entries and reload loss.🛠️ Proposed fix
void SceneSettingsManager::AddSetting(SceneType type, const std::string& featureShortName, const std::string& settingKey, const json& value, TimeOfDayPeriod period) { // TOD only supports float settings (smooth interpolation) - if (type == SceneType::TimeOfDay && DetectSettingType(value) != SettingType::Float) { - logger::warn("[SceneSettings] Rejecting non-float TOD setting: {}.{}", featureShortName, settingKey); - return; - } + if (type == SceneType::TimeOfDay) { + const int periodIdx = static_cast<int>(period); + if (period == TimeOfDayPeriod::Count || periodIdx < 0 || periodIdx >= kPeriodCount) { + logger::warn("[SceneSettings] Rejecting TOD setting with invalid period: {}.{}", featureShortName, settingKey); + return; + } + if (DetectSettingType(value) != SettingType::Float) { + logger::warn("[SceneSettings] Rejecting non-float TOD setting: {}.{}", featureShortName, settingKey); + return; + } + }As per coding guidelines: "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 300 - 320, Add validation in SceneSettingsManager::AddSetting to reject invalid TimeOfDayPeriod values (e.g., TimeOfDayPeriod::Count or any out-of-range/invalid cast) before creating or pushing the SettingEntry: if type == SceneType::TimeOfDay and period is not a valid TimeOfDayPeriod, log a warning (similar style to the existing logger::warn) and return early; ensure you reference the TimeOfDayPeriod enum for validation and avoid pushing into vec returned by GetEntriesMut or calling HasDuplicateEntry for invalid periods so serialization and blending code (which expects a valid period) won't produce ghost/inert entries.
🧹 Nitpick comments (2)
src/SceneSettingsManager.cpp (2)
1-1: Optional: tighten PR metadata for traceabilityConsider a conventional-commit style title like
feat(ui): add time-of-day scene settings, and add an issue link in the PR body (e.g.,Implements #<id>/Addresses #<id>).As per coding guidelines: "
**/*: provide suggestions for Conventional Commit Titles and Issue References for features/bugfixes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1, Update the PR title and body to follow conventional commits and include an issue reference: rename the PR title to a conventional-commit style (for example "feat(ui): add time-of-day scene settings") and add an issue link in the PR description like "Implements #<id>" or "Addresses #<id>"; mention the affected file/class (SceneSettingsManager.cpp / SceneSettingsManager) in the PR body so reviewers can quickly map the change to the code.
1007-1032: Keep TOD load behavior consistent with the float-only contract
AddSettingand overwrite discovery both reject non-float TOD entries, butLoadUserSettingscurrently accepts them. Consider filtering non-float TOD values here too, so malformed JSON can’t bypass the runtime contract.♻️ Suggested hardening
if (type == SceneType::TimeOfDay) { if (!item.contains("period")) { logger::warn("SceneSettingsManager: TimeOfDay entry for feature '{}' key '{}' is missing 'period' — skipping to avoid ghost entry", entry.featureShortName, entry.settingKey); continue; } + if (DetectSettingType(entry.value) != SettingType::Float) { + logger::warn("SceneSettingsManager: TimeOfDay entry for feature '{}' key '{}' has non-float value — skipping", + entry.featureShortName, entry.settingKey); + continue; + } if (!item["period"].is_string()) {As per coding guidelines: "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 1007 - 1032, LoadUserSettings currently accepts TimeOfDay entries with non-float values; mirror the validation used by AddSetting by checking the JSON "value" for TimeOfDay entries and rejecting any non-numeric/ non-float values before pushing the entry. Inside the block handling SceneType::TimeOfDay (alongside the existing period checks and GetPeriodFromName/TimeOfDayPeriod::Count validation), verify item.contains("value") and that item["value"].is_number() (or convertible to float); if not, emit a logger::warn mentioning entry.featureShortName and entry.settingKey and continue to skip the entry so HasDuplicateEntry / vec.push_back(std::move(entry)) never receives malformed TOD values. Ensure this keeps behavior consistent with AddSetting and other overwrite discovery paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 300-320: Add validation in SceneSettingsManager::AddSetting to
reject invalid TimeOfDayPeriod values (e.g., TimeOfDayPeriod::Count or any
out-of-range/invalid cast) before creating or pushing the SettingEntry: if type
== SceneType::TimeOfDay and period is not a valid TimeOfDayPeriod, log a warning
(similar style to the existing logger::warn) and return early; ensure you
reference the TimeOfDayPeriod enum for validation and avoid pushing into vec
returned by GetEntriesMut or calling HasDuplicateEntry for invalid periods so
serialization and blending code (which expects a valid period) won't produce
ghost/inert entries.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1: Update the PR title and body to follow conventional commits and
include an issue reference: rename the PR title to a conventional-commit style
(for example "feat(ui): add time-of-day scene settings") and add an issue link
in the PR description like "Implements #<id>" or "Addresses #<id>"; mention the
affected file/class (SceneSettingsManager.cpp / SceneSettingsManager) in the PR
body so reviewers can quickly map the change to the code.
- Around line 1007-1032: LoadUserSettings currently accepts TimeOfDay entries
with non-float values; mirror the validation used by AddSetting by checking the
JSON "value" for TimeOfDay entries and rejecting any non-numeric/ non-float
values before pushing the entry. Inside the block handling SceneType::TimeOfDay
(alongside the existing period checks and
GetPeriodFromName/TimeOfDayPeriod::Count validation), verify
item.contains("value") and that item["value"].is_number() (or convertible to
float); if not, emit a logger::warn mentioning entry.featureShortName and
entry.settingKey and continue to skip the entry so HasDuplicateEntry /
vec.push_back(std::move(entry)) never receives malformed TOD values. Ensure this
keeps behavior consistent with AddSetting and other overwrite discovery paths.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SceneSettingsManager.cpp (1)
448-460:⚠️ Potential issue | 🟠 MajorValidate TOD edit values before storing
UpdateEntryValuecurrently accepts any JSON for TOD entries, bypassing the float-only guard used inAddSetting/overwrite loading. Invalid edits can still enter blending state.Suggested fix
void SceneSettingsManager::UpdateEntryValue(SceneType type, size_t index, const json& newValue, bool deferSave) { auto& vec = GetEntriesMut(type); if (index >= vec.size()) return; - vec[index].value = newValue; + if (type == SceneType::TimeOfDay) { + if (!newValue.is_number()) { + logger::warn("[SceneSettings] Rejecting non-numeric TOD edit: {}.{}", vec[index].featureShortName, vec[index].settingKey); + return; + } + float v = newValue.get<float>(); + if (!std::isfinite(v)) { + logger::warn("[SceneSettings] Rejecting non-finite TOD edit: {}.{}", vec[index].featureShortName, vec[index].settingKey); + return; + } + vec[index].value = v; + } else { + vec[index].value = newValue; + }As per coding guidelines: "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 448 - 460, UpdateEntryValue currently writes arbitrary JSON into vec[index].value for SceneType::TimeOfDay and can introduce non-float values into blending; before assigning newValue in UpdateEntryValue (the block that sets vec[index].value and the subsequent TimeOfDay branch), validate that newValue is a numeric float (match the same guard used by AddSetting/overwrite loading), and if it fails, reject the update (do not assign, do not call SaveUserSettings, do not touch lastBlendedHour or call ApplyTimeOfDayBlended) and optionally log or return an error; keep the existing behavior for non-TimeOfDay types and ensure the validation only triggers when type == SceneType::TimeOfDay and vec[index].source == EntrySource::User or whenever user edits are allowed.
🧹 Nitpick comments (1)
src/SceneSettingsManager.cpp (1)
1-1: Optional PR metadata polish: conventional title + issue linkSuggested title style:
feat(ui): add time-of-day scene settings(lowercase scope/description).
If this implements a tracked issue, addImplements #<id>(orAddresses #<id>) in the PR body.As per coding guidelines: "When reviewing PRs, please provide suggestions for Conventional Commit Titles ... [and] Issue References..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1, Update the PR metadata: change the PR title to follow Conventional Commits (e.g., "feat(ui): add time-of-day scene settings") and, if this change implements or addresses a tracked issue, add an issue reference like "Implements #<id>" or "Addresses #<id>" in the PR body; locate this change alongside the SceneSettingsManager.cpp-related commit so the PR title and body clearly reference the file and intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 914-917: The code treats malformed TOD period values as a numeric
zero by adding f to coveredFactor inside the pr.value->is_number() false branch;
instead, do not add f to coveredFactor for invalid samples so their weight falls
back to the baseline blend. In other words, in SceneSettingsManager where you
check pr.value->is_number() and currently call coveredFactor += f, remove that
increment (leave coveredFactor unchanged) so invalid entries are skipped and the
baseline blending logic receives their weight.
- Around line 1034-1039: Load path currently accepts any JSON number
(entry.value.is_number()), causing inconsistency with AddSetting which enforces
float-only TOD values; update the TimeOfDay loading code in SceneSettingsManager
to only accept floating-point numbers by replacing entry.value.is_number() with
entry.value.is_number_float(), and adjust the warn message to state "non-float
value" (so integers/unsigned are rejected the same way as AddSetting),
referencing the existing AddSetting behavior and the entry.value checks in this
load block.
---
Outside diff comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 448-460: UpdateEntryValue currently writes arbitrary JSON into
vec[index].value for SceneType::TimeOfDay and can introduce non-float values
into blending; before assigning newValue in UpdateEntryValue (the block that
sets vec[index].value and the subsequent TimeOfDay branch), validate that
newValue is a numeric float (match the same guard used by AddSetting/overwrite
loading), and if it fails, reject the update (do not assign, do not call
SaveUserSettings, do not touch lastBlendedHour or call ApplyTimeOfDayBlended)
and optionally log or return an error; keep the existing behavior for
non-TimeOfDay types and ensure the validation only triggers when type ==
SceneType::TimeOfDay and vec[index].source == EntrySource::User or whenever user
edits are allowed.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1: Update the PR metadata: change the PR title to follow Conventional
Commits (e.g., "feat(ui): add time-of-day scene settings") and, if this change
implements or addresses a tracked issue, add an issue reference like "Implements
#<id>" or "Addresses #<id>" in the PR body; locate this change alongside the
SceneSettingsManager.cpp-related commit so the PR title and body clearly
reference the file and intent.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SceneSettingsManager.cpp (1)
1003-1011:⚠️ Potential issue | 🟠 MajorHarden per-item parsing so one malformed entry doesn't abort the entire settings file load.
Lines 1008–1009 directly call
.get<std::string>()onitem["feature"]anditem["setting"]without validating type first. If either field is not a string (e.g.,"feature": 123), it throws and the outer catch at line 1050 fails the entire file load, skipping all remaining valid entries. The same function already implements the correct pattern for the"period"field (lines 1021–1023) using.is_string()checks with logging.🛠️ Proposed fix
for (const auto& item : data) { if (!item.contains("feature") || !item.contains("setting") || !item.contains("value")) continue; + if (!item["feature"].is_string() || !item["setting"].is_string()) { + logger::warn("SceneSettingsManager: skipping entry with non-string feature/setting (feature type: {}, setting type: {})", + item.contains("feature") ? item["feature"].type_name() : "missing", + item.contains("setting") ? item["setting"].type_name() : "missing"); + continue; + } SettingEntry entry; entry.featureShortName = item["feature"].get<std::string>(); entry.settingKey = item["setting"].get<std::string>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 1003 - 1011, The loop that builds SettingEntry objects currently calls item["feature"].get<std::string>() and item["setting"].get<std::string>() without type checks, which can throw and abort the whole load; update the parsing in the for-loop that constructs SettingEntry (the code that assigns entry.featureShortName and entry.settingKey) to first verify item["feature"].is_string() and item["setting"].is_string(), log a warning if either is missing or the wrong type (consistent with the existing period handling), and skip that single item instead of throwing; keep assigning entry.value and entry.paused as before and only push the entry when both string checks pass.
🧹 Nitpick comments (2)
src/SceneSettingsManager.cpp (2)
1-1: PR metadata polish: align title casing and add issue linkage if applicable.Suggested title:
feat(ui): add time-of-day scene settings
If this maps to a tracked issue, addImplements #<id>(orAddresses #<id>) in the PR body.As per coding guidelines: "
**/*: provide suggestions for Conventional Commit titles and Issue References when applicable."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1, Update the PR metadata to use the suggested conventional commit title casing by renaming the PR title to "feat(ui): add time-of-day scene settings" and, if this work maps to a tracked issue, add an issue reference line (e.g., "Implements #<id>" or "Addresses #<id>") in the PR body; this applies to the change touching SceneSettingsManager (class/filename SceneSettingsManager.cpp) so include that context in the PR description.
442-460: Keep TOD type invariants consistent inUpdateEntryValue.
AddSetting, overwrite discovery, and load all enforce float-only TOD values, but edit path currently accepts any JSON type. Add the same guard here to keep runtime behavior consistent.♻️ Proposed fix
void SceneSettingsManager::UpdateEntryValue(SceneType type, size_t index, const json& newValue, bool deferSave) { auto& vec = GetEntriesMut(type); if (index >= vec.size()) return; + + if (type == SceneType::TimeOfDay && DetectSettingType(newValue) != SettingType::Float) { + logger::warn("[SceneSettings] Rejecting non-float TOD value edit: {}.{}", vec[index].featureShortName, vec[index].settingKey); + return; + } vec[index].value = newValue;As per coding guidelines: "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 442 - 460, UpdateEntryValue currently accepts any JSON for SceneType::TimeOfDay which breaks the float-only invariants enforced elsewhere; add a guard at the start of the TimeOfDay branch in SceneSettingsManager::UpdateEntryValue to validate and normalize newValue to float-only form: if newValue is a number convert/store as a float JSON, if it is an array ensure every element is numeric and convert each to float, otherwise reject the update (do not modify vec[index].value) and return; keep the existing behavior of calling SaveUserSettings (via SaveUserSettings) when source is EntrySource::User and deferSave is false, and preserve the existing time-of-day post-update logic (reset lastBlendedHour, check isTimeOfDayActive, call ApplyTimeOfDayBlended) only after successful validation/normalization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 1003-1011: The loop that builds SettingEntry objects currently
calls item["feature"].get<std::string>() and item["setting"].get<std::string>()
without type checks, which can throw and abort the whole load; update the
parsing in the for-loop that constructs SettingEntry (the code that assigns
entry.featureShortName and entry.settingKey) to first verify
item["feature"].is_string() and item["setting"].is_string(), log a warning if
either is missing or the wrong type (consistent with the existing period
handling), and skip that single item instead of throwing; keep assigning
entry.value and entry.paused as before and only push the entry when both string
checks pass.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1: Update the PR metadata to use the suggested conventional commit title
casing by renaming the PR title to "feat(ui): add time-of-day scene settings"
and, if this work maps to a tracked issue, add an issue reference line (e.g.,
"Implements #<id>" or "Addresses #<id>") in the PR body; this applies to the
change touching SceneSettingsManager (class/filename SceneSettingsManager.cpp)
so include that context in the PR description.
- Around line 442-460: UpdateEntryValue currently accepts any JSON for
SceneType::TimeOfDay which breaks the float-only invariants enforced elsewhere;
add a guard at the start of the TimeOfDay branch in
SceneSettingsManager::UpdateEntryValue to validate and normalize newValue to
float-only form: if newValue is a number convert/store as a float JSON, if it is
an array ensure every element is numeric and convert each to float, otherwise
reject the update (do not modify vec[index].value) and return; keep the existing
behavior of calling SaveUserSettings (via SaveUserSettings) when source is
EntrySource::User and deferSave is false, and preserve the existing time-of-day
post-update logic (reset lastBlendedHour, check isTimeOfDayActive, call
ApplyTimeOfDayBlended) only after successful validation/normalization.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/SceneSettingsManager.cpp (1)
1-1: Consider tightening PR metadata scope to match runtime changesThe title
feat(UI): Time of Dayundersells the large SceneSettingsManager runtime/persistence changes. A more precise option isfeat(scene-settings): add time-of-day blending. If this maps to a tracked task, add an issue reference (e.g.,Implements #<id>).As per coding guidelines: "Conventional Commit Titles ... type(scope): description" and "Issue References ... Implements
#123/ Addresses#123".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1, Update the PR/commit title and metadata to follow Conventional Commits and accurately reflect the runtime/persistence changes in SceneSettingsManager: change the title to something like "feat(scene-settings): add time-of-day blending" and, if this work maps to a tracked task, append an issue reference (e.g., "Implements #<id>"). Ensure the scope uses "scene-settings" and the description mentions "time-of-day blending" so it maps to changes in SceneSettingsManager (class/file SceneSettingsManager.cpp) and related persistence/runtime updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 1018-1029: The code currently uses item.value("paused", false)
which throws if paused exists but isn't a boolean; update the logic around
SettingEntry assignment to first check item.contains("paused") and
item["paused"].is_boolean() (or use is_null/is_boolean as appropriate) before
calling entry.paused = item.value("paused", false); if the paused field exists
but is not a boolean, log a warning via logger::warn referencing the
feature/setting (same context as the existing skip log) and leave entry.paused
as the default false; keep the rest of the population (entry.featureShortName,
entry.settingKey, entry.value, entry.source) unchanged so only the paused
extraction is guarded.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1: Update the PR/commit title and metadata to follow Conventional Commits
and accurately reflect the runtime/persistence changes in SceneSettingsManager:
change the title to something like "feat(scene-settings): add time-of-day
blending" and, if this work maps to a tracked task, append an issue reference
(e.g., "Implements #<id>"). Ensure the scope uses "scene-settings" and the
description mentions "time-of-day blending" so it maps to changes in
SceneSettingsManager (class/file SceneSettingsManager.cpp) and related
persistence/runtime updates.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SceneSettingsManager.cpp (1)
340-349:⚠️ Potential issue | 🟠 MajorHandle filesystem delete outcomes correctly before dropping overwrite entries
Line 345 can log a false error when
remove()returnsfalsewith noec(already missing file), and Line 399–409 removes in-memory overwrite entries even if on-disk deletion fails. That can make files reappear on next startup without clear diagnosis.🛠️ Suggested fix
@@ - if (std::filesystem::remove(filepath, ec)) + ec.clear(); + if (std::filesystem::remove(filepath, ec)) logger::info("[SceneSettings] Deleted overwrite file: {}", filepath.string()); - else + else if (ec) logger::error("[SceneSettings] Failed to delete overwrite file: {} ({})", filepath.string(), ec.message()); + else + logger::debug("[SceneSettings] Overwrite file already absent: {}", filepath.string()); @@ - for (const auto& entry : vec) { + std::unordered_set<std::string> failedDeletes; + for (const auto& entry : vec) { if (entry.source == EntrySource::Overwrite && !entry.sourceFilename.empty()) { @@ - std::filesystem::remove(filepath, ec); + ec.clear(); + const bool removed = std::filesystem::remove(filepath, ec); + if (!removed && ec) { + logger::error("[SceneSettings] Failed to delete overwrite file: {} ({})", filepath.string(), ec.message()); + failedDeletes.insert(entry.sourceFilename); + } } } @@ - std::erase_if(vec, [](const SettingEntry& e) { - return e.source == EntrySource::Overwrite; + std::erase_if(vec, [&](const SettingEntry& e) { + return e.source == EntrySource::Overwrite && + (e.sourceFilename.empty() || !failedDeletes.contains(e.sourceFilename)); });Based on learnings: "file deletion error handling improvements ... with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring."
Also applies to: 399-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 340 - 349, The delete handling currently treats any false return from std::filesystem::remove as an error and then still drops the in-memory overwrite entry; change the logic in SceneSettingsManager where it builds filepath (using GetOverwritesPath, entry.sourceFilename, entry.period, TimeOfDayPeriod) to inspect the std::error_code after remove: if remove succeeds or remove returns false with ec.value()==0 (meaning the file simply did not exist) treat as success and log info, but if ec indicates an actual error log an error and do NOT erase the corresponding in-memory overwrite entry; apply the same check before the in-memory removal loop (the block that currently removes overwrite entries around the 399–409 area) so disk-delete failures keep the entry intact.
🧹 Nitpick comments (1)
src/SceneSettingsManager.cpp (1)
1066-1066: Log accepted entry count instead of raw JSON item countLine 1066 logs
data.size(), even when many entries are skipped. Consider trackingloadedCountfor more accurate diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1066, The log at the logger::info call currently reports data.size() and typeName which reflects the raw JSON input count rather than the number of entries actually loaded; modify the loading routine in SceneSettingsManager.cpp to maintain a loadedCount (increment when an entry is successfully parsed/accepted) and replace data.size() with loadedCount in the logger::info message so it reports the accepted/loaded entries for typeName instead of the raw JSON item count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 340-349: The delete handling currently treats any false return
from std::filesystem::remove as an error and then still drops the in-memory
overwrite entry; change the logic in SceneSettingsManager where it builds
filepath (using GetOverwritesPath, entry.sourceFilename, entry.period,
TimeOfDayPeriod) to inspect the std::error_code after remove: if remove succeeds
or remove returns false with ec.value()==0 (meaning the file simply did not
exist) treat as success and log info, but if ec indicates an actual error log an
error and do NOT erase the corresponding in-memory overwrite entry; apply the
same check before the in-memory removal loop (the block that currently removes
overwrite entries around the 399–409 area) so disk-delete failures keep the
entry intact.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1066: The log at the logger::info call currently reports data.size() and
typeName which reflects the raw JSON input count rather than the number of
entries actually loaded; modify the loading routine in SceneSettingsManager.cpp
to maintain a loadedCount (increment when an entry is successfully
parsed/accepted) and replace data.size() with loadedCount in the logger::info
message so it reports the accepted/loaded entries for typeName instead of the
raw JSON item count.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SceneSettingsManager.cpp (1)
397-414:⚠️ Potential issue | 🟠 MajorDelete-all can desync memory from disk on I/O errors
At Line 412, overwrite entries are erased unconditionally even if file deletion failed earlier. That can leave undeleted files on disk which reappear on next load, while the current session loses control over them.
🛠️ Suggested fix
void SceneSettingsManager::DeleteAllOverwrites(SceneType type) { auto overwritesPath = GetOverwritesPath(type); - std::error_code ec; - auto& vec = GetEntriesMut(type); - for (const auto& entry : vec) { - if (entry.source == EntrySource::Overwrite && !entry.sourceFilename.empty()) { - // TOD overwrites live in per-period subfolders; use the same path - // construction as SaveOverwritesToDisk to ensure we hit the right file. - auto filepath = (type == SceneType::TimeOfDay && entry.period != TimeOfDayPeriod::Count) ? overwritesPath / GetPeriodName(entry.period) / entry.sourceFilename : overwritesPath / entry.sourceFilename; - std::filesystem::remove(filepath, ec); - } - } - - std::erase_if(vec, [](const SettingEntry& e) { - return e.source == EntrySource::Overwrite; - }); + std::erase_if(vec, [&](const SettingEntry& entry) { + if (entry.source != EntrySource::Overwrite) + return false; + + if (entry.sourceFilename.empty()) + return true; + + auto filepath = (type == SceneType::TimeOfDay && entry.period != TimeOfDayPeriod::Count) + ? overwritesPath / GetPeriodName(entry.period) / entry.sourceFilename + : overwritesPath / entry.sourceFilename; + + std::error_code ec; + std::filesystem::remove(filepath, ec); + if (ec) { + logger::error("[SceneSettings] Failed to delete overwrite file: {} ({}) — keeping entry", + filepath.string(), ec.message()); + return false; // keep entry active/visible if disk delete failed + } + return true; // removed or already missing + }); allOverwritesPausedMap[type] = false; ReapplyIfActive(); }Based on learnings: "file deletion error handling improvements ... are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 397 - 414, DeleteAllOverwrites currently removes SettingEntry items from memory regardless of whether std::filesystem::remove succeeded, which can desync disk and memory; modify SceneSettingsManager::DeleteAllOverwrites (and the loop using GetEntriesMut, GetOverwritesPath, GetPeriodName and EntrySource::Overwrite) to check the std::error_code ec after each std::filesystem::remove and only mark/collect entries as successfully deleted when ec is clear (or when the file no longer exists), log any failures, and after the loop erase only the entries that were actually deleted (e.g., build a list of filenames/indices that succeeded and pass a predicate to std::erase_if or remove them by those identifiers) so failed deletions remain in memory for retry or user visibility.
🧹 Nitpick comments (1)
src/SceneSettingsManager.cpp (1)
1-1: Consider tightening PR metadata to match repository conventionCurrent title is close; guideline-preferred style would be
feat(ui): add time-of-day scene settings(lowercase scope/description).
If this maps to a tracked issue, consider addingImplements #<id>in the PR description.As per coding guidelines: "
**/*: provide suggestions for Conventional Commit titles and Issue References."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1, Update the PR title and description to follow the repository's Conventional Commit style: change the title to the recommended form, e.g. "feat(ui): add time-of-day scene settings" (lowercase scope and description), and if this PR resolves a tracked issue include an "Implements #<id>" line in the PR description referencing that issue ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 397-414: DeleteAllOverwrites currently removes SettingEntry items
from memory regardless of whether std::filesystem::remove succeeded, which can
desync disk and memory; modify SceneSettingsManager::DeleteAllOverwrites (and
the loop using GetEntriesMut, GetOverwritesPath, GetPeriodName and
EntrySource::Overwrite) to check the std::error_code ec after each
std::filesystem::remove and only mark/collect entries as successfully deleted
when ec is clear (or when the file no longer exists), log any failures, and
after the loop erase only the entries that were actually deleted (e.g., build a
list of filenames/indices that succeeded and pass a predicate to std::erase_if
or remove them by those identifiers) so failed deletions remain in memory for
retry or user visibility.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1: Update the PR title and description to follow the repository's
Conventional Commit style: change the title to the recommended form, e.g.
"feat(ui): add time-of-day scene settings" (lowercase scope and description),
and if this PR resolves a tracked issue include an "Implements #<id>" line in
the PR description referencing that issue ID.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/SceneSettingsManager.cpp (1)
1-1: PR metadata: tighten title + issue linkage.Consider
feat(ui): add time-of-day scene settingsand add an issue keyword in the PR body (for example,Implements #<id>).As per coding guidelines: "
**/*: ... provide suggestions for Conventional Commit Titles ... and Issue References."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1, Update the PR title to follow Conventional Commits (e.g., "feat(ui): add time-of-day scene settings") and edit the PR body to include an issue reference using the suggested keyword format, for example add a line "Implements #<id>" to link the PR to the issue; ensure the commit/PR title and body match the repo's guideline for Conventional Commit titles and include the issue number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 470-481: The TimeOfDay branch in UpdateEntryValue
(SceneSettingsManager.cpp) accepts newValue.get<float>() without validating
finite-ness; change it to extract the float into a local (e.g., float tod =
newValue.get<float>()), check std::isfinite(tod) and if not finite log a warning
referencing vec[index].featureShortName and vec[index].settingKey and return,
otherwise assign vec[index].value = tod so only finite floats are stored; this
prevents NaN/Inf from entering the TOD blending state.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1: Update the PR title to follow Conventional Commits (e.g., "feat(ui):
add time-of-day scene settings") and edit the PR body to include an issue
reference using the suggested keyword format, for example add a line "Implements
#<id>" to link the PR to the issue; ensure the commit/PR title and body match
the repo's guideline for Conventional Commit titles and include the issue
number.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/SceneSettingsManager.cpp (2)
1-1: PR metadata polish: conventional title + issue link suggestion.Suggested title:
feat(ui): add time-of-day scene settings
Also consider adding an issue reference in the PR body, e.g.Implements #<issue-number>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1, Update the PR metadata: change the PR title to the conventional format suggested (e.g., "feat(ui): add time-of-day scene settings") and add an issue reference in the PR description (e.g., "Implements #<issue-number>"); mention that this change implements the SceneSettingsManager time-of-day feature (class/identifier SceneSettingsManager) so reviewers can immediately link the PR to the feature and issue number.
1057-1057: Consider warning on invalidpausedtype instead of silently coercing tofalse.Line 1057 is safe (no throw), but malformed configs become hard to diagnose without a warning.
As per coding guidelines: "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1057, When parsing the "paused" field currently we silently coerce any non-boolean to false; change the logic in the SceneSettingsManager parsing path that touches entry.paused and item so that if item.contains("paused") and item["paused"].is_boolean() you assign item["paused"].get<bool>(), but if item.contains("paused") and NOT is_boolean() you emit a warning (use the module's logging facility e.g., SceneSettingsManager or the existing logger used nearby) indicating the "paused" field has an invalid type and leave entry.paused at its default rather than silently coercing; this keeps behavior safe while surfacing malformed configs for diagnosis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 300-315: In AddSetting, when handling SceneType::TimeOfDay (in
SceneSettingsManager::AddSetting), also validate that the provided json float
value is finite before accepting it: after verifying DetectSettingType(value) ==
SettingType::Float, parse/extract the float and reject NaN/Inf (log a warn
similar to the other checks and return) so TOD entries cannot be created with
non-finite values; mirror the same non-finite check logic used in
UpdateEntryValue to keep invariants consistent (use the same float-finiteness
test and message pattern).
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1: Update the PR metadata: change the PR title to the conventional format
suggested (e.g., "feat(ui): add time-of-day scene settings") and add an issue
reference in the PR description (e.g., "Implements #<issue-number>"); mention
that this change implements the SceneSettingsManager time-of-day feature
(class/identifier SceneSettingsManager) so reviewers can immediately link the PR
to the feature and issue number.
- Line 1057: When parsing the "paused" field currently we silently coerce any
non-boolean to false; change the logic in the SceneSettingsManager parsing path
that touches entry.paused and item so that if item.contains("paused") and
item["paused"].is_boolean() you assign item["paused"].get<bool>(), but if
item.contains("paused") and NOT is_boolean() you emit a warning (use the
module's logging facility e.g., SceneSettingsManager or the existing logger used
nearby) indicating the "paused" field has an invalid type and leave entry.paused
at its default rather than silently coercing; this keeps behavior safe while
surfacing malformed configs for diagnosis.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/SceneSettingsManager.cpp (2)
1-1: PR metadata polish: conventional title + issue linkage.Suggested title:
feat(ui): add time-of-day scene settings
Also consider adding an issue reference in the PR body (e.g.,Implements #<id>/Addresses #<id>), if applicable.As per coding guidelines: "When reviewing PRs, please provide suggestions for: Conventional Commit Titles ... Issue References ...".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1, Update the PR title to follow Conventional Commits using the suggested title "feat(ui): add time-of-day scene settings" and add an issue reference line (e.g., "Implements #<id>" or "Addresses #<id>") in the PR body; ensure the PR description explicitly links to the related issue and uses the conventional commit prefix "feat(ui):" so reviewers and CI tooling can pick up the metadata.
596-610: Use active TOD entries for activation checks, not just non-empty vector.
hasEntries = !GetEntries(SceneType::TimeOfDay).empty()keeps TOD active (and updating) even when all TOD entries are paused or feature-paused.Suggested fix
- bool hasEntries = !GetEntries(SceneType::TimeOfDay).empty(); + const auto& todEntries = GetEntries(SceneType::TimeOfDay); + bool hasEntries = std::any_of(todEntries.begin(), todEntries.end(), + [this](const SettingEntry& e) { return IsEntryActive(e); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 596 - 610, Replace the simplistic hasEntries check with a test for actually active TOD entries (i.e., entries that are not paused and not feature-paused) instead of !GetEntries(SceneType::TimeOfDay).empty(); compute hasEntries by iterating GetEntries(SceneType::TimeOfDay) and checking each entry’s active flags (e.g., !entry->IsPaused() && !entry->IsFeaturePaused() or equivalent methods) so the existing branches that call RevertTimeOfDayBaseline, SaveTimeOfDayBaseline, ApplyTimeOfDayBlended, DeactivateTimeOfDay and ActivateTimeOfDay only run when there are truly active TOD entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1063: The code silently coerces non-boolean "paused" values to false;
update the logic around the entry.paused assignment so that if
item.contains("paused") and item["paused"].is_boolean() is false, you log a
warning (including the offending value/type and some context such as the entry
id or index) and then keep the safe fallback of false; locate the assignment to
entry.paused and add a conditional branch that logs the malformed type before
setting entry.paused = false (reference symbols: entry.paused and
item["paused"]).
- Around line 54-64: SceneSettingsManager::GetCurrentGameHour currently clamps
values to 24.0f, but callers expect times normalized to the half-open range
[0,24), so change the return logic in GetCurrentGameHour to wrap values into
[0,24) instead of clamping to 24.0f: after obtaining the raw hour
(calendar->gameHour->value or sky->currentGameHour), normalize it with a wrap
operation (e.g., ((hour % 24) + 24) % 24 or using std::fmod with +24 adjustment)
to ensure values equal to 24 become 0 and negative hours are corrected, then
return the normalized value; keep the existing use of calendar and sky to obtain
the raw hour.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1: Update the PR title to follow Conventional Commits using the suggested
title "feat(ui): add time-of-day scene settings" and add an issue reference line
(e.g., "Implements #<id>" or "Addresses #<id>") in the PR body; ensure the PR
description explicitly links to the related issue and uses the conventional
commit prefix "feat(ui):" so reviewers and CI tooling can pick up the metadata.
- Around line 596-610: Replace the simplistic hasEntries check with a test for
actually active TOD entries (i.e., entries that are not paused and not
feature-paused) instead of !GetEntries(SceneType::TimeOfDay).empty(); compute
hasEntries by iterating GetEntries(SceneType::TimeOfDay) and checking each
entry’s active flags (e.g., !entry->IsPaused() && !entry->IsFeaturePaused() or
equivalent methods) so the existing branches that call RevertTimeOfDayBaseline,
SaveTimeOfDayBaseline, ApplyTimeOfDayBlended, DeactivateTimeOfDay and
ActivateTimeOfDay only run when there are truly active TOD entries.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/SceneSettingsManager.cpp (1)
54-64:⚠️ Potential issue | 🟡 MinorNormalize game hour into
[0, 24)instead of clamping to24.0f.At Line 60 and Line 63, clamping can return exactly
24.0f, while TOD period checks use half-open ranges (h < end). Wrapping avoids edge-case misclassification at midnight and matches the normalization intent in Line 73.Suggested fix
float SceneSettingsManager::GetCurrentGameHour() { // Prefer calendar (ground truth), which the Weather Editor slider writes to. // sky->currentGameHour may lag when timeScale is 0 (time paused). + auto normalizeHour = [](float h) { + if (!std::isfinite(h)) + return 12.0f; + h = std::fmod(h, 24.0f); + if (h < 0.0f) + h += 24.0f; + return h; + }; + auto calendar = globals::game::calendar ? globals::game::calendar : RE::Calendar::GetSingleton(); if (calendar && calendar->gameHour) - return std::clamp(calendar->gameHour->value, 0.0f, 24.0f); + return normalizeHour(calendar->gameHour->value); auto sky = globals::game::sky; - return sky ? std::clamp(sky->currentGameHour, 0.0f, 24.0f) : 12.0f; + return sky ? normalizeHour(sky->currentGameHour) : 12.0f; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 54 - 64, GetCurrentGameHour currently clamps calendar->gameHour->value and sky->currentGameHour to 24.0f which allows a result of 24.0f; instead normalize into the half-open range [0,24) by replacing the std::clamp calls with a wrap-normalization: read the raw hour (calendar->gameHour->value or sky->currentGameHour), compute hour = std::fmod(hour, 24.0f); if (hour < 0.0f) hour += 24.0f; if (hour == 24.0f) hour = 0.0f; then return hour so midnight is 0 and never 24.
🧹 Nitpick comments (1)
src/SceneSettingsManager.cpp (1)
1-1: PR metadata tweak: align title/reference with repo convention.Suggested title:
feat(ui): add time-of-day scene settings
If this maps to an issue, add:Implements #<issue-number>in the PR description.As per coding guidelines: "Conventional Commit Titles ... Style: lowercase description" and "Issue References ... 'Implements
#123' for features."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` at line 1, Update the PR metadata to follow the repo's Conventional Commit style: change the PR title to "feat(ui): add time-of-day scene settings" (all lowercase description) and, if this change implements an issue, add "Implements #<issue-number>" to the PR description; mention the SceneSettingsManager (and related class names) in the description so the intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 54-64: GetCurrentGameHour currently clamps
calendar->gameHour->value and sky->currentGameHour to 24.0f which allows a
result of 24.0f; instead normalize into the half-open range [0,24) by replacing
the std::clamp calls with a wrap-normalization: read the raw hour
(calendar->gameHour->value or sky->currentGameHour), compute hour =
std::fmod(hour, 24.0f); if (hour < 0.0f) hour += 24.0f; if (hour == 24.0f) hour
= 0.0f; then return hour so midnight is 0 and never 24.
---
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Line 1: Update the PR metadata to follow the repo's Conventional Commit style:
change the PR title to "feat(ui): add time-of-day scene settings" (all lowercase
description) and, if this change implements an issue, add "Implements
#<issue-number>" to the PR description; mention the SceneSettingsManager (and
related class names) in the description so the intent is clear.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/SceneSettingsManager.cpp (2)
602-617: Only activate TOD when at least one entry is active.
hasEntries/empty-check includes paused entries, so TOD can run with no effective writes. Prefer “has active entries” gating.Suggested patch
@@ - bool hasEntries = !GetEntries(SceneType::TimeOfDay).empty(); + const auto& todEntries = GetEntries(SceneType::TimeOfDay); + bool hasActiveEntries = std::any_of(todEntries.begin(), todEntries.end(), + [this](const SettingEntry& e) { return IsEntryActive(e); }); @@ - if (hasEntries) { + if (hasActiveEntries) { @@ - } else if (isExterior && hasEntries && !isCurrentlyApplied) { + } else if (isExterior && hasActiveEntries && !isCurrentlyApplied) { @@ - if (isTimeOfDayActive || GetEntries(SceneType::TimeOfDay).empty()) + const auto& todEntries = GetEntries(SceneType::TimeOfDay); + bool hasActiveEntries = std::any_of(todEntries.begin(), todEntries.end(), + [this](const SettingEntry& e) { return IsEntryActive(e); }); + if (isTimeOfDayActive || !hasActiveEntries) return;Also applies to: 780-781
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 602 - 617, The current gating uses hasEntries = !GetEntries(SceneType::TimeOfDay).empty() which counts paused/inactive entries; change the checks to require at least one active (non-paused) TOD entry by iterating GetEntries(SceneType::TimeOfDay) and testing the entry active flag (e.g. entry->IsActive() or !entry->IsPaused()/entry->enabled) instead of empty(), then use that boolean in the existing branches (the block around RevertTimeOfDayBaseline/SaveTimeOfDayBaseline/ApplyTimeOfDayBlended, DeactivateTimeOfDay, and the ActivateTimeOfDay path); make the same replacement for the other occurrence referenced (around lines 780-781).
1095-1100: Validate non-finite TOD values at ingestion, not only during blending.TOD load/discovery currently enforces float type but not finite value. Reject/normalize here to keep invariants consistent with
AddSetting/UpdateEntryValue.As per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".Suggested patch
@@ - // TOD only supports float settings — use DetectSettingType to match AddSetting/DiscoverOverwrites + // TOD only supports finite float settings — match AddSetting/UpdateEntryValue invariants if (DetectSettingType(entry.value) != SettingType::Float) { logger::warn("SceneSettingsManager: TimeOfDay entry for feature '{}' key '{}' has non-float value (type: {}) — skipping", entry.featureShortName, entry.settingKey, entry.value.type_name()); continue; } + float todValue = entry.value.get<float>(); + if (!std::isfinite(todValue)) { + logger::warn("SceneSettingsManager: TimeOfDay entry for feature '{}' key '{}' has non-finite value — skipping", + entry.featureShortName, entry.settingKey); + continue; + } + entry.value = todValue; @@ - // TOD only supports float settings (smooth interpolation) + // TOD only supports finite float settings (smooth interpolation) if (type == SceneType::TimeOfDay && DetectSettingType(settingValue) != SettingType::Float) { logger::warn("[SceneSettings] Skipping overwrite '{}': non-float setting '{}' not allowed in Time of Day", filename, settingKey); continue; } + if (type == SceneType::TimeOfDay) { + float todValue = settingValue.get<float>(); + if (!std::isfinite(todValue)) { + logger::warn("[SceneSettings] Skipping overwrite '{}': non-finite setting '{}' not allowed in Time of Day", filename, settingKey); + continue; + } + settingValue = todValue; + }Also applies to: 1208-1211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.cpp` around lines 1095 - 1100, The TimeOfDay ingestion currently only checks that entry.value is a float via DetectSettingType and skips other types, but does not validate that the float is finite; update the ingestion path that uses DetectSettingType / SettingType::Float to also extract the float from entry.value and reject or normalize non-finite values (NaN/inf) using std::isfinite before accepting the entry (mirroring the invariants in AddSetting and UpdateEntryValue); apply the same finite-value check to the other TOD handling block mentioned (the code around the other occurrence at 1208-1211) and log a clear warning via logger::warn when rejecting a non-finite TOD value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/SceneSettingsManager.cpp`:
- Around line 602-617: The current gating uses hasEntries =
!GetEntries(SceneType::TimeOfDay).empty() which counts paused/inactive entries;
change the checks to require at least one active (non-paused) TOD entry by
iterating GetEntries(SceneType::TimeOfDay) and testing the entry active flag
(e.g. entry->IsActive() or !entry->IsPaused()/entry->enabled) instead of
empty(), then use that boolean in the existing branches (the block around
RevertTimeOfDayBaseline/SaveTimeOfDayBaseline/ApplyTimeOfDayBlended,
DeactivateTimeOfDay, and the ActivateTimeOfDay path); make the same replacement
for the other occurrence referenced (around lines 780-781).
- Around line 1095-1100: The TimeOfDay ingestion currently only checks that
entry.value is a float via DetectSettingType and skips other types, but does not
validate that the float is finite; update the ingestion path that uses
DetectSettingType / SettingType::Float to also extract the float from
entry.value and reject or normalize non-finite values (NaN/inf) using
std::isfinite before accepting the entry (mirroring the invariants in AddSetting
and UpdateEntryValue); apply the same finite-value check to the other TOD
handling block mentioned (the code around the other occurrence at 1208-1211) and
log a clear warning via logger::warn when rejecting a non-finite TOD value.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/SceneSettingsManager.h (1)
17-20: PR metadata can be tightened for maintainability and traceability.Consider a conventional title like
feat(weather-editor): add time-of-day scene settings, and add an issue keyword in the PR description (e.g.,Implements #<id>/Addresses #<id>).As per coding guidelines: "
**/*: provide suggestions for Conventional Commit Titles ... and Issue References ...".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneSettingsManager.h` around lines 17 - 20, Update the PR metadata to follow our Conventional Commits and issue-tracking guideline: change the PR/commit title to a conventional-style title such as "feat(weather-editor): add time-of-day scene settings" and add an issue reference line in the PR description like "Implements #<id>" (or "Addresses #<id>") so the change in SceneSettingsManager (file/class SceneSettingsManager.h) is traceable and matches the "`**/*`: provide suggestions for Conventional Commit Titles ... and Issue References ..." rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/SceneSettingsManager.h`:
- Around line 17-20: Update the PR metadata to follow our Conventional Commits
and issue-tracking guideline: change the PR/commit title to a conventional-style
title such as "feat(weather-editor): add time-of-day scene settings" and add an
issue reference line in the PR description like "Implements #<id>" (or
"Addresses #<id>") so the change in SceneSettingsManager (file/class
SceneSettingsManager.h) is traceable and matches the "`**/*`: provide
suggestions for Conventional Commit Titles ... and Issue References ..." rule.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allows for feature specific time of day settings through the scene manager.
Summary by CodeRabbit
New Features
Behavior Changes
UI
Documentation
Quality