refactor(slf): extract Tier-2 testable helpers#56
Open
alandtse wants to merge 7 commits into
Open
Conversation
📝 WalkthroughWalkthroughRefactoring PR that extracts testable math and utility functions from three production modules into dedicated headers. ISL radius/attenuation logic moves to ISLMath, float sanitization to SettingsSanitize, and shadow-caster validation/percentile computation to ShadowCasterMath. Dependent modules delegate to these extracted functions. Comprehensive unit tests verify correctness of extracted logic. ChangesMath and Utility Extraction for Testability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 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. |
alandtse
added a commit
that referenced
this pull request
May 29, 2026
…sting More Tier-2 cpp-test extractions (continues #56): - ShadowCasterMath.h (namespace ShadowCasterManager): IsPlausibleShadowLightPtr -- the null/alignment/user-mode-range check behind ForEachShadowLight's OOB guard (the locus of the SLF OOB-CTD fixes) -- and FrameTimePercentile90. - SettingsSanitize.h: SanitizeFloat (non-finite -> lower bound, else clamp), the LightLimitFix GetCommonBufferData guard keeping bad config off the GPU. Production delegates to the headers (pure float/uintptr signatures, no engine types; no behavior change). Adds test_shadowcaster_math.cpp + test_llf_sanitize.cpp. cpp suite: 198 assertions / 56 cases. Plugin builds clean (MSVC 14.50). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alandtse
changed the title
|
✅ A pre-release build is available for this PR: |
alandtse
changed the title
alandtse
force-pushed
the
test/cpp-tier1-pure-utils
branch
from
13b55da to
6a5743e
Compare
alandtse
force-pushed
the
test/cpp-tier2-extracted-logic
branch
from
6febe52 to
681057d
Compare
Owner
Author
|
@copilot please rebase on origin/dev and resolve conflicts intelligtently. |
Copilot AI
pushed a commit
that referenced
this pull request
May 30, 2026
…sting More Tier-2 cpp-test extractions (continues #56): - ShadowCasterMath.h (namespace ShadowCasterManager): IsPlausibleShadowLightPtr -- the null/alignment/user-mode-range check behind ForEachShadowLight's OOB guard (the locus of the SLF OOB-CTD fixes) -- and FrameTimePercentile90. - SettingsSanitize.h: SanitizeFloat (non-finite -> lower bound, else clamp), the LightLimitFix GetCommonBufferData guard keeping bad config off the GPU. Production delegates to the headers (pure float/uintptr signatures, no engine types; no behavior change). Adds test_shadowcaster_math.cpp + test_llf_sanitize.cpp. cpp suite: 198 assertions / 56 cases. Plugin builds clean (MSVC 14.50). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Done. I rebased this branch onto |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/Light Limit Fix/Shaders/LightLimitFix/ClusterCullingCS.hlsl (1)
43-69:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDirect global light reads can turn this pass into a bandwidth bottleneck.
With the LDS path gone, every thread now fetches every
lights[i], so a 1024-thread group can re-read the same light 1024 times. That is a big regression risk for dense-light scenes and VR. Consider staging a compact payload (positionWS+radius/flags only) or keeping this direct-read path behind a user-facing performance toggle so heavy scenes can fall back to a cheaper culling mode.🤖 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/Light` Limit Fix/Shaders/LightLimitFix/ClusterCullingCS.hlsl around lines 43 - 69, The shader currently reads lights[i] directly inside the loop (symbols: lights, Light, LightCount, visibleLightIndices, visibleLightCount, MAX_CLUSTER_LIGHTS, FrameBuffer::WorldToView, LightIntersectsCluster), which causes every thread in a large group to fetch the same full-light record and creates a bandwidth hotspot; change the loop to read from a compact, tightly-packed payload (e.g., positionWS.xyz + radius/flags) instead of the full Light struct and stage that compact payload in small group-shared batches to amortize loads across threads (use a batch size that keeps group shared under LDS limits), or alternatively gate the direct-read path behind a user-facing performance toggle so heavy scenes/VR can use the cheaper culling mode; implement this by adding a compactLights buffer or view, a small per-group staging loop that SIMD-loads a batch into group shared then evaluates LightIntersectsCluster using the compact fields, and honor a compile-time or runtime flag to switch between direct reads and staged compact reads.
🧹 Nitpick comments (6)
.github/workflows/release-dev.yaml (1)
84-88: 💤 Low value30-minute sleep incurs significant runner cost.
The debounce sleep consumes 30 minutes of GitHub-hosted runner time per push to
dev. While the intent (coalescing merge storms viacancel-in-progress) is documented and valid, consider whether a shorter window (e.g., 5-10 minutes) would provide sufficient coalescing while reducing cost—especially if dev sees frequent pushes.Alternatively, GitHub's native concurrency debouncing could be enhanced with a workflow-level delay via a scheduled dispatch pattern, though that adds complexity.
🤖 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 @.github/workflows/release-dev.yaml around lines 84 - 88, The "Coalesce a burst of merges" workflow step is sleeping for 1800s (run: sleep 1800) which wastes GitHub runner time on every push; replace that hard 30-minute sleep with a shorter debounce (e.g., run: sleep 300 or 600 for 5–10 minutes) or remove the sleep and rely on GitHub concurrency/cancel-in-progress semantics, and update the surrounding comment/reset-debounce text accordingly; change the run value in the step that has if: github.event_name == 'push' (or remove the step) so the workflow only holds runners for a minimal debounce window.package/Shaders/Common/DirectionalShadow.hlsli (1)
4-20: ⚡ Quick winMake the
LightLimitFixdependency explicit.This helper calls
LightLimitFix::GetDirectionalShadowbut only documents the required include order in a comment. That makes future shader reuse brittle. Please include the dependency directly or wrap the LLF path in a header that owns both includes.🤖 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 `@package/Shaders/Common/DirectionalShadow.hlsli` around lines 4 - 20, The LIGHT_LIMIT_FIX path in DirectionalShadow::GetSceneDirectionalShadow calls LightLimitFix::GetDirectionalShadow but only relies on a comment about include order; explicitly include the dependency by adding an `#include` "LightLimitFix.hlsli" (or create/include a wrapper header that includes both Common/Math.hlsli and LightLimitFix.hlsli) at the top of DirectionalShadow.hlsli so LightLimitFix::GetDirectionalShadow is guaranteed to be available when LIGHT_LIMIT_FIX is defined.src/Features/LightLimitFix/ParticleLights.h (1)
3-24: ⚡ Quick winMake this header self-contained.
ParticleLights.husesstd::string,ankerl::unordered_dense::map, andRE::NiColor, but it only includes<cstdint>. Please include the owning headers here instead of relying on transitive includes or the PCH.Suggested includes
`#include` <cstdint> +#include <string> +#include <ankerl/unordered_dense.h>Add the repo's normal header for
RE::NiColoras well.🤖 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 `@src/Features/LightLimitFix/ParticleLights.h` around lines 3 - 24, The header is not self-contained: ParticleLights declares std::string, ankerl::unordered_dense::map and RE::NiColor but only includes <cstdint>; update ParticleLights.h to directly include the owning headers (e.g., add <string>, the ankerl unordered_dense header used by ankerl::unordered_dense::map, and the project header that defines RE::NiColor) so that the types used in struct Config, GradientConfig, and the member maps (particleLightConfigs, particleLightGradientConfigs) are available without relying on transitive includes or PCH.tests/cpp/test_isl_radiusmath.cpp (1)
40-62: ⚡ Quick winAdd a regression for the exact-zero radius boundary.
The suite covers the negative-sqrt
NaNpath, but not theradius == 0boundary that also needs clamping onceCalculateRadiusis hardened. A focused test here would lock down the crash-prone case.Suggested test
TEST_CASE("CalculateRadius clamps a NaN (negative-sqrt) result to 1", "[isl]") { // intensity 0 with a large size makes the sqrt argument negative -> NaN -> 1. REQUIRE(ISLMath::CalculateRadius(0.0f, false, 1.0f, 10.0f) == Approx(1.0f)); } + +TEST_CASE("CalculateRadius clamps an exact-zero radius to 1", "[isl]") +{ + // 2 * intensity == cutoff * size^2 -> sqrt(0). + REQUIRE(ISLMath::CalculateRadius(0.025f, false, 1.0f, 1.0f) == Approx(1.0f)); +}🤖 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 `@tests/cpp/test_isl_radiusmath.cpp` around lines 40 - 62, Add a regression test for the exact-zero radius boundary by adding a TEST_CASE in tests/cpp/test_isl_radiusmath.cpp that calls ISLMath::CalculateRadius with parameters that would produce an exact zero radius and asserts the result is clamped to 1.0f; reference ISLMath::CalculateRadius to locate the implementation and mirror the style of the existing "CalculateRadius clamps a NaN" test so this covers the zero-radius crash path once CalculateRadius is hardened. Ensure the test is focused and uses REQUIRE(... == Approx(1.0f)).src/Features/LightLimitFix/ShadowCasterMath.h (1)
24-35: 💤 Low valueP90 index calculation is off by one.
int(n * 0.9f)for n=10 yields index 9 (the maximum), but P90 should select the 9th smallest (index 8). This returns a value closer to the 100th percentile for typical sample sizes.Consider
int((n - 1) * 0.9f)orstd::max(0, static_cast<int>(n * 0.9f) - 1)depending on the desired interpolation behavior.♻️ Suggested fix
const int n = std::min(count, Window); float tmp[Window]; std::copy(ring, ring + n, tmp); - const int idx = static_cast<int>(n * 0.9f); + const int idx = std::max(0, static_cast<int>(n * 0.9f) - 1); std::nth_element(tmp, tmp + idx, tmp + n); return tmp[idx];🤖 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 `@src/Features/LightLimitFix/ShadowCasterMath.h` around lines 24 - 35, The P90 index in FrameTimePercentile90 is off-by-one: replace the current idx = static_cast<int>(n * 0.9f) with a computation that clamps to the valid [0, n-1] range and uses (n-1) for percentile selection (for example idx = std::max(0, static_cast<int>((n - 1) * 0.9f))) so nth_element(tmp, tmp + idx, tmp + n) picks the correct 90th percentile sample; ensure idx is an int and stays within bounds before the nth_element call in the FrameTimePercentile90 template.src/Hooks.cpp (1)
1024-1030: Consider renaming the PR to match the shipped behavior.These hook sites add particle-light render-pass culling, so the current
refactor(slf): extract Tier-2 testable helperstitle is misleading. Something likefeat(lightlimitfix): cull particle light passeswould be easier to trace. If this closes tracked work, addCloses #...orAddresses #....As per coding guidelines: provide suggestions for conventional commit titles and issue references when the current title does not describe the code changes.
🤖 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 `@src/Hooks.cpp` around lines 1024 - 1030, The PR title should be updated to reflect the actual change: these hooks (stl::write_thunk_call calls for BSBatchRenderer_RenderPassImmediately1, BSBatchRenderer_RenderPassImmediately2, BSBatchRenderer_RenderPassImmediately3 targeting BSBatchRenderer::RenderPassImmediately) implement particle-light render-pass culling, so rename the PR to a conventional commit like "feat(lightlimitfix): cull particle light passes" (or "fix(light): cull particle-light render passes") and, if this work closes an issue, add "Closes #<issue>" or "Addresses #<issue>" to the description; update the PR title and body accordingly to match the shipped behavior and include the issue reference.
🤖 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.
Inline comments:
In `@src/Features/InverseSquareLighting/RadiusMath.h`:
- Around line 25-41: The CalculateRadius implementation only checks for NaN but
must also guard against zero, negative or infinite results so downstream
divisions (e.g., in GetAttenuation's fadeZone calculation) cannot produce
INF/NaN; after computing radius in CalculateRadius, replace the single NaN check
with a validation that the value is finite and greater than a small positive
epsilon (e.g., 1e-6f) and if not return/assign 1.0f (or clamp via std::max) so
callers (GetAttenuation, fadeZone) always receive a valid positive radius.
In `@src/Features/LightLimitFix/Particle.cpp`:
- Around line 528-534: The current dimmer calc can divide by zero when
lightFadeStart == lightFadeEnd; in Particle.cpp inside the block using
distance/lightFadeStart/lightFadeEnd set dimmer explicitly for the equal-case
(or when fabs(lightFadeEnd - lightFadeStart) < epsilon): if distance <=
lightFadeStart set dimmer = 1.0f else dimmer = 0.0f, otherwise keep the existing
linear interpolation using (distance - lightFadeStart) / (lightFadeEnd -
lightFadeStart); use a small epsilon to guard floating comparisons and refer to
the variables distance, lightFadeStart, lightFadeEnd and the dimmer local
variable.
In `@src/Features/LightLimitFix/ParticleLights.cpp`:
- Around line 33-40: The code relies on
clib_util::distribution::get_configs("Data\\ParticleLights", "", ".ini") and
then iterates over the returned vector configs with a "first wins" duplicate
policy, but the vector's iteration order is filesystem-dependent; make the
policy deterministic by sorting configs (e.g., std::sort with default
lexicographic comparator) immediately after get_configs() and before any loops
that choose the first occurrence so the same filename always wins; update both
places where configs is iterated (the initial loop starting at the first
for(auto& path : configs) and the second loop mentioned in the comment) to
operate on the sorted configs.
- Around line 102-128: The color parsing in ParticleLights.cpp accepts any
hex-length string because matches only checks character set; update the
validation after removing prefixes (the code operating on str and matches) to
require str.size() == 6 || str.size() == 8 (i.e. exactly 6 or 8 hex digits)
before calling std::stoul, and log the same "[LLF] invalid color" and continue
when the length check fails; ensure you still verify characters with std::strspn
(the existing matches variable) and only attempt conversion into data.color when
both the character check and the exact-length check pass.
In `@src/Features/Upscaling.cpp`:
- Around line 515-520: The stored settings.streamlineLogLevel must be clamped as
well as the UI index to avoid persisting invalid values; update the code around
where logLevelIdx is computed so you clamp and assign the sanitized value back
into settings.streamlineLogLevel (use std::clamp with the same bounds: 0 and
IM_ARRAYSIZE(logLevels)-1) before calling ImGui::Combo and before calling
Util::UI::RestartGatedAnnotate, ensuring the persisted setting is always valid.
---
Outside diff comments:
In `@features/Light` Limit Fix/Shaders/LightLimitFix/ClusterCullingCS.hlsl:
- Around line 43-69: The shader currently reads lights[i] directly inside the
loop (symbols: lights, Light, LightCount, visibleLightIndices,
visibleLightCount, MAX_CLUSTER_LIGHTS, FrameBuffer::WorldToView,
LightIntersectsCluster), which causes every thread in a large group to fetch the
same full-light record and creates a bandwidth hotspot; change the loop to read
from a compact, tightly-packed payload (e.g., positionWS.xyz + radius/flags)
instead of the full Light struct and stage that compact payload in small
group-shared batches to amortize loads across threads (use a batch size that
keeps group shared under LDS limits), or alternatively gate the direct-read path
behind a user-facing performance toggle so heavy scenes/VR can use the cheaper
culling mode; implement this by adding a compactLights buffer or view, a small
per-group staging loop that SIMD-loads a batch into group shared then evaluates
LightIntersectsCluster using the compact fields, and honor a compile-time or
runtime flag to switch between direct reads and staged compact reads.
---
Nitpick comments:
In @.github/workflows/release-dev.yaml:
- Around line 84-88: The "Coalesce a burst of merges" workflow step is sleeping
for 1800s (run: sleep 1800) which wastes GitHub runner time on every push;
replace that hard 30-minute sleep with a shorter debounce (e.g., run: sleep 300
or 600 for 5–10 minutes) or remove the sleep and rely on GitHub
concurrency/cancel-in-progress semantics, and update the surrounding
comment/reset-debounce text accordingly; change the run value in the step that
has if: github.event_name == 'push' (or remove the step) so the workflow only
holds runners for a minimal debounce window.
In `@package/Shaders/Common/DirectionalShadow.hlsli`:
- Around line 4-20: The LIGHT_LIMIT_FIX path in
DirectionalShadow::GetSceneDirectionalShadow calls
LightLimitFix::GetDirectionalShadow but only relies on a comment about include
order; explicitly include the dependency by adding an `#include`
"LightLimitFix.hlsli" (or create/include a wrapper header that includes both
Common/Math.hlsli and LightLimitFix.hlsli) at the top of DirectionalShadow.hlsli
so LightLimitFix::GetDirectionalShadow is guaranteed to be available when
LIGHT_LIMIT_FIX is defined.
In `@src/Features/LightLimitFix/ParticleLights.h`:
- Around line 3-24: The header is not self-contained: ParticleLights declares
std::string, ankerl::unordered_dense::map and RE::NiColor but only includes
<cstdint>; update ParticleLights.h to directly include the owning headers (e.g.,
add <string>, the ankerl unordered_dense header used by
ankerl::unordered_dense::map, and the project header that defines RE::NiColor)
so that the types used in struct Config, GradientConfig, and the member maps
(particleLightConfigs, particleLightGradientConfigs) are available without
relying on transitive includes or PCH.
In `@src/Features/LightLimitFix/ShadowCasterMath.h`:
- Around line 24-35: The P90 index in FrameTimePercentile90 is off-by-one:
replace the current idx = static_cast<int>(n * 0.9f) with a computation that
clamps to the valid [0, n-1] range and uses (n-1) for percentile selection (for
example idx = std::max(0, static_cast<int>((n - 1) * 0.9f))) so nth_element(tmp,
tmp + idx, tmp + n) picks the correct 90th percentile sample; ensure idx is an
int and stays within bounds before the nth_element call in the
FrameTimePercentile90 template.
In `@src/Hooks.cpp`:
- Around line 1024-1030: The PR title should be updated to reflect the actual
change: these hooks (stl::write_thunk_call calls for
BSBatchRenderer_RenderPassImmediately1, BSBatchRenderer_RenderPassImmediately2,
BSBatchRenderer_RenderPassImmediately3 targeting
BSBatchRenderer::RenderPassImmediately) implement particle-light render-pass
culling, so rename the PR to a conventional commit like "feat(lightlimitfix):
cull particle light passes" (or "fix(light): cull particle-light render passes")
and, if this work closes an issue, add "Closes #<issue>" or "Addresses #<issue>"
to the description; update the PR title and body accordingly to match the
shipped behavior and include the issue reference.
In `@tests/cpp/test_isl_radiusmath.cpp`:
- Around line 40-62: Add a regression test for the exact-zero radius boundary by
adding a TEST_CASE in tests/cpp/test_isl_radiusmath.cpp that calls
ISLMath::CalculateRadius with parameters that would produce an exact zero radius
and asserts the result is clamped to 1.0f; reference ISLMath::CalculateRadius to
locate the implementation and mirror the style of the existing "CalculateRadius
clamps a NaN" test so this covers the zero-radius crash path once
CalculateRadius is hardened. Ensure the test is focused and uses REQUIRE(... ==
Approx(1.0f)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbb50177-cb38-42bb-8595-e6eb85e5312d
📒 Files selected for processing (37)
.github/actions/publish-release/action.yaml.github/workflows/pr-checks.yaml.github/workflows/release-build.yaml.github/workflows/release-dev.yamlextern/CommonLibSSE-NGfeatures/Light Limit Fix/Shaders/LightLimitFix/ClusterCullingCS.hlslfeatures/Light Limit Fix/Shaders/LightLimitFix/Common.hlslipackage/Shaders/Common/DirectionalShadow.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/Particle.hlslpackage/Shaders/RunGrass.hlslsrc/Features/InverseSquareLighting.cppsrc/Features/InverseSquareLighting.hsrc/Features/InverseSquareLighting/RadiusMath.hsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix.hsrc/Features/LightLimitFix/Particle.cppsrc/Features/LightLimitFix/ParticleLights.cppsrc/Features/LightLimitFix/ParticleLights.hsrc/Features/LightLimitFix/SettingsSanitize.hsrc/Features/LightLimitFix/ShadowCasterManager.cppsrc/Features/LightLimitFix/ShadowCasterManager.hsrc/Features/LightLimitFix/ShadowCasterMath.hsrc/Features/RenderDoc.cppsrc/Features/Upscaling.cppsrc/Features/VolumetricLighting.cppsrc/Hooks.cppsrc/Hooks.hsrc/Utils/BootSnapshot.hsrc/Utils/StringUtils.hsrc/Utils/UI.htests/cpp/CMakeLists.txttests/cpp/test_isl_radiusmath.cpptests/cpp/test_llf_sanitize.cpptests/cpp/test_shadowcaster_math.cpptests/cpp/test_stringutils.cpp
Comment on lines
+528
to
+534
| float dimmer = 0.0f; | ||
| if (distance < lightFadeStart || lightFadeEnd == 0.0f) | ||
| dimmer = 1.0f; | ||
| else if (distance <= lightFadeEnd) | ||
| dimmer = 1.0f - ((distance - lightFadeStart) / (lightFadeEnd - lightFadeStart)); | ||
| else | ||
| dimmer = 0.0f; |
There was a problem hiding this comment.
Division by zero when lightFadeStart == lightFadeEnd.
If both engine globals are equal (non-zero), the denominator (lightFadeEnd - lightFadeStart) is zero, producing NaN/Inf that propagates to GPU lighting math.
🛡️ Proposed fix
float dimmer = 0.0f;
- if (distance < lightFadeStart || lightFadeEnd == 0.0f)
+ if (distance < lightFadeStart || lightFadeEnd == 0.0f || lightFadeEnd <= lightFadeStart)
dimmer = 1.0f;
else if (distance <= lightFadeEnd)
dimmer = 1.0f - ((distance - lightFadeStart) / (lightFadeEnd - lightFadeStart));🤖 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 `@src/Features/LightLimitFix/Particle.cpp` around lines 528 - 534, The current
dimmer calc can divide by zero when lightFadeStart == lightFadeEnd; in
Particle.cpp inside the block using distance/lightFadeStart/lightFadeEnd set
dimmer explicitly for the equal-case (or when fabs(lightFadeEnd -
lightFadeStart) < epsilon): if distance <= lightFadeStart set dimmer = 1.0f else
dimmer = 0.0f, otherwise keep the existing linear interpolation using (distance
- lightFadeStart) / (lightFadeEnd - lightFadeStart); use a small epsilon to
guard floating comparisons and refer to the variables distance, lightFadeStart,
lightFadeEnd and the dimmer local variable.
Comment on lines
+33
to
+40
| auto configs = clib_util::distribution::get_configs("Data\\ParticleLights", "", ".ini"); | ||
|
|
||
| if (configs.empty()) { | ||
| logger::warn("[LLF] No .ini files were found within the Data\\ParticleLights folder, aborting..."); | ||
| } else { | ||
| logger::info("[LLF] {} matching inis found", configs.size()); | ||
|
|
||
| for (auto& path : configs) { |
There was a problem hiding this comment.
Make the duplicate "first wins" policy deterministic.
Right now the surviving config depends on whatever order get_configs() returns. If that order follows filesystem iteration, two installs with the same duplicate filenames can pick different winners. Sort configs before both loops so "keep first entry" is stable.
♻️ Proposed fix
- auto configs = clib_util::distribution::get_configs("Data\\ParticleLights", "", ".ini");
+ auto configs = clib_util::distribution::get_configs("Data\\ParticleLights", "", ".ini");
+ std::sort(configs.begin(), configs.end());
...
- auto configs = clib_util::distribution::get_configs("Data\\ParticleLights\\Gradients", "", ".ini");
+ auto configs = clib_util::distribution::get_configs("Data\\ParticleLights\\Gradients", "", ".ini");
+ std::sort(configs.begin(), configs.end());Also applies to: 80-89
🤖 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 `@src/Features/LightLimitFix/ParticleLights.cpp` around lines 33 - 40, The code
relies on clib_util::distribution::get_configs("Data\\ParticleLights", "",
".ini") and then iterates over the returned vector configs with a "first wins"
duplicate policy, but the vector's iteration order is filesystem-dependent; make
the policy deterministic by sorting configs (e.g., std::sort with default
lexicographic comparator) immediately after get_configs() and before any loops
that choose the first occurrence so the same filename always wins; update both
places where configs is iterated (the initial loop starting at the first
for(auto& path : configs) and the second loop mentioned in the comment) to
operate on the sorted configs.
Comment on lines
+102
to
+128
| constexpr std::string_view prefix1 = "0x"; | ||
| constexpr std::string_view prefix2 = "#"; | ||
| constexpr std::string_view cset = "0123456789ABCDEFabcdef"; | ||
|
|
||
| const char* value = ini.GetValue("Gradient", "Color"); | ||
| if (value && strcmp(value, "") != 0) { | ||
| std::string_view str = value; | ||
|
|
||
| if (str.starts_with(prefix1)) | ||
| str.remove_prefix(prefix1.size()); | ||
| if (str.starts_with(prefix2)) | ||
| str.remove_prefix(prefix2.size()); | ||
|
|
||
| bool matches = std::strspn(str.data(), cset.data()) == str.size(); | ||
|
|
||
| if (matches) { | ||
| try { | ||
| uint32_t color = static_cast<uint32_t>(std::stoul(std::string(str), nullptr, 16)); | ||
| data.color = color; | ||
| } catch (const std::exception&) { | ||
| logger::error("[LLF] invalid color"); | ||
| continue; | ||
| } | ||
| } else { | ||
| logger::error("[LLF] invalid color"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Reject gradient colors with unsupported hex widths.
matches only validates the character set, so values like #1 or #12345 are accepted and packed as unintended 32-bit colors. Requiring exactly 6 or 8 hex digits makes malformed configs fail closed instead of silently producing the wrong gradient.
🛡️ Proposed fix
- bool matches = std::strspn(str.data(), cset.data()) == str.size();
+ const bool hasSupportedWidth = str.size() == 6 || str.size() == 8;
+ bool matches = hasSupportedWidth &&
+ std::strspn(str.data(), cset.data()) == str.size();📝 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.
Suggested change
| constexpr std::string_view prefix1 = "0x"; | |
| constexpr std::string_view prefix2 = "#"; | |
| constexpr std::string_view cset = "0123456789ABCDEFabcdef"; | |
| const char* value = ini.GetValue("Gradient", "Color"); | |
| if (value && strcmp(value, "") != 0) { | |
| std::string_view str = value; | |
| if (str.starts_with(prefix1)) | |
| str.remove_prefix(prefix1.size()); | |
| if (str.starts_with(prefix2)) | |
| str.remove_prefix(prefix2.size()); | |
| bool matches = std::strspn(str.data(), cset.data()) == str.size(); | |
| if (matches) { | |
| try { | |
| uint32_t color = static_cast<uint32_t>(std::stoul(std::string(str), nullptr, 16)); | |
| data.color = color; | |
| } catch (const std::exception&) { | |
| logger::error("[LLF] invalid color"); | |
| continue; | |
| } | |
| } else { | |
| logger::error("[LLF] invalid color"); | |
| continue; | |
| } | |
| constexpr std::string_view prefix1 = "0x"; | |
| constexpr std::string_view prefix2 = "#"; | |
| constexpr std::string_view cset = "0123456789ABCDEFabcdef"; | |
| const char* value = ini.GetValue("Gradient", "Color"); | |
| if (value && strcmp(value, "") != 0) { | |
| std::string_view str = value; | |
| if (str.starts_with(prefix1)) | |
| str.remove_prefix(prefix1.size()); | |
| if (str.starts_with(prefix2)) | |
| str.remove_prefix(prefix2.size()); | |
| const bool hasSupportedWidth = str.size() == 6 || str.size() == 8; | |
| bool matches = hasSupportedWidth && | |
| std::strspn(str.data(), cset.data()) == str.size(); | |
| if (matches) { | |
| try { | |
| uint32_t color = static_cast<uint32_t>(std::stoul(std::string(str), nullptr, 16)); | |
| data.color = color; | |
| } catch (const std::exception&) { | |
| logger::error("[LLF] invalid color"); | |
| continue; | |
| } | |
| } else { | |
| logger::error("[LLF] invalid color"); | |
| continue; | |
| } |
🤖 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 `@src/Features/LightLimitFix/ParticleLights.cpp` around lines 102 - 128, The
color parsing in ParticleLights.cpp accepts any hex-length string because
matches only checks character set; update the validation after removing prefixes
(the code operating on str and matches) to require str.size() == 6 || str.size()
== 8 (i.e. exactly 6 or 8 hex digits) before calling std::stoul, and log the
same "[LLF] invalid color" and continue when the length check fails; ensure you
still verify characters with std::strspn (the existing matches variable) and
only attempt conversion into data.color when both the character check and the
exact-length check pass.
Comment on lines
+515
to
+520
| int logLevelIdx = std::clamp(static_cast<int>(settings.streamlineLogLevel), | ||
| 0, IM_ARRAYSIZE(logLevels) - 1); | ||
| if (ImGui::Combo("Streamline Logging", &logLevelIdx, logLevels, IM_ARRAYSIZE(logLevels))) { | ||
| settings.streamlineLogLevel = static_cast<uint>(logLevelIdx); | ||
| } | ||
| Util::UI::DrawSettingDiff(bootSnapshot, settings, &Settings::streamlineLogLevel); | ||
| if (auto _tt = Util::HoverTooltipWrapper()) { | ||
| ImGui::Text("Streamline logging controls the verbosity of NVIDIA Streamline backend logs. Useful for debugging issues with DLSS/DLSS-G."); | ||
| } | ||
| Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::streamlineLogLevel, |
There was a problem hiding this comment.
Clamp the stored log-level value, not just the UI index.
Right now a stale or hand-edited settings.streamlineLogLevel is only sanitized in logLevelIdx. If the user opens settings and never changes this combo, the invalid value remains in settings, so restart diff/save paths can still see and persist the bad value.
Suggested fix
const char* logLevels[] = { "Off", "Default", "Verbose" };
- // Clamp before use: streamlineLogLevel is JSON-persisted and could be out
- // of range (or a value that overflows the int cast) on a stale or
- // hand-edited config; an unclamped value would index logLevels OOB.
- int logLevelIdx = std::clamp(static_cast<int>(settings.streamlineLogLevel),
- 0, IM_ARRAYSIZE(logLevels) - 1);
+ // Clamp before use: streamlineLogLevel is JSON-persisted and could be out
+ // of range on a stale or hand-edited config.
+ constexpr uint kMaxLogLevel = static_cast<uint>(IM_ARRAYSIZE(logLevels) - 1);
+ settings.streamlineLogLevel = std::min(settings.streamlineLogLevel, kMaxLogLevel);
+ int logLevelIdx = static_cast<int>(settings.streamlineLogLevel);
if (ImGui::Combo("Streamline Logging", &logLevelIdx, logLevels, IM_ARRAYSIZE(logLevels))) {
settings.streamlineLogLevel = static_cast<uint>(logLevelIdx);
}📝 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.
Suggested change
| int logLevelIdx = std::clamp(static_cast<int>(settings.streamlineLogLevel), | |
| 0, IM_ARRAYSIZE(logLevels) - 1); | |
| if (ImGui::Combo("Streamline Logging", &logLevelIdx, logLevels, IM_ARRAYSIZE(logLevels))) { | |
| settings.streamlineLogLevel = static_cast<uint>(logLevelIdx); | |
| } | |
| Util::UI::DrawSettingDiff(bootSnapshot, settings, &Settings::streamlineLogLevel); | |
| if (auto _tt = Util::HoverTooltipWrapper()) { | |
| ImGui::Text("Streamline logging controls the verbosity of NVIDIA Streamline backend logs. Useful for debugging issues with DLSS/DLSS-G."); | |
| } | |
| Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::streamlineLogLevel, | |
| const char* logLevels[] = { "Off", "Default", "Verbose" }; | |
| // Clamp before use: streamlineLogLevel is JSON-persisted and could be out | |
| // of range on a stale or hand-edited config. | |
| constexpr uint kMaxLogLevel = static_cast<uint>(IM_ARRAYSIZE(logLevels) - 1); | |
| settings.streamlineLogLevel = std::min(settings.streamlineLogLevel, kMaxLogLevel); | |
| int logLevelIdx = static_cast<int>(settings.streamlineLogLevel); | |
| if (ImGui::Combo("Streamline Logging", &logLevelIdx, logLevels, IM_ARRAYSIZE(logLevels))) { | |
| settings.streamlineLogLevel = static_cast<uint>(logLevelIdx); | |
| } | |
| Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::streamlineLogLevel, |
🤖 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 `@src/Features/Upscaling.cpp` around lines 515 - 520, The stored
settings.streamlineLogLevel must be clamped as well as the UI index to avoid
persisting invalid values; update the code around where logLevelIdx is computed
so you clamp and assign the sanitized value back into
settings.streamlineLogLevel (use std::clamp with the same bounds: 0 and
IM_ARRAYSIZE(logLevels)-1) before calling ImGui::Combo and before calling
Util::UI::RestartGatedAnnotate, ensuring the persisted setting is always valid.
Move InverseSquareLighting's radius/attenuation constants and the CalculateRadius / GetAttenuation / SmoothStep math into a new pure header (Features/InverseSquareLighting/RadiusMath.h, namespace ISLMath). The functions take only plain floats, so the header compiles standalone into the cpp_tests binary with no game/RE runtime. The class's static methods now delegate to it -- call sites (incl. LightEditor) are unchanged. Adds test_isl_radiusmath.cpp (13 assertions): SmoothStep ramp/clamp, CalculateRadius closed-form + shadow/override branches + NaN->1 guard, GetAttenuation peak/vanish/monotonic. Validated: cpp suite + plugin build. Phase 2 (Tier 2) of the cpp-test expansion; follows #55. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ShadowCasterManager.cpp pulls in the large exprtk header; under MSVC 14.50 its object file exceeds the COFF section limit (C1128). /bigobj is the standard fix (already used for cpp-mcp). CI's VS2022 toolchain didn't trip it, but local VS2026/14.50 clean builds need it -- surfaced here because the ISL header change forces a ShadowCasterManager recompile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sting More Tier-2 cpp-test extractions (continues #56): - ShadowCasterMath.h (namespace ShadowCasterManager): IsPlausibleShadowLightPtr -- the null/alignment/user-mode-range check behind ForEachShadowLight's OOB guard (the locus of the SLF OOB-CTD fixes) -- and FrameTimePercentile90. - SettingsSanitize.h: SanitizeFloat (non-finite -> lower bound, else clamp), the LightLimitFix GetCommonBufferData guard keeping bad config off the GPU. Production delegates to the headers (pure float/uintptr signatures, no engine types; no behavior change). Adds test_shadowcaster_math.cpp + test_llf_sanitize.cpp. cpp suite: 198 assertions / 56 cases. Plugin builds clean (MSVC 14.50). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The exprtk-heavy ShadowCasterManager.cpp only overflows the COFF section limit (C1128) in a Debug compile, where /Gy emits per-function COMDATs without /GL deferring codegen. Both CI and local builds are Release-only (/GL LTCG produces thin IL objects), where exprtk compiles clean without /bigobj -- verified by a from-scratch Release build of dev under VS2026. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alandtse
force-pushed
the
test/cpp-tier2-extracted-logic
branch
from
0b9f7a8 to
64d4537
Compare
There was a problem hiding this comment.
Pull request overview
This PR continues the cpp unit-test expansion (Tier-2) by extracting engine-independent helper logic from runtime-coupled feature code into small header-only utilities, then adding Catch2 tests to validate the extracted math/sanitization behavior. Production code paths are updated to delegate to the extracted helpers.
Changes:
- Added new pure helper headers for ISL radius/attenuation math, LLF float sanitization, and ShadowCasterManager pointer validation + frame-time P90.
- Updated feature/runtime code (
InverseSquareLighting,ShadowCasterManager,LightLimitFix) to delegate to the extracted helpers. - Added new Catch2 test files and wired them into
cpp_tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cpp/test_shadowcaster_math.cpp | Adds unit tests for ShadowCaster pointer plausibility + frame-time P90 helper. |
| tests/cpp/test_llf_sanitize.cpp | Adds unit tests for LLF float sanitization behavior (clamp + non-finite handling). |
| tests/cpp/test_isl_radiusmath.cpp | Adds unit tests for ISL SmoothStep, radius closed-form, and attenuation properties. |
| tests/cpp/CMakeLists.txt | Registers the new test sources in the cpp_tests target. |
| src/Features/LightLimitFix/ShadowCasterMath.h | New header with extracted pure helpers for SCM pointer plausibility + P90. |
| src/Features/LightLimitFix/ShadowCasterManager.h | Switches accumulator iteration to use the extracted plausibility helper. |
| src/Features/LightLimitFix/ShadowCasterManager.cpp | Switches percentile computation to the extracted P90 helper. |
| src/Features/LightLimitFix/SettingsSanitize.h | New header with extracted float sanitization helper. |
| src/Features/LightLimitFix.cpp | Delegates float sanitization to the extracted helper. |
| src/Features/InverseSquareLighting/RadiusMath.h | New header with extracted ISL radius/attenuation math + constants. |
| src/Features/InverseSquareLighting.h | Removes in-class constants/method declarations now housed in RadiusMath.h. |
| src/Features/InverseSquareLighting.cpp | Delegates radius/attenuation calculations to ISLMath. |
alandtse
force-pushed
the
test/cpp-tier2-extracted-logic
branch
from
4c6fe65 to
64d4537
Compare
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Address review findings on the newly extracted pure helpers (the unit tests make these degenerate paths cheap to cover): - ISLMath::CalculateRadius clamped only NaN. An exact-zero sqrt argument yields radius 0 (callers divide by radius -> 0/0 SmoothStep) and a zero cutoffOverride yields +inf (sqrt(+inf) is not NaN, so isnan missed it). Clamp any non-finite or non-positive result to 1.0 instead. - IsPlausibleShadowLightPtr accepted near-null addresses (e.g. 0x8). The Windows x64 low 64 KiB is never a valid user mapping, so reject raw below 0x10000 to avoid dereferencing near-null garbage (AV/CTD). - FrameTimePercentile90 guarded only count == 0; a negative count would drive a negative n into std::copy / std::nth_element (UB). Guard <= 0. Tests extended with the zero/inf radius cases, the near-null pointer case, and a negative-count case. Also re-applies the pre-commit.ci comment-alignment formatting on test_llf_sanitize.cpp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Features/LightLimitFix/SettingsSanitize.h (1)
15-18: ⚡ Quick winConsider validating bounds or documenting preconditions.
The function correctly handles non-finite
vby returninglo, but it doesn't validate that the bounds themselves are finite or thatlo <= hi. If a caller passes NaN forlo, the function would return NaN (defeating its purpose). Ifhi < lo,std::clamphas undefined behavior (precondition violation in C++20).Consider either:
- Adding defensive checks:
assert(std::isfinite(lo) && std::isfinite(hi) && lo <= hi)- Documenting the preconditions in a comment above the function
🛡️ Proposed defensive validation
// Clamp a user/config float to [lo, hi]. std::clamp passes NaN through // unchanged (every NaN comparison is false), which would let a non-finite // value reach the GPU and cause divisions / infinite loops / corruption, so // reject non-finite inputs explicitly and fall back to the lower bound for // degraded-but-stable behavior. + // Precondition: lo and hi must be finite, and lo <= hi. inline float SanitizeFloat(float v, float lo, float hi) { + assert(std::isfinite(lo) && std::isfinite(hi) && lo <= hi); return std::isfinite(v) ? std::clamp(v, lo, hi) : lo; }🤖 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 `@src/Features/LightLimitFix/SettingsSanitize.h` around lines 15 - 18, SanitizeFloat currently checks only the input v but not the bounds; add validation to ensure lo and hi are finite and lo <= hi (e.g., in SanitizeFloat add an assertion like assert(std::isfinite(lo) && std::isfinite(hi) && lo <= hi) or, alternatively, document the precondition above the function that callers must provide finite bounds with lo <= hi); ensure the chosen approach prevents std::clamp from receiving invalid bounds (or documents that responsibility) and reference the SanitizeFloat function in the comment or assertion.tests/cpp/test_shadowcaster_math.cpp (1)
41-47: ⚡ Quick winConsider adding a
count > Windowcase.The existing tests cover
count == 0,count == Window, andcount < Window, but notcount > Window— the branch wherestd::min(count, Window)clampsnand prevents an out-of-bounds copy. A regression that drops the clamp would read pastring; a test pins that contract.💚 Suggested additional test
TEST_CASE("FrameTimePercentile90 clamps count above the window size", "[scm]") { // count > Window: n is clamped to Window (10); all slots sampled. float ring[10] = { 5, 2, 9, 1, 7, 3, 10, 4, 8, 6 }; REQUIRE(FrameTimePercentile90(ring, 99) == Approx(10.0f)); }🤖 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 `@tests/cpp/test_shadowcaster_math.cpp` around lines 41 - 47, Add a test that validates FrameTimePercentile90 correctly clamps count above the Window size: create a TEST_CASE that supplies a ring buffer of size Window (e.g., 10 values) and calls FrameTimePercentile90(ring, 99) (or any count > Window) and REQUIREs the expected percentile (e.g., Approx(10.0f) for the provided sample); this ensures the std::min(count, Window) behavior is preserved and prevents out-of-bounds reads in FrameTimePercentile90.
🤖 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 `@src/Features/LightLimitFix/SettingsSanitize.h`:
- Around line 15-18: SanitizeFloat currently checks only the input v but not the
bounds; add validation to ensure lo and hi are finite and lo <= hi (e.g., in
SanitizeFloat add an assertion like assert(std::isfinite(lo) &&
std::isfinite(hi) && lo <= hi) or, alternatively, document the precondition
above the function that callers must provide finite bounds with lo <= hi);
ensure the chosen approach prevents std::clamp from receiving invalid bounds (or
documents that responsibility) and reference the SanitizeFloat function in the
comment or assertion.
In `@tests/cpp/test_shadowcaster_math.cpp`:
- Around line 41-47: Add a test that validates FrameTimePercentile90 correctly
clamps count above the Window size: create a TEST_CASE that supplies a ring
buffer of size Window (e.g., 10 values) and calls FrameTimePercentile90(ring,
99) (or any count > Window) and REQUIREs the expected percentile (e.g.,
Approx(10.0f) for the provided sample); this ensures the std::min(count, Window)
behavior is preserved and prevents out-of-bounds reads in FrameTimePercentile90.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e1a458f-2da2-41a9-9544-a872e216cf22
📒 Files selected for processing (12)
src/Features/InverseSquareLighting.cppsrc/Features/InverseSquareLighting.hsrc/Features/InverseSquareLighting/RadiusMath.hsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix/SettingsSanitize.hsrc/Features/LightLimitFix/ShadowCasterManager.cppsrc/Features/LightLimitFix/ShadowCasterManager.hsrc/Features/LightLimitFix/ShadowCasterMath.htests/cpp/CMakeLists.txttests/cpp/test_isl_radiusmath.cpptests/cpp/test_llf_sanitize.cpptests/cpp/test_shadowcaster_math.cpp
✅ Files skipped from review due to trivial changes (2)
- tests/cpp/CMakeLists.txt
- src/Features/LightLimitFix.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Features/LightLimitFix/ShadowCasterManager.cpp
- src/Features/InverseSquareLighting.cpp
Address CodeRabbit nitpicks on the extracted helpers: - Add a FrameTimePercentile90 count > Window case pinning the std::min(count, Window) clamp that keeps the copy in bounds. - Document SanitizeFloat's precondition (finite lo <= hi): only the value is untrusted; bounds are compile-time constants at call sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2 (Tier 2) of the cpp-test expansion — pure logic lifted out of engine-coupled
.cpps into standalone, testable TUs. Production delegates to each extracted header (pure signatures, no behavior change). Stacked on #55; auto-retargets todevwhen #55 merges.Extracted + tested units
Features/InverseSquareLighting/RadiusMath.h(ISLMath)Features/LightLimitFix/ShadowCasterMath.hIsPlausibleShadowLightPtrFrameTimePercentile90Features/LightLimitFix/SettingsSanitize.hSanitizeFloatInverseSquareLighting,ShadowCasterManager(validator + percentile), andLightLimitFix::GetCommonBufferDatanow delegate to these headers; call sites unchanged.Build note (no
/bigobjneeded)ShadowCasterManager.cpp(exprtk-heavy) initially overflowed the COFF section limit under MSVC 14.50 (C1128) when the header change forced its recompile, so/bigobjwas added as a stopgap. Extracting the math into headers shrank the translation unit back under the section limit, so/bigobjwas dropped again — the two build commits net to zero and the plugin target is unchanged fromdev. Verified by a clean local plugin build under MSVC 14.50 without/bigobj. (CI's VS2022 was never affected.)Validation
CommunityShaders) builds clean locally (MSVC 14.50) with all delegations — no/bigobjon the plugin target.Deferred (with reasons) — not in this PR
exprtkheader into thecpp_testsbinary, turning test builds from seconds → minutes. Needs a decision (separate opt-in slow-tests target vs accept the cost) → its own PR.IEqualsissue suggested —FileSystem.hdragsFormat.h+WinApi.h, both referencingREL::Version. NeedsSanitizeFileName/DiffJsonlifted into a pure TU.mean/median: trivial near-duplicates of the already-testedPerfUtils::Mean/Median— negligible marginal value.Summary by CodeRabbit
Refactor
Tests