fix(dynamic cubemap): blend native cubemap fallback#2328
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a user-configurable ReflectionFallbackAmount to Dynamic Cubemaps: defined in C++ headers, persisted via JSON, exposed in ImGui, passed into the GPU constant buffer, and applied in the compute shader to scale fallback reflection blending. ChangesReflection Fallback Amount Feature
Sequence Diagram(s)sequenceDiagram
participant ImGui as ImGui (Editor)
participant Settings as DynamicCubemaps::Settings
participant CB as SharedData::CubemapCreatorSettings
participant Shader as InferCubemapCS
ImGui->>Settings: slider sets ReflectionFallbackAmount
Settings->>Settings: clamp to kReflectionFallbackMin/Max
Settings->>CB: write ReflectionFallbackAmount into constant buffer
Shader->>CB: read ReflectionFallbackAmount
Shader->>Shader: compute fallbackWeight = saturate(mipLevel/7.0) * ReflectionFallbackAmount
Shader->>Shader: blend sampled reflections using fallbackWeight
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Comment |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
features/Dynamic Cubemaps/Shaders/Features/DynamicCubemaps.ini (1)
2-2: Consider tightening PR metadata for traceability.Suggested title (<=50 chars):
fix(dynamiccubemaps): reduce fallback blend
If this maps to a tracked issue, addFixes #<id>in the PR description.As per coding guidelines, "provide suggestions for Conventional Commit Titles" and "Suggest adding appropriate GitHub keywords: 'Fixes
#123' or 'Closes#123'."🤖 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 `@features/Dynamic` Cubemaps/Shaders/Features/DynamicCubemaps.ini at line 2, Update the PR metadata: set the PR title to the suggested Conventional Commit short title "fix(dynamiccubemaps): reduce fallback blend" and add a GitHub keyword line like "Fixes #<id>" in the PR description (replace <id> with the tracked issue number); reference the DynamicCubemaps module by mentioning Features/Dynamic Cubemaps/Shaders/Features/DynamicCubemaps.ini (e.g., the Version = 2-3-2 entry) in the PR body if helpful for traceability.
🤖 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.
Nitpick comments:
In `@features/Dynamic` Cubemaps/Shaders/Features/DynamicCubemaps.ini:
- Line 2: Update the PR metadata: set the PR title to the suggested Conventional
Commit short title "fix(dynamiccubemaps): reduce fallback blend" and add a
GitHub keyword line like "Fixes #<id>" in the PR description (replace <id> with
the tracked issue number); reference the DynamicCubemaps module by mentioning
Features/Dynamic Cubemaps/Shaders/Features/DynamicCubemaps.ini (e.g., the
Version = 2-3-2 entry) in the PR body if helpful for traceability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad3bbb2f-578c-4b15-9a56-9b12407f7d30
📒 Files selected for processing (5)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/InferCubemapCS.hlslfeatures/Dynamic Cubemaps/Shaders/Features/DynamicCubemaps.inipackage/Shaders/Common/SharedData.hlslisrc/Features/DynamicCubemaps.cppsrc/Features/DynamicCubemaps.h
|
✅ A pre-release build is available for this PR: |
|
When enabling IBL the effect of this significantly diminishes. I am not sure if it is a worthwile slider to add when IBL already has such a massive impact on the same metallics. |
IBL has nothing to do with this. Also, metallics are broken with IBL unless you use DALC. |
|
Okay this slider makes sense, Am just surprised that it wasn't done like this before. |
Blends native cubemap fallback in dynamic cubemap reflections %50 and adds a slider in the UI.
Before (player is under shaded area):

After (player is under shaded area):

Summary by CodeRabbit
New Features
Chores