feat(ISL): add saving for light placer json settings#2024
Merged
alandtse merged 2 commits intoApr 2, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughApplied editor overrides earlier in runtime light processing so downstream inverse-square and disabled checks use updated fields; expanded Light Editor with Light Placer integration (parsing, save-to-JSON, restore/original state, UI control, and helper utilities). Changes
Sequence DiagramsequenceDiagram
participant Process as ProcessLight()
participant Editor as LightEditor
participant Runtime as RuntimeData
participant UI as EditorUI
participant FS as LightPlacer JSON
Process->>Editor: ApplyOverrides(niLight, runtimeData)
activate Editor
Editor->>Editor: Validate active selection & lpInfo
Editor->>Runtime: Copy overrides (color/fade/radius/size/flags)
Editor-->>Process: return (overrides applied)
deactivate Editor
Process->>Process: Check Disabled / isInverseSquare using updated runtimeData
Process->>Process: CalculateRadius and apply inverse-square logic
Note over UI,Editor: User triggers Save
UI->>Editor: SaveToLightPlacer()
activate Editor
Editor->>Editor: ParseLPLightName(), MatchesLPFilters(), UpdateLPFlags()
Editor->>FS: Load JSON
Editor->>FS: Modify model/light entry (color/fade/radius/offsets/flags)
Editor->>FS: Save JSON
Editor-->>UI: return (success/failure)
deactivate Editor
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. |
![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
🤖 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/Features/InverseSquareLighting/LightEditor.cpp`:
- Around line 535-536: The code reads JSON nodes directly (e.g., iterating
entry["models"] and using modelEntry.get<std::string>(), and reading
data["light"] / offset) which can throw on malformed types; before accessing
these values update LightEditor.cpp to validate node types and presence: check
entry.contains("models") && entry["models"].is_array() and for each modelEntry
assert modelEntry.is_string() before calling get<std::string>(), compare to
normalizedOwner only when valid, and when reading data (data["light"]) verify
the field exists and is_string() and that offset is an array of the expected
length with numeric elements (use is_number() or
is_number_float()/is_number_integer()) before converting; on any validation
failure skip the entry and log a warning instead of letting an exception
propagate.
- Around line 600-601: After a successful save, update the in-memory baseline
(original.data) with the fields you persisted so RestoreOriginal() uses the new
values; currently only original.pos is advanced and current.pos zeroed. In the
save-success path (the same block that does original.pos = original.pos +
current.pos; current.pos = {0,0,0};), copy the persisted fields from current
(e.g., color, fade, radius, cutoff, size and any other data members you write to
disk) into original.data so subsequent RestoreOriginal() calls restore the saved
runtime state instead of the old values.
- Around line 387-425: The ApplyOverrides/RestoreOriginal flow uses shared
mutable globals (activeNiLight, current, activeRefr, activeLigh, activeIsRef)
across render/UI threads causing unsafe cross-thread access and dangling
raw-pointer dereferences; change to a synchronized, owned snapshot: introduce a
thread-safe selection snapshot/queue (e.g., guarded by a mutex or lock-free
queue) that copies the necessary data (diffuse, fade, cutoffOverride, size,
original.data, original.pos, original.tesFlags) and holds owned handles
(RE::NiPointer for NiLight and ref-counted handles for TES objects) instead of
raw pointers; have ProcessLight/ApplyOverrides consume the snapshot atomically
(pop or swap) and operate only on the snapshot, and have RestoreOriginal pull
the same owned snapshot to restore state without touching globals or raw
pointers, clearing only the snapshot entry when done.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7493b1a0-414a-4808-afe8-3330f59f1357
📒 Files selected for processing (3)
src/Features/InverseSquareLighting.cppsrc/Features/InverseSquareLighting/LightEditor.cppsrc/Features/InverseSquareLighting/LightEditor.h
|
✅ A pre-release build is available for this PR: |
InTheBottle
force-pushed
the
feat/light-editor-lp-save
branch
from
7416e57 to
8abe6dd
Compare
InTheBottle
changed the title
Contributor
Author
|
Realized the light editor was never updated along with light placer, so it should be able to handle saving when multiples of the same form ID exist in one json along different whitelisted references. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Features/InverseSquareLighting/LightEditor.cpp (1)
672-673:⚠️ Potential issue | 🟠 MajorSuccessful save doesn't update
original.databaseline.After saving, only
original.posis advanced. WhenRestoreOriginal()runs later (on deselect or editor close), it will restoreoriginal.datawhich still contains pre-save values, undoing the visual effect of saved changes until the light reloads.🐛 Proposed fix
+ original.data = current.data; original.pos = original.pos + current.pos; current.pos = { 0, 0, 0 };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/InverseSquareLighting/LightEditor.cpp` around lines 672 - 673, The save code only advances original.pos and zeroes current.pos, leaving original.data unchanged so RestoreOriginal() later reverts the visible changes; after advancing original.pos you must also update the saved baseline by merging current edits into original.data (e.g. original.data = original.data + current.data or memcpy component-wise) and then clear current.data and current.pos (keep the existing current.pos = {0,0,0} semantics), so future RestoreOriginal() uses the updated baseline; apply this change in the same save routine where original.pos and current.pos are modified and keep RestoreOriginal() logic unchanged.
🧹 Nitpick comments (2)
src/Features/InverseSquareLighting/LightEditor.cpp (2)
321-321:activeNiLight.reset()called every frame instead of only on selection change.Line 321 runs unconditionally every frame the light is selected, while the
if (previous != selected)block above (lines 293-318) only runs on selection change. This seems intentional to ensure the pointer stays valid, but consider moving inside the condition block to avoid repeated ref-count churn, or add a comment explaining the design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/InverseSquareLighting/LightEditor.cpp` at line 321, activeNiLight.reset(niLight) is being called every frame even though the surrounding selection-change logic (the if (previous != selected) block around lines ~293-318) only runs when the selection actually changes; to fix, either move the activeNiLight.reset(niLight) call into that if (previous != selected) block (or another code path that only runs on selection change) to avoid unnecessary ref-count churn, or if the per-frame reset is intentional to maintain pointer validity, add a concise comment at the activeNiLight.reset(niLight) site explaining why it must run every frame (referring to activeNiLight and niLight) so future readers know it’s deliberate.
93-101: No user feedback on save failure.
SaveToLightPlacer()returns aboolindicating success/failure, but the result is discarded. Users won't know if the save failed (e.g., file not found, parse error, no matching entry). Consider showing a status message or log notification.♻️ Proposed enhancement
if (lpInfo.isLPLight) { ImGui::SameLine(); if (ImGui::Button("Save to Light Placer")) { - SaveToLightPlacer(); + if (!SaveToLightPlacer()) { + logger::warn("[LightEditor] Save to Light Placer failed - check log for details"); + // Optionally: set a transient UI message to display + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/InverseSquareLighting/LightEditor.cpp` around lines 93 - 101, Save the bool result of SaveToLightPlacer() (called when ImGui::Button("Save to Light Placer") is pressed) and surface failure to the user instead of discarding it: check the return value, and on failure set a visible status (e.g., a member string like lastSaveStatus or boolean like lastSaveFailed) and show it in the UI near the button (using ImGui::Text/ImGui::TextColored or an ImGui popup) and/or log the error; ensure the tooltip code path (Util::HoverTooltipWrapper()) still works and update the message to reflect success/failure so users know if SaveToLightPlacer() failed.
🤖 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/Features/InverseSquareLighting/LightEditor.cpp`:
- Around line 663-670: The code currently writes configArray to outFile but
never checks whether the write succeeded; after writing (outFile <<
configArray.dump(1, '\t');) flush the stream and verify its state (e.g.
outFile.flush(); if (outFile.fail() || outFile.bad()) {
logger::error("[LightEditor] Failed to write Light Placer config to {}: stream
error", filePath.string()); return false; }) so failures (disk full,
permissions) are detected and the function returns false instead of true;
reference the existing symbols outFile, configArray, filePath and
logger::warn/logger::error when adding the check.
In `@src/Features/InverseSquareLighting/LightEditor.h`:
- Around line 105-118: The cached raw pointers activeRefr and activeLigh can
become dangling; replace them with ref-counted/managed handles (e.g.,
RE::TESObjectREFRHandle or RE::NiPointer<RE::TESObjectREFR> / an appropriate RE
handle type for TESObjectLIGH) and update all uses (including RestoreOriginal()
and SaveToLightPlacer()) to dereference via the handle API and check validity
before accessing (handle.IsValid() / handle.get()). Also ensure any assignment
to these members uses the handle form (from the current RE pointer) so the
editor no longer holds raw, potentially-freed pointers.
---
Duplicate comments:
In `@src/Features/InverseSquareLighting/LightEditor.cpp`:
- Around line 672-673: The save code only advances original.pos and zeroes
current.pos, leaving original.data unchanged so RestoreOriginal() later reverts
the visible changes; after advancing original.pos you must also update the saved
baseline by merging current edits into original.data (e.g. original.data =
original.data + current.data or memcpy component-wise) and then clear
current.data and current.pos (keep the existing current.pos = {0,0,0}
semantics), so future RestoreOriginal() uses the updated baseline; apply this
change in the same save routine where original.pos and current.pos are modified
and keep RestoreOriginal() logic unchanged.
---
Nitpick comments:
In `@src/Features/InverseSquareLighting/LightEditor.cpp`:
- Line 321: activeNiLight.reset(niLight) is being called every frame even though
the surrounding selection-change logic (the if (previous != selected) block
around lines ~293-318) only runs when the selection actually changes; to fix,
either move the activeNiLight.reset(niLight) call into that if (previous !=
selected) block (or another code path that only runs on selection change) to
avoid unnecessary ref-count churn, or if the per-frame reset is intentional to
maintain pointer validity, add a concise comment at the
activeNiLight.reset(niLight) site explaining why it must run every frame
(referring to activeNiLight and niLight) so future readers know it’s deliberate.
- Around line 93-101: Save the bool result of SaveToLightPlacer() (called when
ImGui::Button("Save to Light Placer") is pressed) and surface failure to the
user instead of discarding it: check the return value, and on failure set a
visible status (e.g., a member string like lastSaveStatus or boolean like
lastSaveFailed) and show it in the UI near the button (using
ImGui::Text/ImGui::TextColored or an ImGui popup) and/or log the error; ensure
the tooltip code path (Util::HoverTooltipWrapper()) still works and update the
message to reflect success/failure so users know if SaveToLightPlacer() failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3be15e15-b72d-427a-b981-1118037146dc
📒 Files selected for processing (3)
src/Features/InverseSquareLighting.cppsrc/Features/InverseSquareLighting/LightEditor.cppsrc/Features/InverseSquareLighting/LightEditor.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/InverseSquareLighting.cpp
InTheBottle
force-pushed
the
feat/light-editor-lp-save
branch
from
8abe6dd to
9bc5778
Compare
InTheBottle
changed the title
Collaborator
|
@coderabbitai review for DRYness particularly given json handling |
alandtse
changed the title
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 15216
DRY Issues1. Duplicate
|
| Issue | Location | Severity |
|---|---|---|
Duplicate diffuse/fade writes |
UpdateSelectedLight + ApplyOverrides |
Medium |
| Duplicate JSON array string-match loop | SaveToLightPlacer |
Medium |
| Duplicate whitelist/blacklist iteration | MatchesLPFilters |
Medium |
Repeated contains + is_array guard |
Both JSON functions | Low |
Inline existingOffset validation |
SaveToLightPlacer |
Low |
🧠 Learnings used
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1271-1274
Timestamp: 2026-03-22T18:40:49.633Z
Learning: Repo: doodlum/skyrim-community-shaders — Maintainer preference (PR `#2000` on 2026-03-22): In src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI, keep `resetLayout = false;` at the end of the frame (after PaletteWindow::Draw). The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows.
Learnt from: SkrubbySkrubInAShrub
Repo: doodlum/skyrim-community-shaders PR: 1997
File: src/Features/InverseSquareLighting/LightEditor.cpp:91-96
Timestamp: 2026-03-21T16:50:35.510Z
Learning: In `src/Features/InverseSquareLighting/LightEditor.cpp` (doodlum/skyrim-community-shaders), spotlight lights are intentionally classified as `isOther` (`current.isOther = ... || current.isSpotlight`). The `!selected.isOther` guard in `DrawSettings()` hides TES light flag checkboxes and position offset for spotlights. Color, intensity, radius, and the Linear Light checkbox remain editable for spotlights by design — this is the intended "view but can't edit critical flags" behavior for spotlight entries in the ISL editor.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1239-1244
Timestamp: 2026-03-22T18:32:22.461Z
Learning: In doodlum/skyrim-community-shaders (PR `#2000`, 2026-03-22), maintainer preference: keep the default browser width logic in src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI — browserWidth = min(availableWidth * 0.5f, 960.0f * Util::GetUIScale()) is applied unconditionally (before checking settings.showViewport) to preserve a consistent left-rail width on first-use/reset. Do not change this to fill full width when the viewport is hidden.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2033
File: src/Menu/ThemeManager.cpp:181-186
Timestamp: 2026-03-30T13:06:55.665Z
Learning: In doodlum/skyrim-community-shaders (PR `#2033`), `ThemeManager::ForceApplyDefaultTheme` loads the actual `Default.json` preset file (via `ThemePresets`) which already contains all correct color values. The simple-palette override block used in `SetupImGuiStyle` is intentionally absent from `ForceApplyDefaultTheme` — it is not needed in the emergency recovery path because the preset file has the full palette. `CreateDefaultThemeFiles` emits a separate minimal fallback only when no theme files exist at all, which is a distinct scenario unrelated to the emergency recovery path.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds persistence support to the Inverse Square Lighting (ISL) in-game Light Editor so that edits to Light Placer-attached lights can be written back into the original Light Placer JSON config, while also improving selection lifecycle and preview synchronization.
Changes:
- Add Light Placer light metadata tracking (config path, EDID, owner model/form info) and a “Save to Light Placer” UI action.
- Implement JSON read/modify/write workflow for updating Light Placer entries (including offset + flags + filter matching).
- Introduce editor override application + original-state restoration to keep previews consistent when selection/input state changes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Features/InverseSquareLighting/LightEditor.h |
Adds new editor APIs/state for overrides, restoration, LP parsing, filter matching, and saving. |
src/Features/InverseSquareLighting/LightEditor.cpp |
Implements LP name parsing, filter matching (incl. FormID resolving), JSON persistence, and improved selection/original restoration flow. |
src/Features/InverseSquareLighting.cpp |
Calls the editor override hook during light processing to keep runtime data aligned with editor values. |
Collaborator
|
@copilot review for correctness and for DRY ness. |
Allows direct saving to the original JSON from edits made in the CS UI.
InTheBottle
force-pushed
the
feat/light-editor-lp-save
branch
from
9bc5778 to
a8a2ccc
Compare
Dlizzio
approved these changes
Apr 1, 2026
alandtse
approved these changes
Apr 2, 2026
ParticleTroned
pushed a commit
to ParticleTroned/skyrim-community-shaders
that referenced
this pull request
May 2, 2026
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
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 direct saving to the original JSON from edits made in the CS UI.
Summary by CodeRabbit
New Features
Improvements