Removed redundant Settings nodes#1403
Conversation
|
Warning Rate limit exceeded@Ungeziefi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughFlattened several ImGui Settings trees (DynamicCubemaps, VolumetricLighting). Dynamic cubemap export flow hardened with explicit filename/path assembly, existence checks, try/catch logging, and deterministic resource cleanup. PerformanceOverlay now displays a historical average FPS alongside smoothed FPS. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ImGui UI
participant DCC as DynamicCubemap Creator
participant FS as Filesystem
participant DX as DirectX
UI->>DCC: Click Export
DCC->>DCC: Build filename (RxxxGxxxBxxxAxxx.dds)
DCC->>FS: Ensure directory exists
DCC->>FS: Check file existence
alt File exists
DCC->>UI: Log "skip (exists)"
else
DCC->>DX: Create temp texture & capture to ScratchImage
DX-->>DCC: Image data
DCC->>FS: Save DDS
DCC->>UI: Log "written"
end
DCC->>DX: Release image/texture
note over DCC: try/catch logs failures
sequenceDiagram
participant PO as PerformanceOverlay::DrawFPS
participant Hist as FrameTimeHistory
participant UI as ImGui
PO->>Hist: GetData()
Hist-->>PO: frame times
PO->>PO: Compute avgFrameTime, avgFps
PO->>UI: Print "FPS: smoothFps (ms) | Avg: avgFps"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Features/PerformanceOverlay.cpp (1)
395-401: Avg FPS calculation works but should exclude zero/uninitialized samples to avoid misleading low averagesThe history buffer is pre-sized; early frames will include zeros. Averaging zeros yields "Avg: 0.0" or an artificially low Avg FPS until the buffer is warmed up. Excluding non-positive samples makes the metric more representative immediately.
Apply this diff to compute the average over valid samples only:
- float avgFrameTime = std::accumulate(this->state.frameTimeHistory.GetData().begin(), - this->state.frameTimeHistory.GetData().end(), 0.0f) / - this->state.frameTimeHistory.GetData().size(); - float avgFps = avgFrameTime > 0.0f ? 1000.0f / avgFrameTime : 0.0f; + const auto& data = this->state.frameTimeHistory.GetData(); + size_t validCount = std::count_if(data.begin(), data.end(), [](float ft) { return ft > 0.0f; }); + float sum = 0.0f; + for (float ft : data) { + if (ft > 0.0f) sum += ft; + } + float avgFrameTime = validCount > 0 ? (sum / static_cast<float>(validCount)) : 0.0f; + float avgFps = (avgFrameTime > 0.0f) ? (1000.0f / avgFrameTime) : 0.0f;src/Features/DynamicCubemaps.cpp (1)
64-88: Zero-initialize D3D11_SUBRESOURCE_DATA to avoid undefined fieldsWhile you set the essential fields, value-initializing the array prevents garbage in unused members and is cheap.
Apply this small change:
- D3D11_SUBRESOURCE_DATA subresourceData[6]; + D3D11_SUBRESOURCE_DATA subresourceData[6] = {};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Features/DynamicCubemaps.cpp(1 hunks)src/Features/PerformanceOverlay.cpp(1 hunks)src/Features/VolumetricLighting.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (3)
src/Features/VolumetricLighting.cpp (1)
24-35: Good UI flattening; toggles still call SetupVL() immediatelyRemoving the outer Settings wrapper and exposing the two toggles directly aligns with the PR objective and preserves behavior.
src/Features/DynamicCubemaps.cpp (2)
28-41: SSR section flattening looks correctPromoting “Screen Space Reflections” to a top-level TreeNode with its own Tooltip/VR note maintains behavior and meets the PR goal.
122-128: VR advanced settings moved to a top-level section—looks goodThe VR settings are now self-contained under their own node with both visible and hidden settings, matching the flattened UI intent.
| ID3D11Texture2D* tempTexture; | ||
| DirectX::ScratchImage image; | ||
|
|
||
| if (std::filesystem::create_directories(defaultDynamicCubeMapSavePath)) { | ||
| logger::info("Missing DynamicCubeMap Creator directory created: {}", defaultDynamicCubeMapSavePath); | ||
| } | ||
| try { | ||
| DX::ThrowIfFailed(device->CreateTexture2D(&texDesc, subresourceData, &tempTexture)); | ||
| DX::ThrowIfFailed(CaptureTexture(device, context, tempTexture, image)); | ||
|
|
||
| std::filesystem::path DynamicCubeMapSavePath = defaultDynamicCubeMapSavePath; | ||
| std::filesystem::path filename(std::format("R{:03d}G{:03d}B{:03d}A{:03d}.dds", colorPixel.r, colorPixel.g, colorPixel.b, colorPixel.a)); | ||
| DynamicCubeMapSavePath /= filename; | ||
| if (std::filesystem::create_directories(defaultDynamicCubeMapSavePath)) { | ||
| logger::info("Missing DynamicCubeMap Creator directory created: {}", defaultDynamicCubeMapSavePath); | ||
| } | ||
|
|
||
| if (std::filesystem::exists(DynamicCubeMapSavePath)) { | ||
| logger::info("DynamicCubeMap Creator file for {} already exists, skipping.", filename.string()); | ||
| } else { | ||
| DX::ThrowIfFailed(SaveToDDSFile(image.GetImages(), image.GetImageCount(), image.GetMetadata(), DirectX::DDS_FLAGS::DDS_FLAGS_NONE, DynamicCubeMapSavePath.c_str())); | ||
| logger::info("DynamicCubeMap Creator file for {} written", filename.string()); | ||
| } | ||
| std::filesystem::path DynamicCubeMapSavePath = defaultDynamicCubeMapSavePath; | ||
| std::filesystem::path filename(std::format("R{:03d}G{:03d}B{:03d}A{:03d}.dds", colorPixel.r, colorPixel.g, colorPixel.b, colorPixel.a)); | ||
| DynamicCubeMapSavePath /= filename; | ||
|
|
||
| } catch (const std::exception& e) { | ||
| logger::error("Failed in DynamicCubeMap Creator file: {} {}", defaultDynamicCubeMapSavePath, e.what()); | ||
| if (std::filesystem::exists(DynamicCubeMapSavePath)) { | ||
| logger::info("DynamicCubeMap Creator file for {} already exists, skipping.", filename.string()); | ||
| } else { | ||
| DX::ThrowIfFailed(SaveToDDSFile(image.GetImages(), image.GetImageCount(), image.GetMetadata(), DirectX::DDS_FLAGS::DDS_FLAGS_NONE, DynamicCubeMapSavePath.c_str())); | ||
| logger::info("DynamicCubeMap Creator file for {} written", filename.string()); | ||
| } | ||
|
|
||
| image.Release(); | ||
| tempTexture->Release(); | ||
| } catch (const std::exception& e) { | ||
| logger::error("Failed in DynamicCubeMap Creator file: {} {}", defaultDynamicCubeMapSavePath, e.what()); | ||
| } | ||
|
|
||
| image.Release(); | ||
| tempTexture->Release(); | ||
| } |
There was a problem hiding this comment.
Potential null/garbage dereference on cleanup when exceptions occur
- tempTexture is uninitialized and released unconditionally after the try/catch. If CreateTexture2D throws before assignment, tempTexture is indeterminate and dereferencing it is UB.
- Prefer RAII (ComPtr) or at least initialize to nullptr and null-check before Release().
Minimal safe fix:
- ID3D11Texture2D* tempTexture;
+ ID3D11Texture2D* tempTexture = nullptr;
DirectX::ScratchImage image;
@@
- image.Release();
- tempTexture->Release();
+ image.Release();
+ if (tempTexture) {
+ tempTexture->Release();
+ tempTexture = nullptr;
+ }Optional: Switch to Microsoft::WRL::ComPtr to make cleanup exception-safe automatically.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ID3D11Texture2D* tempTexture; | |
| DirectX::ScratchImage image; | |
| if (std::filesystem::create_directories(defaultDynamicCubeMapSavePath)) { | |
| logger::info("Missing DynamicCubeMap Creator directory created: {}", defaultDynamicCubeMapSavePath); | |
| } | |
| try { | |
| DX::ThrowIfFailed(device->CreateTexture2D(&texDesc, subresourceData, &tempTexture)); | |
| DX::ThrowIfFailed(CaptureTexture(device, context, tempTexture, image)); | |
| std::filesystem::path DynamicCubeMapSavePath = defaultDynamicCubeMapSavePath; | |
| std::filesystem::path filename(std::format("R{:03d}G{:03d}B{:03d}A{:03d}.dds", colorPixel.r, colorPixel.g, colorPixel.b, colorPixel.a)); | |
| DynamicCubeMapSavePath /= filename; | |
| if (std::filesystem::create_directories(defaultDynamicCubeMapSavePath)) { | |
| logger::info("Missing DynamicCubeMap Creator directory created: {}", defaultDynamicCubeMapSavePath); | |
| } | |
| if (std::filesystem::exists(DynamicCubeMapSavePath)) { | |
| logger::info("DynamicCubeMap Creator file for {} already exists, skipping.", filename.string()); | |
| } else { | |
| DX::ThrowIfFailed(SaveToDDSFile(image.GetImages(), image.GetImageCount(), image.GetMetadata(), DirectX::DDS_FLAGS::DDS_FLAGS_NONE, DynamicCubeMapSavePath.c_str())); | |
| logger::info("DynamicCubeMap Creator file for {} written", filename.string()); | |
| } | |
| std::filesystem::path DynamicCubeMapSavePath = defaultDynamicCubeMapSavePath; | |
| std::filesystem::path filename(std::format("R{:03d}G{:03d}B{:03d}A{:03d}.dds", colorPixel.r, colorPixel.g, colorPixel.b, colorPixel.a)); | |
| DynamicCubeMapSavePath /= filename; | |
| } catch (const std::exception& e) { | |
| logger::error("Failed in DynamicCubeMap Creator file: {} {}", defaultDynamicCubeMapSavePath, e.what()); | |
| if (std::filesystem::exists(DynamicCubeMapSavePath)) { | |
| logger::info("DynamicCubeMap Creator file for {} already exists, skipping.", filename.string()); | |
| } else { | |
| DX::ThrowIfFailed(SaveToDDSFile(image.GetImages(), image.GetImageCount(), image.GetMetadata(), DirectX::DDS_FLAGS::DDS_FLAGS_NONE, DynamicCubeMapSavePath.c_str())); | |
| logger::info("DynamicCubeMap Creator file for {} written", filename.string()); | |
| } | |
| image.Release(); | |
| tempTexture->Release(); | |
| } catch (const std::exception& e) { | |
| logger::error("Failed in DynamicCubeMap Creator file: {} {}", defaultDynamicCubeMapSavePath, e.what()); | |
| } | |
| image.Release(); | |
| tempTexture->Release(); | |
| } | |
| // Ensure tempTexture is always initialized | |
| ID3D11Texture2D* tempTexture = nullptr; | |
| DirectX::ScratchImage image; | |
| try { | |
| DX::ThrowIfFailed(device->CreateTexture2D(&texDesc, subresourceData, &tempTexture)); | |
| DX::ThrowIfFailed(CaptureTexture(device, context, tempTexture, image)); | |
| if (std::filesystem::create_directories(defaultDynamicCubeMapSavePath)) { | |
| logger::info("Missing DynamicCubeMap Creator directory created: {}", defaultDynamicCubeMapSavePath); | |
| } | |
| std::filesystem::path DynamicCubeMapSavePath = defaultDynamicCubeMapSavePath; | |
| std::filesystem::path filename(std::format( | |
| "R{:03d}G{:03d}B{:03d}A{:03d}.dds", | |
| colorPixel.r, colorPixel.g, colorPixel.b, colorPixel.a)); | |
| DynamicCubeMapSavePath /= filename; | |
| if (std::filesystem::exists(DynamicCubeMapSavePath)) { | |
| logger::info( | |
| "DynamicCubeMap Creator file for {} already exists, skipping.", | |
| filename.string()); | |
| } else { | |
| DX::ThrowIfFailed(SaveToDDSFile( | |
| image.GetImages(), image.GetImageCount(), | |
| image.GetMetadata(), DirectX::DDS_FLAGS::DDS_FLAGS_NONE, | |
| DynamicCubeMapSavePath.c_str())); | |
| logger::info( | |
| "DynamicCubeMap Creator file for {} written", | |
| filename.string()); | |
| } | |
| } catch (const std::exception& e) { | |
| logger::error( | |
| "Failed in DynamicCubeMap Creator file: {} {}", | |
| defaultDynamicCubeMapSavePath, e.what()); | |
| } | |
| image.Release(); | |
| if (tempTexture) { | |
| tempTexture->Release(); | |
| tempTexture = nullptr; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Features/DynamicCubemaps.cpp around lines 90-118, tempTexture is declared
uninitialized and unconditionally released after the try/catch which can cause
undefined behavior if CreateTexture2D throws; fix by making tempTexture
initialized to nullptr and only calling Release() when it's non-null, or better
replace the raw pointer with Microsoft::WRL::ComPtr<ID3D11Texture2D> (or a
similar RAII wrapper) so the texture is released automatically even if an
exception is thrown; ensure the image.Release() and texture cleanup happen
safely in all control paths (if keeping raw pointer, null-check before Release
and set to nullptr after release).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/ShaderCache.h (2)
263-263: API extension preserves source compatibilityAdding a_elapsedOnly with a default maintains backward compatibility and aligns with the new UI usage. No issues spotted.
Consider documenting that a_elapsedOnly is only meaningful when a_timeOnly is true, so callers don’t assume it affects the full summary mode.
399-399: Consistent two-flag signature for stats stringSignature matches CompilationSet and enables the UI’s concise duration display. Good cohesion between layers.
Same note as above: a brief comment on parameter semantics would help future readers.
src/Menu/SettingsTabRenderer.cpp (1)
52-55: Differentiate “current” vs “last” build and surface ETA while compilingShowing only the elapsed time is great post-build, but when a build is in progress, including ETA is more helpful. Also, labeling “Current” vs “Last” removes ambiguity.
Apply this diff:
- if (shaderCache->GetTotalTasks() > 0) { - ImGui::Text("Last shader cache build duration: %s", - shaderCache->GetShaderStatsString(true, true).c_str()); - } + if (shaderCache->GetTotalTasks() > 0) { + const bool compiling = shaderCache->IsCompiling(); + ImGui::Text("%s shader cache build %s: %s", + compiling ? "Current" : "Last", + compiling ? "(elapsed/estimated)" : "duration", + shaderCache->GetShaderStatsString(true, !compiling).c_str()); + }If you prefer to avoid passing c_str() of a temporary to a variadic function, store the returned std::string in a local and then pass c_str(); it’s safe either way with ImGui::Text.
src/ShaderCache.cpp (1)
2617-2640: Time-only mode now supports “elapsed-only” and “elapsed/estimated” — solid UX improvementThe split between a_elapsedOnly returning only elapsed and the default returning elapsed/estimated is sound and matches UI expectations.
Edge-case: when there are no tasks, totalMs is 0 and both branches format to 00:00:00 or 00:00:00/00:00:00, which is fine. If you want to be explicit, you could early-return an empty string in a_timeOnly mode when totalTasks == 0.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Menu/SettingsTabRenderer.cpp(2 hunks)src/ShaderCache.cpp(2 hunks)src/ShaderCache.h(2 hunks)
🔇 Additional comments (2)
src/Menu/SettingsTabRenderer.cpp (1)
41-41: Tooltip rewording is clearerThe updated text better communicates what happens when disk cache is disabled. Consistent tone with the “Use Custom Shaders” tooltip.
src/ShaderCache.cpp (1)
2060-2063: Forwarding both flags from ShaderCache to CompilationSet is correctThe new overload cleanly propagates a_timeOnly and a_elapsedOnly. Matches the header.
Added an a_elapsedOnly argument to GetStatsString to easily allow the addition of the feature without doing any formatting on the original one to remove the estimated part. Also improved wording for a tooltip.
The user config is now created from the difference between defaults and current settings. finalSettings is built from the default+override+user layering and the user config is automatically removed when identical to default.
This makes Dynamic Cubemaps and VL menus consistent with the rest
|
✅ A pre-release build is available for this PR: |
1 participant
This makes Dynamic Cubemaps and VL menus consistent with the rest
Summary by CodeRabbit