feat(cs-editor): rework lighting editor#2458
Conversation
…d includes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… window Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erEditor Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d includes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… window Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erEditor Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/SKSE/Plugins/CommunityShaders/Translations/zh_CN.json (1)
814-814:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
feature.light_editor.cellformat placeholders are out of sync with runtime usage.On Line 814, the string is
"单元格:%s"buten.jsonnow defines this key as"Cell: 0x%08X | %s". This key is used in formatted UI output; keep placeholder count/order aligned to avoid bad formatting behavior.Proposed patch
- "feature.light_editor.cell": "单元格:%s", + "feature.light_editor.cell": "单元格:0x%08X | %s",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/SKSE/Plugins/CommunityShaders/Translations/zh_CN.json` at line 814, The translation for the key feature.light_editor.cell uses a single %s placeholder but must match the runtime format used in en.json ("Cell: 0x%08X | %s"); update the zh_CN value to include both format specifiers in the same order and form (e.g., "单元格:0x%08X | %s") so the numeric hex and the string are passed into the UI without formatting errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package/SKSE/Plugins/CommunityShaders/Translations/en.json`:
- Line 815: Replace the mojibake sequence "â��" with a proper em dash "—" (or a
hyphen if preferred) in the Light Editor error strings; specifically update the
value for "feature.light_editor.add_light_failed" (and the other Light Editor
error message keys in the same namespace) to use "—" instead of the encoding
artifact so the UI renders correctly. Ensure you only change the string literal
value in en.json and keep surrounding punctuation and spacing consistent.
In `@package/SKSE/Plugins/CommunityShaders/Translations/zh_CN.json`:
- Around line 2099-2115: This PR adds deprecated translation keys under
feature.light_editor.* (e.g., feature.light_editor.dynamic, filter_by, flicker,
flicker_slow, hemi_shadow, inverse_square_light, linear_light, negative,
omni_shadow, portal_strict, position_offset, pulse, pulse_slow, revert_changes,
save_to_light_placer_tooltip, sort_by, spotlight_not_applicable) that do not
exist in en.json and break i18n validation; remove these stale keys from
zh_CN.json (or rename each to the corresponding current key names used in
en.json if there are direct replacements) so the file matches en.json and the CI
i18n job passes.
In `@src/CSEditor/EditorWindow.cpp`:
- Line 22: The Settings struct field paletteValues is omitted from the JSON
(de)serialization macro; update the
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT invocation for
EditorWindow::Settings to include paletteValues so the field is persisted across
save/load cycles, ensuring the identifier exactly matches the field name in
EditorWindow::Settings.
In `@src/CSEditor/LightEditor.cpp`:
- Around line 83-103: The current regex-based rounding block (static const
std::regex floatPattern ... using output, rounded, and iterating
sregex_iterator) mutates any float-like substrings in the final serialized text
(including inside JSON string values); instead, parse the JSON text into a
structured object, recursively walk the object before serialization and round
numeric JSON values to the desired precision (e.g. multiply/round/divide or use
std::round) then reserialize; replace the regex block with code that
deserializes output into your JSON type, visits all numeric nodes to round them,
and then serializes back into output so only actual numeric values are changed
(use the project’s JSON library/type and helper functions to find/modify
numbers).
- Around line 2164-2178: The code copies the old light info into newData
("newData[\"light\"] = data[\"light\"]"), so edits to the Bulb type only affect
the preview and aren't persisted; update newData["light"] to reflect the edited
value from the live/current state before writing (e.g., assign newData["light"]
= current.data.light or explicitly copy the modified bulb field into
newData["light"][...]) so that the changed Bulb type in current is stored in
newData and will survive reloadlp; adjust the block around newData, data, and
current (symbols: newData, data, current, "light", "fade") accordingly.
- Around line 38-50: The ScheduleConsoleCommand function uses a static
REL::Relocation selectedRef that only encodes SSE/AE variants and will be wrong
on VR; replace that direct REL::Relocation usage with the repo’s multi-runtime
relocation pattern (e.g., call REL::RelocateMember or the runtime-detection
helper used elsewhere) so the address is chosen via runtime detection across
SE/AE/VR, update the selectedRef definition to use REL::RelocateMember or the
equivalent multi-runtime relocation wrapper and keep the same type
RE::ObjectRefHandle* and usage inside ScheduleConsoleCommand.
In `@src/CSEditor/LightEditor.h`:
- Around line 201-209: AttachedBulb currently records
lightEDID/configPath/refrId/index but lacks the specific points[]/nodes[]
locator for bulbs emitted by a single LP entry, causing the save path to pick
the wrong point; update the AttachedBulb struct (used in attachedBulbs and
referenced by addSelectedBulb) to carry a stable identifier for the originating
point/node (for example add a uint32_t pointIndex or a std::string jsonLocator)
and ensure all places that construct AttachedBulb (popup open / GatherLights
code paths) populate this new field and the save/load/serialize logic uses that
field to round-trip edits to the correct bulb.
In `@src/CSEditor/LightPicker.cpp`:
- Around line 169-181: BeginPick()/Cancel() currently leave hoverMesh and the
cached mouse hit state (lastMouseX/lastMouseY or whatever lastMouse* variables)
intact, allowing stale hits to persist; when entering pick mode
(LightPicker::BeginPick), canceling it (LightPicker::Cancel) and whenever pick
mode or pick type changes (where mode is switched), reset hoverMesh and the
lastMouse* cached coordinates so Update() will recompute the hit immediately
even if the cursor hasn't moved; locate and clear hoverMesh and the lastMouse*
variables in BeginPick, in Cancel, and in the code path that flips modes (the
mode-change handler) to prevent stale hover state.
---
Outside diff comments:
In `@package/SKSE/Plugins/CommunityShaders/Translations/zh_CN.json`:
- Line 814: The translation for the key feature.light_editor.cell uses a single
%s placeholder but must match the runtime format used in en.json ("Cell: 0x%08X
| %s"); update the zh_CN value to include both format specifiers in the same
order and form (e.g., "单元格:0x%08X | %s") so the numeric hex and the string are
passed into the UI without formatting errors.
🪄 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 Plus
Run ID: 95a794e0-d6e4-46a1-846d-78f90866365b
📒 Files selected for processing (8)
package/SKSE/Plugins/CommunityShaders/Translations/en.jsonpackage/SKSE/Plugins/CommunityShaders/Translations/zh_CN.jsonsrc/CSEditor/EditorWindow.cppsrc/CSEditor/EditorWindow.hsrc/CSEditor/LightEditor.cppsrc/CSEditor/LightEditor.hsrc/CSEditor/LightPicker.cppsrc/CSEditor/LightPicker.h
|
✅ A pre-release build is available for this PR: |
|
Will make some changes based on feedback before merge. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/CSEditor/LightEditor.h (1)
245-251:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCarry the matched point/node index in
AttachedBulb.This struct can identify the runtime bulb, but not the specific
points[]/nodes[]element that spawned it when one LP entry emits multiple bulbs with the samelightEDID. Theindexfield tracks the runtime GatherLights ordering, not the JSON array position. The save path then has to guess and currently falls back to the first point, causing edits to round-trip to the wrong bulb.🔧 Suggested fix
Add a stable JSON locator to the struct:
struct AttachedBulb { std::string lightEDID; std::string configPath; RE::FormID refrId = 0; // owner reference (the picked mesh ref) uint32_t index = 0; // running per-ref index, mirrors GatherLights ordering + uint32_t pointIndex = 0; // index into the LP entry's points[] or nodes[] array };Then populate
pointIndexin the code paths that constructAttachedBulb(GatherLights, popup open) and use it in the save/serialize logic to target the correct point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CSEditor/LightEditor.h` around lines 245 - 251, AttachedBulb cannot identify which specific points[]/nodes[] JSON element spawned a runtime bulb because its index field reflects GatherLights ordering; add a new uint32_t pointIndex (or similar) to struct AttachedBulb to hold the stable JSON array position, populate pointIndex wherever AttachedBulb instances are constructed (e.g., GatherLights and when opening the edit popup), and change the save/serialize logic to use pointIndex (fall back to existing index only if pointIndex is unset) so edits target the correct JSON point/node rather than the first matching entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/CSEditor/LightEditor.h`:
- Around line 245-251: AttachedBulb cannot identify which specific
points[]/nodes[] JSON element spawned a runtime bulb because its index field
reflects GatherLights ordering; add a new uint32_t pointIndex (or similar) to
struct AttachedBulb to hold the stable JSON array position, populate pointIndex
wherever AttachedBulb instances are constructed (e.g., GatherLights and when
opening the edit popup), and change the save/serialize logic to use pointIndex
(fall back to existing index only if pointIndex is unset) so edits target the
correct JSON point/node rather than the first matching entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f3595be6-eb33-4cd1-a367-4ba4e4b883d7
📒 Files selected for processing (6)
package/SKSE/Plugins/CommunityShaders/Translations/en.jsonpackage/SKSE/Plugins/CommunityShaders/Translations/zh_CN.jsonsrc/CSEditor/LightEditor.cppsrc/CSEditor/LightEditor.hsrc/CSEditor/LightPicker.cppsrc/CSEditor/LightPicker.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/CSEditor/LightPicker.h
- package/SKSE/Plugins/CommunityShaders/Translations/en.json
- src/CSEditor/LightPicker.cpp
- package/SKSE/Plugins/CommunityShaders/Translations/zh_CN.json
This is a major rework of the Light editor.
It includes a reworked UI, picking meshes in worldspace as well as reading and writing LP jsons.
Summary by CodeRabbit
New Features
Bug Fixes