chore: improved default sss settings#1446
Conversation
WalkthroughDefault values in SubsurfaceScattering::Settings were updated: SSMode now defaults to 1, and HumanProfile’s BlurRadius default changed from 1.0f to 0.5f. No other fields or files were modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Features/SubsurfaceScattering.h (1)
23-23: Behavior change: SSMode default now 1 — verify load/reset paths and shader selection.This alters out‑of‑box rendering. Please confirm:
- LoadSettings uses this new default when SSMode is absent, and RestoreDefaultSettings reflects it.
- DrawSettings/UI shows “1” as the initial mode and any clamping/range still valid.
- Shader selection branches (e.g., blur vs. Burley compute) map “1” to the intended path.
Run to audit usages and reset/serialization:
#!/bin/bash set -euo pipefail echo "Occurrences of SSMode with context:" rg -nP -C3 '\bSSMode\b' echo -e "\nReset/serialization implementations:" rg -nP -C2 '(RestoreDefaultSettings|LoadSettings|SaveSettings)\s*\(' echo -e "\nConditionals comparing SSMode to literals:" rg -nP -C3 'SSMode\s*==\s*\d' echo -e "\nPotential shader path selection sites:" rg -nP -C3 '(DrawSSS|GetComputeShader(Burley|HorizontalBlur|VerticalBlur))'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
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.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.h
⏰ 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 (1)
src/Features/SubsurfaceScattering.h (1)
25-25: HumanProfile BlurRadius 1.0f → 0.5f — check kernel math and UI bounds.Smaller radius tightens the SSS blur. Please verify:
- Gaussian/Profile/CalculateKernel logic behaves well for 0.5f (no underflow/degenerate weights).
- ImGui (or equivalent) slider min/step includes 0.5f so the default isn’t auto‑clamped.
- Any VR tuning relying on a ≥1.0f radius isn’t implicitly weakened.
Audit references to BlurRadius and UI bounds:
#!/bin/bash set -euo pipefail echo "BlurRadius usage across codebase:" rg -nP -C3 '\bBlurRadius\b|CalculateKernel\s*\(|Gaussian\s*\(|Profile\s*\(' echo -e "\nUI controls potentially constraining HumanProfile.BlurRadius:" rg -nP -C3 '(HumanProfile|BlurRadius).*(SliderFloat|DragFloat|InputFloat|SameLine)'
|
✅ A pre-release build is available for this PR: |
Summary by CodeRabbit