feat(weather-editor): full screen background blur#2009
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Weather Editor open/close gating and lifecycle handling, adds game-menu visibility control, introduces a weather-editor fullscreen blur mode (toggled via BackgroundBlur API), and updates the blur shader to skip SDF rounded-rect masking when fullscreen blur mode is active. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Menu as Menu / Hotkey
participant Editor as EditorWindow
participant UI as UI System
participant Blur as BackgroundBlur
participant Shader as BackgroundBlurComposite.hlsl
User->>Menu: Press editor hotkey
Menu->>Editor: Call CanBeOpen()
Editor->>Editor: Validate player & cell + menus
alt CanBeOpen == true
Menu->>Editor: Set open = true
Editor->>Editor: UpdateOpenState()
Editor->>UI: HideGameMenus()
Editor->>Blur: SetWeatherEditorActive(true)
Blur->>Shader: Set WindowParams.w = 1.0
Shader->>Shader: Skip SDF masking
else CanBeOpen == false
Menu-->>User: Ignore open request
end
User->>Editor: Close editor
Editor->>Editor: Set open = false
Editor->>Editor: UpdateOpenState()
Editor->>UI: ShowGameMenus()
Editor->>Blur: SetWeatherEditorActive(false)
Blur->>Shader: Set WindowParams.w = 0.0
Shader->>Shader: Resume SDF masking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/WeatherEditor/EditorWindow.h (1)
107-114: Consider hidingopenbehind this lifecycle API.Now that open/close side effects live in
UpdateOpenState(), every caller that writesopenhas to remember to schedule that sync separately. A smallSetOpen(bool)/ToggleOpen()entry point would keep the blur/menu/camera bookkeeping local and make this much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/EditorWindow.h` around lines 107 - 114, Add a small lifecycle API that encapsulates the open state: introduce SetOpen(bool) and ToggleOpen() on the EditorWindow and make the raw `open` flag private (or stop writing it directly). Implement SetOpen to update the internal `open` flag and perform the existing side-effects now in UpdateOpenState (call DisableVanityCamera/RestoreVanityCamera/HideGameMenus/ShowGameMenus or schedule the same sync), and have ToggleOpen call SetOpen(!open). Update callers to use SetOpen/ToggleOpen instead of writing `open` directly so all blur/menu/camera bookkeeping remains local to EditorWindow and UpdateOpenState no longer requires external syncs.
🤖 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/Menu/OverlayRenderer.cpp`:
- Around line 56-58: Move the editor lifecycle sync so it runs before the
early-return gate in ShouldSkipRendering(): ensure you call the block that
checks and sets editorWindow->open (including the EditorWindow::CanBeOpen()
check) and then editorWindow->UpdateOpenState() prior to invoking
ShouldSkipRendering() or returning early; this guarantees that any changes from
ProcessInputEventQueue() or the previous frame's close button are applied and
the close transition runs (preventing the D3D12 path from leaving
menu/hiding/camera/blur/session state latched).
---
Nitpick comments:
In `@src/WeatherEditor/EditorWindow.h`:
- Around line 107-114: Add a small lifecycle API that encapsulates the open
state: introduce SetOpen(bool) and ToggleOpen() on the EditorWindow and make the
raw `open` flag private (or stop writing it directly). Implement SetOpen to
update the internal `open` flag and perform the existing side-effects now in
UpdateOpenState (call
DisableVanityCamera/RestoreVanityCamera/HideGameMenus/ShowGameMenus or schedule
the same sync), and have ToggleOpen call SetOpen(!open). Update callers to use
SetOpen/ToggleOpen instead of writing `open` directly so all blur/menu/camera
bookkeeping remains local to EditorWindow and UpdateOpenState no longer requires
external syncs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1f230f5-c24d-4676-8e84-5e57b6261875
📒 Files selected for processing (8)
package/Shaders/Menu/BackgroundBlurComposite.hlslsrc/Features/WeatherEditor.cppsrc/Menu.cppsrc/Menu/BackgroundBlur.cppsrc/Menu/BackgroundBlur.hsrc/Menu/OverlayRenderer.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/EditorWindow.h
|
✅ A pre-release build is available for this PR: |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Just the one comment; otherwise good.
|
|
||
| bool EditorWindow::CanBeOpen() | ||
| { | ||
| auto player = RE::PlayerCharacter::GetSingleton(); |
There was a problem hiding this comment.
Do we not have this as a global?
There was a problem hiding this comment.
it's not a global yet
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/WeatherEditor/EditorWindow.cpp (1)
478-479: Consider usingglobals::game::playerfor consistency.These lines still use
RE::PlayerCharacter::GetSingleton()directly, while the newCanBeOpen()at line 1926 usesglobals::game::player. For consistency across the file, consider replacing these direct singleton calls with the cached global.♻️ Example change for line 478
-auto player = RE::PlayerCharacter::GetSingleton(); +auto player = globals::game::player;Also applies to: 563-564, 963-964
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/EditorWindow.cpp` around lines 478 - 479, Replace direct singleton calls to RE::PlayerCharacter::GetSingleton() with the cached global globals::game::player for consistency with CanBeOpen(); specifically, where code checks "auto player = RE::PlayerCharacter::GetSingleton();" and then uses player->parentCell, change to use "auto player = globals::game::player" (or null-check globals::game::player) and keep the existing parentCell check and logic intact; apply the same replacement pattern in the other occurrences mentioned so all player access in this file uses globals::game::player.
🤖 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/WeatherEditor/EditorWindow.cpp`:
- Around line 478-479: Replace direct singleton calls to
RE::PlayerCharacter::GetSingleton() with the cached global globals::game::player
for consistency with CanBeOpen(); specifically, where code checks "auto player =
RE::PlayerCharacter::GetSingleton();" and then uses player->parentCell, change
to use "auto player = globals::game::player" (or null-check
globals::game::player) and keep the existing parentCell check and logic intact;
apply the same replacement pattern in the other occurrences mentioned so all
player access in this file uses globals::game::player.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ccaf0c99-0b85-4fb8-ac3e-d8303463e5da
📒 Files selected for processing (3)
src/Globals.cppsrc/Globals.hsrc/WeatherEditor/EditorWindow.cpp
|
Oh wait. You're doing at data loaded vs load game. Is it available then? You'd know because it'd break immediately. Just remembered the issue fixed with UW. |
Current implementation works the same as before, checked UW and saw nothing for a singleton timing fix. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Globals.h (1)
224-224: Add issue linkage in the PR description (if tracked).Since this is a feature PR, consider adding
Implements #...orAddresses #...for traceability.As per coding guidelines: "Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Globals.h` at line 224, The PR lacks an issue reference for traceability; update the PR description to include a GitHub keyword linking to the tracked issue (e.g., "Implements `#123`", "Addresses `#456`" or similar) and ensure it mentions the feature tied to the Globals symbol RE::PlayerCharacter* player so reviewers can map the change back to the issue; add the appropriate keyword line to the PR description and, if applicable, note any related commit hashes or milestone.
🤖 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/Globals.h`:
- Line 224: The PR lacks an issue reference for traceability; update the PR
description to include a GitHub keyword linking to the tracked issue (e.g.,
"Implements `#123`", "Addresses `#456`" or similar) and ensure it mentions the
feature tied to the Globals symbol RE::PlayerCharacter* player so reviewers can
map the change back to the issue; add the appropriate keyword line to the PR
description and, if applicable, note any related commit hashes or milestone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f575f7f3-b6ee-4c65-a8c0-f217a75c57b5
📒 Files selected for processing (2)
src/Globals.cppsrc/Globals.h
2 participants
Adds fullscreen background blur to the weather editor when background blur is enabled. Works with framegen disabled and upscaling disabled.
Summary by CodeRabbit
New Features
Bug Fixes