fix(sss): disable burley, broken#1484
Conversation
|
✅ A pre-release build is available for this PR: |
WalkthroughThe UI controls for switching Subsurface Scattering modes were removed by commenting out the radio buttons in the CPP. A note marks Burley as disabled. The default SSMode in the header was changed from 1 to 0. No other logic or public interfaces were modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/SubsurfaceScattering.cpp (1)
72-97: Burley path still reachable via config (SSMode=1) — not truly disabled.Hiding the radios is insufficient; existing configs can still select the Burley branch. Clamp on load.
Add a hard clamp in LoadSettings:
void SubsurfaceScattering::LoadSettings(json& o_json) { settings = o_json; // Force-disable Burley mode regardless of saved value if (settings.SSMode != 0) { settings.SSMode = 0; logger::info("[SSS] Burley disabled — forcing SSMode=0 from loaded settings."); } }Optionally also sanitize when saving to prevent stale “1” persisting:
void SubsurfaceScattering::SaveSettings(json& o_json) { if (settings.SSMode != 0) settings.SSMode = 0; o_json = settings; }
🧹 Nitpick comments (2)
src/Features/SubsurfaceScattering.cpp (2)
33-36: UI: Replace commented controls with an explicit disabled notice.Avoid dead UI space and make the state obvious.
- // Burley currently broken, disabled - // ImGui::RadioButton("Separable SSS", &settings.SSMode, 0); - // ImGui::SameLine(); - // ImGui::RadioButton("Burley", &settings.SSMode, 1); + ImGui::TextDisabled("SSS mode: Burley is disabled (broken).");
284-296: Ensure Burley dispatch is unreachable.If you don’t clamp on load, this branch will still run. Either keep the load‑time clamp (preferred) or guard here with a runtime fallback.
Example fallback at the start of DrawSSS’s dispatch section:
if (settings.SSMode == 1) { logger::warn("[SSS] Burley mode requested but disabled; falling back to separable SSS."); settings.SSMode = 0; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/SubsurfaceScattering.cpp(1 hunks)src/Features/SubsurfaceScattering.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/SubsurfaceScattering.cppsrc/Features/SubsurfaceScattering.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/SubsurfaceScattering.cppsrc/Features/SubsurfaceScattering.h
🔇 Additional comments (1)
src/Features/SubsurfaceScattering.h (1)
23-23: Clamp SSMode on config load; ensure legacy JSON can't re-enable Burley.Defaulting SSMode = 0 is fine; enforce/override it during configuration deserialization so pre‑existing JSON with SSMode=1 cannot re-enable Burley. I could not locate src/Features/SubsurfaceScattering.h or any SSMode occurrences in this repository — confirm the file path or add the clamp in the config parser (after parsing set/limit SSMode to 0).
Summary by CodeRabbit
Bug Fixes
Chores