refactor(weather-editor): migrate isl light editor#2414
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLightEditor ownership moves from InverseSquareLighting to EditorWindow. EditorWindow now stores and renders the Lighting editor, calls its GatherLights/ResetOverrides lifecycle methods, and InverseSquareLighting queries EditorWindow::GetSingleton()->lightEditor for overrides and disable flags. LightEditor drops its internal enabled flag and adds ResetOverrides(). ChangesLightEditor Ownership Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.11][ERROR]: Error: exception Unix_error: No such file or directory stat src/WeatherEditor/LightEditor.h 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/WeatherEditor/EditorWindow.h (1)
67-68: PR metadata: tighten commit title and add issue linkageSuggested title:
refactor(weather-editor): migrate isl light editor
If this maps to a tracker item, addAddresses #<id>(orImplements #<id>).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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/WeatherEditor/EditorWindow.h` around lines 67 - 68, Update the PR commit title and description to follow Conventional Commits by using the suggested title format (e.g., "refactor(weather-editor): migrate isl light editor") and, if this change maps to a tracker item, append an issue reference like "Addresses #<id>" or "Implements #<id>" in the PR description; mention the relevant symbol (LightEditor in EditorWindow.h) in the PR body to provide context for reviewers.
🤖 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 `@src/Features/InverseSquareLighting.cpp`:
- Around line 49-57: The code checks a non-existent member editorRef.enabled
causing a compile error; edit the conditional that sets
LightLimitFix::LightFlags::Disabled (the if using bsLight->pointLight, isInvSq,
and editorRef) to remove the stale "editorRef.enabled &&" clause so the decision
only depends on bsLight->pointLight and the editor flags
editorRef.disableInvSqLights / editorRef.disableRegularLights (keep the existing
isInvSq logic and the call to light.lightFlags.set(...)).
In `@src/WeatherEditor/EditorWindow.cpp`:
- Around line 1427-1429: EditorWindow::Draw() currently calls
lightEditor.GatherLights() only while open, so when open flips false the
LightEditor cleanup isn't run; update UpdateOpenState() to detect the open ->
closed transition and invoke a dedicated cleanup/restore method on lightEditor
(e.g., LightEditor::RestoreState() or LightEditor::ResetOverrides()) to perform
the same cleanup GatherLights() would do at close; if that method doesn't exist,
add it to LightEditor and have it clear selected overrides/restore state, then
call it from UpdateOpenState() when transitioning to closed.
---
Nitpick comments:
In `@src/WeatherEditor/EditorWindow.h`:
- Around line 67-68: Update the PR commit title and description to follow
Conventional Commits by using the suggested title format (e.g.,
"refactor(weather-editor): migrate isl light editor") and, if this change maps
to a tracker item, append an issue reference like "Addresses #<id>" or
"Implements #<id>" in the PR description; mention the relevant symbol
(LightEditor in EditorWindow.h) in the PR body to provide context for reviewers.
🪄 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: e3767410-af44-4594-ab2f-06882a37adac
📒 Files selected for processing (6)
src/Features/InverseSquareLighting.cppsrc/Features/InverseSquareLighting.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/LightEditor.cppsrc/WeatherEditor/LightEditor.h
|
✅ A pre-release build is available for this PR: |
…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>
bf43ca6 to
30d6c3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/Features/InverseSquareLighting.cpp`:
- Around line 49-57: The disableInvSqLights and disableRegularLights members of
LightEditor are read uninitialized by InverseSquareLighting::ProcessLight via
EditorWindow::GetSingleton()->lightEditor, causing indeterminate behavior;
initialize these flags to sensible defaults (e.g., false) by adding default
member initializers in the LightEditor declaration or setting them in
LightEditor’s constructor so EditorWindow::GetSingleton()->lightEditor has
well-defined values before any ImGui interaction.
🪄 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: e0b1a506-2901-4252-95b4-4db1aee872df
📒 Files selected for processing (6)
src/Features/InverseSquareLighting.cppsrc/Features/InverseSquareLighting.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/LightEditor.cppsrc/WeatherEditor/LightEditor.h
First step of lighting editor rework.
the current design is aimed at letting ISL be absorbed into LLF, while making the weather editor the central place to edit game visuals.
Summary by CodeRabbit
New Features
Improvements