From 44500e772b8c0f5221879f26d86405d651c11381 Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Thu, 28 May 2026 17:54:45 -0700 Subject: [PATCH 1/7] refactor(isl): extract radius/attenuation math to a testable header 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) --- src/Features/InverseSquareLighting.cpp | 21 ++---- src/Features/InverseSquareLighting.h | 12 +--- .../InverseSquareLighting/RadiusMath.h | 46 +++++++++++++ tests/cpp/CMakeLists.txt | 1 + tests/cpp/test_isl_radiusmath.cpp | 64 +++++++++++++++++++ 5 files changed, 118 insertions(+), 26 deletions(-) create mode 100644 src/Features/InverseSquareLighting/RadiusMath.h create mode 100644 tests/cpp/test_isl_radiusmath.cpp diff --git a/src/Features/InverseSquareLighting.cpp b/src/Features/InverseSquareLighting.cpp index 2c8212f29f..bdff81bd57 100644 --- a/src/Features/InverseSquareLighting.cpp +++ b/src/Features/InverseSquareLighting.cpp @@ -1,5 +1,6 @@ #include "InverseSquareLighting.h" #include "Features/InverseSquareLighting/Common.h" +#include "Features/InverseSquareLighting/RadiusMath.h" #include "LightLimitFix.h" #include @@ -75,8 +76,8 @@ void InverseSquareLighting::ProcessLight(LightLimitFix::LightData& light, RE::BS light.radius = CalculateRadius(intensity, ShadowCasterManager::IsShadowLightType(bsLight), runtimeData->cutoffOverride, runtimeData->size); runtimeData->radius = light.radius; light.invRadius = 1.f / light.radius; - light.fadeZone = 1.f / (light.radius * std::clamp(FadeZoneBase * light.invRadius, 0.f, 1.f)); - light.sizeBias = ScaledUnitsSq * runtimeData->size * runtimeData->size * 0.5f; + light.fadeZone = 1.f / (light.radius * std::clamp(ISLMath::FadeZoneBase * light.invRadius, 0.f, 1.f)); + light.sizeBias = ISLMath::ScaledUnitsSq * runtimeData->size * runtimeData->size * 0.5f; // light.color *= intensity; light.fade = intensity; } else { @@ -89,24 +90,12 @@ void InverseSquareLighting::ProcessLight(LightLimitFix::LightData& light, RE::BS float InverseSquareLighting::CalculateRadius(const float intensity, const bool shadowCaster, const float cutoffOverride, const float size) { - float cutoff = shadowCaster ? DefaultShadowCasterCutoff : DefaultCutoff; - cutoff = cutoffOverride == 1.f ? cutoff : cutoffOverride; - const float radius = std::sqrt(ScaledUnitsSq * ((2 * intensity - cutoff * size * size) / (2 * cutoff))); - return isnan(radius) ? 1.f : radius; -} - -inline float InverseSquareLighting::SmoothStep(const float edge0, const float edge1, const float x) -{ - const float t = std::clamp((x - edge0) / (edge1 - edge0), 0.0f, 1.0f); - return t * t * (3.0f - 2.0f * t); + return ISLMath::CalculateRadius(intensity, shadowCaster, cutoffOverride, size); } float InverseSquareLighting::GetAttenuation(const float distance, const float radius, const float size) { - const float attenuation = ScaledUnitsSq / (distance * distance + ScaledUnitsSq * size * size / 2); - const float fadeZone = std::clamp(FadeZoneBase / radius, 0.0f, 1.0f); - const float fade = SmoothStep(0, radius * fadeZone, radius - distance); - return attenuation * fade; + return ISLMath::GetAttenuation(distance, radius, size); } float InverseSquareLighting::BSLight_GetLuminance::thunk(RE::BSLight* bsLight, RE::NiPoint3* targetPosition, RE::NiLight* refLight) diff --git a/src/Features/InverseSquareLighting.h b/src/Features/InverseSquareLighting.h index 70699c3ac3..89a321ba95 100644 --- a/src/Features/InverseSquareLighting.h +++ b/src/Features/InverseSquareLighting.h @@ -58,16 +58,8 @@ struct InverseSquareLighting : Feature private: LightEditor editor = LightEditor(); - static constexpr float DefaultCutoff = 0.05f; - static constexpr float DefaultShadowCasterCutoff = 0.022f; - - static constexpr float Scale = 0.8f; - static constexpr float MetresToUnits = 70.f; - static constexpr float MetresToUnitsSq = MetresToUnits * MetresToUnits; - static constexpr float ScaledUnitsSq = Scale * MetresToUnitsSq; - static constexpr float FadeZoneBase = 4.5f * Scale * MetresToUnits; + // Radius/attenuation constants and math live in InverseSquareLighting/RadiusMath.h + // (namespace ISLMath) so they can be unit-tested without the engine. static void SetExtLightData(RE::NiLight* niLight, const RE::TESObjectLIGH* ligh); - - static inline float SmoothStep(float edge0, float edge1, float x); }; \ No newline at end of file diff --git a/src/Features/InverseSquareLighting/RadiusMath.h b/src/Features/InverseSquareLighting/RadiusMath.h new file mode 100644 index 0000000000..3e1b2c7216 --- /dev/null +++ b/src/Features/InverseSquareLighting/RadiusMath.h @@ -0,0 +1,46 @@ +#pragma once + +#include +#include + +// Pure inverse-square-lighting radius / attenuation math, extracted from +// InverseSquareLighting so it can be unit-tested without the game/RE runtime. +// All inputs and outputs are plain floats -- no engine types -- so this header +// compiles standalone into the cpp_tests binary. InverseSquareLighting's static +// methods delegate here. +namespace ISLMath +{ + inline constexpr float DefaultCutoff = 0.05f; + inline constexpr float DefaultShadowCasterCutoff = 0.022f; + + inline constexpr float Scale = 0.8f; + inline constexpr float MetresToUnits = 70.f; + inline constexpr float MetresToUnitsSq = MetresToUnits * MetresToUnits; + inline constexpr float ScaledUnitsSq = Scale * MetresToUnitsSq; + inline constexpr float FadeZoneBase = 4.5f * Scale * MetresToUnits; + + inline float SmoothStep(float edge0, float edge1, float x) + { + const float t = std::clamp((x - edge0) / (edge1 - edge0), 0.0f, 1.0f); + return t * t * (3.0f - 2.0f * t); + } + + // Radius at which the inverse-square falloff reaches `cutoff`. A cutoffOverride + // of exactly 1.0 means "use the default"; anything else overrides it. NaN + // results (degenerate inputs) clamp to 1.0 so a light never gets a bad radius. + inline float CalculateRadius(float intensity, bool shadowCaster, float cutoffOverride, float size) + { + float cutoff = shadowCaster ? DefaultShadowCasterCutoff : DefaultCutoff; + cutoff = cutoffOverride == 1.f ? cutoff : cutoffOverride; + const float radius = std::sqrt(ScaledUnitsSq * ((2 * intensity - cutoff * size * size) / (2 * cutoff))); + return std::isnan(radius) ? 1.f : radius; + } + + inline float GetAttenuation(float distance, float radius, float size) + { + const float attenuation = ScaledUnitsSq / (distance * distance + ScaledUnitsSq * size * size / 2); + const float fadeZone = std::clamp(FadeZoneBase / radius, 0.0f, 1.0f); + const float fade = SmoothStep(0, radius * fadeZone, radius - distance); + return attenuation * fade; + } +} diff --git a/tests/cpp/CMakeLists.txt b/tests/cpp/CMakeLists.txt index a77dbe83bc..8a3cfd7328 100644 --- a/tests/cpp/CMakeLists.txt +++ b/tests/cpp/CMakeLists.txt @@ -49,6 +49,7 @@ add_executable(cpp_tests test_sphericalharmonics.cpp test_input.cpp test_stringutils.cpp + test_isl_radiusmath.cpp # Compile the unit-under-test directly into the test binary so we don't # depend on the plugin DLL build (which pulls in FFX/Streamline/etc.). "${CMAKE_SOURCE_DIR}/src/Utils/Subrect.cpp" diff --git a/tests/cpp/test_isl_radiusmath.cpp b/tests/cpp/test_isl_radiusmath.cpp new file mode 100644 index 0000000000..cd8fb471ee --- /dev/null +++ b/tests/cpp/test_isl_radiusmath.cpp @@ -0,0 +1,64 @@ +// Unit tests for inverse-square-lighting radius/attenuation math +// (src/Features/InverseSquareLighting/RadiusMath.h). Pure float math extracted +// from InverseSquareLighting so it tests without the game/RE runtime. + +#include "Features/InverseSquareLighting/RadiusMath.h" + +#include +#include + +using Catch::Approx; + +TEST_CASE("SmoothStep is a clamped Hermite ramp", "[isl]") +{ + REQUIRE(ISLMath::SmoothStep(0.0f, 1.0f, 0.0f) == Approx(0.0f)); + REQUIRE(ISLMath::SmoothStep(0.0f, 1.0f, 1.0f) == Approx(1.0f)); + REQUIRE(ISLMath::SmoothStep(0.0f, 1.0f, 0.5f) == Approx(0.5f)); // t^2(3-2t) at 0.5 + // Clamps outside [edge0, edge1]. + REQUIRE(ISLMath::SmoothStep(0.0f, 1.0f, -1.0f) == Approx(0.0f)); + REQUIRE(ISLMath::SmoothStep(0.0f, 1.0f, 2.0f) == Approx(1.0f)); +} + +TEST_CASE("CalculateRadius matches the closed form for default cutoff", "[isl]") +{ + // cutoffOverride == 1 -> default cutoff (0.05 for non-shadow); size 0. + // radius = sqrt(ScaledUnitsSq * (2*intensity / (2*0.05))) = sqrt(3920 * 20) = 280. + REQUIRE(ISLMath::CalculateRadius(1.0f, false, 1.0f, 0.0f) == Approx(280.0f)); +} + +TEST_CASE("Shadow casters get a larger radius (smaller cutoff)", "[isl]") +{ + const float normal = ISLMath::CalculateRadius(1.0f, false, 1.0f, 0.0f); + const float shadow = ISLMath::CalculateRadius(1.0f, true, 1.0f, 0.0f); + REQUIRE(shadow > normal); +} + +TEST_CASE("cutoffOverride replaces the default regardless of shadow flag", "[isl]") +{ + // Override of 0.05 forces the non-shadow default value even for a shadow caster. + REQUIRE(ISLMath::CalculateRadius(1.0f, true, 0.05f, 0.0f) == Approx(280.0f)); +} + +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("GetAttenuation peaks at the source and vanishes past the radius", "[isl]") +{ + // distance 0, radius 280, size 1: attenuation = 3920/(0 + 3920/2) = 2.0; + // fade is 1 at the source. + REQUIRE(ISLMath::GetAttenuation(0.0f, 280.0f, 1.0f) == Approx(2.0f)); + // Beyond the radius the SmoothStep fade clamps to zero. + REQUIRE(ISLMath::GetAttenuation(300.0f, 280.0f, 1.0f) == Approx(0.0f)); +} + +TEST_CASE("GetAttenuation decreases monotonically with distance in range", "[isl]") +{ + // 'near'/'far' are legacy Windows.h macros, so use distinct names. + const float attNear = ISLMath::GetAttenuation(10.0f, 280.0f, 1.0f); + const float attFar = ISLMath::GetAttenuation(150.0f, 280.0f, 1.0f); + REQUIRE(attNear > attFar); + REQUIRE(attFar > 0.0f); +} From f68a5b3b6ef684e7336f6a38903684c192595343 Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Thu, 28 May 2026 17:54:45 -0700 Subject: [PATCH 2/7] build: add /bigobj to the plugin for MSVC 14.50 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) --- cmake/XSEPlugin.cmake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmake/XSEPlugin.cmake b/cmake/XSEPlugin.cmake index d23742ded0..58c410922a 100644 --- a/cmake/XSEPlugin.cmake +++ b/cmake/XSEPlugin.cmake @@ -65,6 +65,8 @@ if(CMAKE_GENERATOR MATCHES "Visual Studio") "${PROJECT_NAME}" PRIVATE /MP + /bigobj # ShadowCasterManager.cpp (exprtk-heavy) exceeds the object-file + # section limit under MSVC 14.50 without this (C1128). /W4 /WX /permissive- From d28c4a53f8e44b0a67f3bb5d6a212b47ccfc066f Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Thu, 28 May 2026 18:07:09 -0700 Subject: [PATCH 3/7] refactor(slf): extract shadow-pointer/percentile/sanitize math for testing 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) --- src/Features/LightLimitFix.cpp | 3 +- src/Features/LightLimitFix/SettingsSanitize.h | 19 ++++++++ .../LightLimitFix/ShadowCasterManager.cpp | 9 +--- .../LightLimitFix/ShadowCasterManager.h | 6 +-- src/Features/LightLimitFix/ShadowCasterMath.h | 36 ++++++++++++++ tests/cpp/CMakeLists.txt | 2 + tests/cpp/test_llf_sanitize.cpp | 31 ++++++++++++ tests/cpp/test_shadowcaster_math.cpp | 47 +++++++++++++++++++ 8 files changed, 141 insertions(+), 12 deletions(-) create mode 100644 src/Features/LightLimitFix/SettingsSanitize.h create mode 100644 src/Features/LightLimitFix/ShadowCasterMath.h create mode 100644 tests/cpp/test_llf_sanitize.cpp create mode 100644 tests/cpp/test_shadowcaster_math.cpp diff --git a/src/Features/LightLimitFix.cpp b/src/Features/LightLimitFix.cpp index 08edec4ade..8c55f38458 100644 --- a/src/Features/LightLimitFix.cpp +++ b/src/Features/LightLimitFix.cpp @@ -1,5 +1,6 @@ #include "LightLimitFix.h" #include "Features/InverseSquareLighting/Common.h" +#include "Features/LightLimitFix/SettingsSanitize.h" #include "Globals.h" #include "InverseSquareLighting.h" #include "LinearLighting.h" @@ -380,7 +381,7 @@ LightLimitFix::PerFrame LightLimitFix::GetCommonBufferData() // so reject non-finite values explicitly first; fall back to the lower // bound on NaN/inf to produce degraded but stable behavior. auto sanitizeFloat = [](float v, float lo, float hi) { - return std::isfinite(v) ? std::clamp(v, lo, hi) : lo; + return LightLimitFixSanitize::SanitizeFloat(v, lo, hi); }; PerFrame perFrame{}; diff --git a/src/Features/LightLimitFix/SettingsSanitize.h b/src/Features/LightLimitFix/SettingsSanitize.h new file mode 100644 index 0000000000..b476c9a180 --- /dev/null +++ b/src/Features/LightLimitFix/SettingsSanitize.h @@ -0,0 +1,19 @@ +#pragma once + +#include +#include + +// Pure settings-sanitization helper extracted from LightLimitFix so it can be +// unit-tested without the game/RE runtime. +namespace LightLimitFixSanitize +{ + // 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. + inline float SanitizeFloat(float v, float lo, float hi) + { + return std::isfinite(v) ? std::clamp(v, lo, hi) : lo; + } +} diff --git a/src/Features/LightLimitFix/ShadowCasterManager.cpp b/src/Features/LightLimitFix/ShadowCasterManager.cpp index 24978fe466..c417077260 100644 --- a/src/Features/LightLimitFix/ShadowCasterManager.cpp +++ b/src/Features/LightLimitFix/ShadowCasterManager.cpp @@ -241,14 +241,7 @@ namespace ShadowCasterManager static float ComputeFrameTimePercentile90() { - if (s_ftCount == 0) - return 16.67f; // fallback: 60 fps target - const int n = std::min(s_ftCount, kFrameWindow); - float tmp[kFrameWindow]; - std::copy(s_ftRing, s_ftRing + n, tmp); - const int idx = static_cast(n * 0.9f); - std::nth_element(tmp, tmp + idx, tmp + n); - return tmp[idx]; + return FrameTimePercentile90(s_ftRing, s_ftCount); } // Maximum ShadowLightCount the installed infrastructure supports. diff --git a/src/Features/LightLimitFix/ShadowCasterManager.h b/src/Features/LightLimitFix/ShadowCasterManager.h index 6230b1c870..2680e4d576 100644 --- a/src/Features/LightLimitFix/ShadowCasterManager.h +++ b/src/Features/LightLimitFix/ShadowCasterManager.h @@ -21,6 +21,8 @@ #include "RE/B/BSShadowLight.h" #include "RE/S/ShadowSceneNode.h" +#include "Features/LightLimitFix/ShadowCasterMath.h" + struct ImVec4; namespace ShadowCasterManager @@ -92,10 +94,8 @@ namespace ShadowCasterManager std::uint32_t idx = 0; while (idx < maxIdx) { RE::BSShadowLight* light = accum[idx]; - if (!light) - break; const auto raw = reinterpret_cast(light); - if (raw >= 0x0000800000000000ull || (raw & 0x7) != 0) + if (!IsPlausibleShadowLightPtr(raw)) // null / misaligned / non-canonical break; fn(light); const std::uint32_t step = light->shadowMapCount; diff --git a/src/Features/LightLimitFix/ShadowCasterMath.h b/src/Features/LightLimitFix/ShadowCasterMath.h new file mode 100644 index 0000000000..f95353bf90 --- /dev/null +++ b/src/Features/LightLimitFix/ShadowCasterMath.h @@ -0,0 +1,36 @@ +#pragma once + +#include +#include + +// Pure helpers extracted from ShadowCasterManager so they can be unit-tested +// without the game/RE runtime. No engine types in any signature. +namespace ShadowCasterManager +{ + // A shadow-light accumulator slot can hold heap garbage between our prepass + // and the engine's read. Treat a pointer as plausible only if it is + // non-null, inside the x64 user-mode canonical range, and 8-byte aligned + // (BSShadowLight is pointer-aligned). ForEachShadowLight stops iterating at + // the first implausible entry rather than dereferencing garbage. + inline bool IsPlausibleShadowLightPtr(std::uintptr_t raw) noexcept + { + return raw != 0 && raw < 0x0000800000000000ull && (raw & 0x7) == 0; + } + + // 90th-percentile of the most-recent min(count, Window) frame-time samples + // in `ring`. Percentile is order-independent, so the first `n` entries are + // sampled directly (ring head/wraparound doesn't matter). Returns the 60fps + // fallback (16.67 ms) before any samples exist. + template + inline float FrameTimePercentile90(const float (&ring)[Window], int count) + { + if (count == 0) + return 16.67f; + const int n = std::min(count, Window); + float tmp[Window]; + std::copy(ring, ring + n, tmp); + const int idx = static_cast(n * 0.9f); + std::nth_element(tmp, tmp + idx, tmp + n); + return tmp[idx]; + } +} diff --git a/tests/cpp/CMakeLists.txt b/tests/cpp/CMakeLists.txt index 8a3cfd7328..c57b60168e 100644 --- a/tests/cpp/CMakeLists.txt +++ b/tests/cpp/CMakeLists.txt @@ -50,6 +50,8 @@ add_executable(cpp_tests test_input.cpp test_stringutils.cpp test_isl_radiusmath.cpp + test_shadowcaster_math.cpp + test_llf_sanitize.cpp # Compile the unit-under-test directly into the test binary so we don't # depend on the plugin DLL build (which pulls in FFX/Streamline/etc.). "${CMAKE_SOURCE_DIR}/src/Utils/Subrect.cpp" diff --git a/tests/cpp/test_llf_sanitize.cpp b/tests/cpp/test_llf_sanitize.cpp new file mode 100644 index 0000000000..191bb4bb0f --- /dev/null +++ b/tests/cpp/test_llf_sanitize.cpp @@ -0,0 +1,31 @@ +// Unit tests for the LightLimitFix settings sanitizer +// (src/Features/LightLimitFix/SettingsSanitize.h). + +#include "Features/LightLimitFix/SettingsSanitize.h" + +#include +#include + +#include +#include + +using Catch::Approx; +using LightLimitFixSanitize::SanitizeFloat; + +TEST_CASE("SanitizeFloat clamps in-range, low, and high inputs", "[llf]") +{ + REQUIRE(SanitizeFloat(0.5f, 0.0f, 1.0f) == Approx(0.5f)); + REQUIRE(SanitizeFloat(-1.0f, 0.0f, 1.0f) == Approx(0.0f)); + REQUIRE(SanitizeFloat(2.0f, 0.0f, 1.0f) == Approx(1.0f)); + REQUIRE(SanitizeFloat(64.0f, 64.0f, 4096.0f) == Approx(64.0f)); // exact lower bound + REQUIRE(SanitizeFloat(4096.0f, 64.0f, 4096.0f) == Approx(4096.0f)); // exact upper bound +} + +TEST_CASE("SanitizeFloat falls back to the lower bound on non-finite input", "[llf]") +{ + const float nan = std::numeric_limits::quiet_NaN(); + const float inf = std::numeric_limits::infinity(); + REQUIRE(SanitizeFloat(nan, 0.5f, 8.0f) == Approx(0.5f)); + REQUIRE(SanitizeFloat(inf, 0.5f, 8.0f) == Approx(0.5f)); + REQUIRE(SanitizeFloat(-inf, 0.5f, 8.0f) == Approx(0.5f)); +} diff --git a/tests/cpp/test_shadowcaster_math.cpp b/tests/cpp/test_shadowcaster_math.cpp new file mode 100644 index 0000000000..dd35420d90 --- /dev/null +++ b/tests/cpp/test_shadowcaster_math.cpp @@ -0,0 +1,47 @@ +// Unit tests for ShadowCasterManager pure helpers +// (src/Features/LightLimitFix/ShadowCasterMath.h): the shadow-light pointer +// plausibility check and the frame-time 90th-percentile. + +#include "Features/LightLimitFix/ShadowCasterMath.h" + +#include +#include + +using Catch::Approx; +using ShadowCasterManager::FrameTimePercentile90; +using ShadowCasterManager::IsPlausibleShadowLightPtr; + +TEST_CASE("IsPlausibleShadowLightPtr rejects null, misaligned, and non-canonical", "[scm]") +{ + REQUIRE_FALSE(IsPlausibleShadowLightPtr(0)); // null + REQUIRE(IsPlausibleShadowLightPtr(0x8)); // aligned, in range + REQUIRE(IsPlausibleShadowLightPtr(0x00007FFFFFFFFFF8ull)); // top of user-mode range + REQUIRE_FALSE(IsPlausibleShadowLightPtr(0x0000800000000000ull)); // first non-canonical + REQUIRE_FALSE(IsPlausibleShadowLightPtr(0xFFFFF80000000000ull)); // kernel-space garbage + + // Any non-8-byte alignment is rejected. + for (std::uintptr_t off = 1; off < 8; ++off) + REQUIRE_FALSE(IsPlausibleShadowLightPtr(0x1000 + off)); + REQUIRE(IsPlausibleShadowLightPtr(0x1000)); +} + +TEST_CASE("FrameTimePercentile90 returns the 60fps fallback with no samples", "[scm]") +{ + float ring[8]{}; + REQUIRE(FrameTimePercentile90(ring, 0) == Approx(16.67f)); +} + +TEST_CASE("FrameTimePercentile90 picks the P90 element", "[scm]") +{ + // 10 samples 1..10: idx = int(10*0.9) = 9 -> the largest sorted element. + float ring[10] = { 5, 2, 9, 1, 7, 3, 10, 4, 8, 6 }; + REQUIRE(FrameTimePercentile90(ring, 10) == Approx(10.0f)); +} + +TEST_CASE("FrameTimePercentile90 honors count below the window size", "[scm]") +{ + // Only the first 5 entries are valid; trailing slots must not be sampled. + float ring[10] = { 10, 20, 30, 40, 50, 999, 999, 999, 999, 999 }; + // n = 5, idx = int(5*0.9) = 4 -> the largest of {10..50} = 50. + REQUIRE(FrameTimePercentile90(ring, 5) == Approx(50.0f)); +} From 64d45371322b7e2208cc6a56dae3c43e0a9f0a5e Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Thu, 28 May 2026 19:05:54 -0700 Subject: [PATCH 4/7] build(slf): drop /bigobj from plugin target 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) --- cmake/XSEPlugin.cmake | 2 -- src/Features/InverseSquareLighting.h | 3 +-- src/Features/InverseSquareLighting/RadiusMath.h | 7 ++----- src/Features/LightLimitFix/ShadowCasterMath.h | 2 +- tests/cpp/test_isl_radiusmath.cpp | 4 +--- 5 files changed, 5 insertions(+), 13 deletions(-) diff --git a/cmake/XSEPlugin.cmake b/cmake/XSEPlugin.cmake index 58c410922a..d23742ded0 100644 --- a/cmake/XSEPlugin.cmake +++ b/cmake/XSEPlugin.cmake @@ -65,8 +65,6 @@ if(CMAKE_GENERATOR MATCHES "Visual Studio") "${PROJECT_NAME}" PRIVATE /MP - /bigobj # ShadowCasterManager.cpp (exprtk-heavy) exceeds the object-file - # section limit under MSVC 14.50 without this (C1128). /W4 /WX /permissive- diff --git a/src/Features/InverseSquareLighting.h b/src/Features/InverseSquareLighting.h index 89a321ba95..81946aa17c 100644 --- a/src/Features/InverseSquareLighting.h +++ b/src/Features/InverseSquareLighting.h @@ -58,8 +58,7 @@ struct InverseSquareLighting : Feature private: LightEditor editor = LightEditor(); - // Radius/attenuation constants and math live in InverseSquareLighting/RadiusMath.h - // (namespace ISLMath) so they can be unit-tested without the engine. + // Constants + math live in RadiusMath.h (ISLMath) for unit testing. static void SetExtLightData(RE::NiLight* niLight, const RE::TESObjectLIGH* ligh); }; \ No newline at end of file diff --git a/src/Features/InverseSquareLighting/RadiusMath.h b/src/Features/InverseSquareLighting/RadiusMath.h index 3e1b2c7216..60ba6a02c2 100644 --- a/src/Features/InverseSquareLighting/RadiusMath.h +++ b/src/Features/InverseSquareLighting/RadiusMath.h @@ -3,11 +3,8 @@ #include #include -// Pure inverse-square-lighting radius / attenuation math, extracted from -// InverseSquareLighting so it can be unit-tested without the game/RE runtime. -// All inputs and outputs are plain floats -- no engine types -- so this header -// compiles standalone into the cpp_tests binary. InverseSquareLighting's static -// methods delegate here. +// Pure ISL radius/attenuation math, extracted so it can be unit-tested without +// the game/RE runtime. InverseSquareLighting's static methods delegate here. namespace ISLMath { inline constexpr float DefaultCutoff = 0.05f; diff --git a/src/Features/LightLimitFix/ShadowCasterMath.h b/src/Features/LightLimitFix/ShadowCasterMath.h index f95353bf90..da8bb8e077 100644 --- a/src/Features/LightLimitFix/ShadowCasterMath.h +++ b/src/Features/LightLimitFix/ShadowCasterMath.h @@ -4,7 +4,7 @@ #include // Pure helpers extracted from ShadowCasterManager so they can be unit-tested -// without the game/RE runtime. No engine types in any signature. +// without the game/RE runtime. namespace ShadowCasterManager { // A shadow-light accumulator slot can hold heap garbage between our prepass diff --git a/tests/cpp/test_isl_radiusmath.cpp b/tests/cpp/test_isl_radiusmath.cpp index cd8fb471ee..67b2bf8a59 100644 --- a/tests/cpp/test_isl_radiusmath.cpp +++ b/tests/cpp/test_isl_radiusmath.cpp @@ -1,6 +1,4 @@ -// Unit tests for inverse-square-lighting radius/attenuation math -// (src/Features/InverseSquareLighting/RadiusMath.h). Pure float math extracted -// from InverseSquareLighting so it tests without the game/RE runtime. +// Unit tests for ISL radius/attenuation math (RadiusMath.h). #include "Features/InverseSquareLighting/RadiusMath.h" From 5499cf234a4b5efc957b65eef044124276b6ce3c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 30 May 2026 19:50:54 +0000 Subject: [PATCH 5/7] =?UTF-8?q?style:=20=F0=9F=8E=A8=20apply=20pre-commit.?= =?UTF-8?q?ci=20formatting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. --- tests/cpp/test_llf_sanitize.cpp | 2 +- tests/cpp/test_shadowcaster_math.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/cpp/test_llf_sanitize.cpp b/tests/cpp/test_llf_sanitize.cpp index 191bb4bb0f..b3a1603313 100644 --- a/tests/cpp/test_llf_sanitize.cpp +++ b/tests/cpp/test_llf_sanitize.cpp @@ -17,7 +17,7 @@ TEST_CASE("SanitizeFloat clamps in-range, low, and high inputs", "[llf]") REQUIRE(SanitizeFloat(0.5f, 0.0f, 1.0f) == Approx(0.5f)); REQUIRE(SanitizeFloat(-1.0f, 0.0f, 1.0f) == Approx(0.0f)); REQUIRE(SanitizeFloat(2.0f, 0.0f, 1.0f) == Approx(1.0f)); - REQUIRE(SanitizeFloat(64.0f, 64.0f, 4096.0f) == Approx(64.0f)); // exact lower bound + REQUIRE(SanitizeFloat(64.0f, 64.0f, 4096.0f) == Approx(64.0f)); // exact lower bound REQUIRE(SanitizeFloat(4096.0f, 64.0f, 4096.0f) == Approx(4096.0f)); // exact upper bound } diff --git a/tests/cpp/test_shadowcaster_math.cpp b/tests/cpp/test_shadowcaster_math.cpp index dd35420d90..d8bb3e5f9b 100644 --- a/tests/cpp/test_shadowcaster_math.cpp +++ b/tests/cpp/test_shadowcaster_math.cpp @@ -14,10 +14,10 @@ using ShadowCasterManager::IsPlausibleShadowLightPtr; TEST_CASE("IsPlausibleShadowLightPtr rejects null, misaligned, and non-canonical", "[scm]") { REQUIRE_FALSE(IsPlausibleShadowLightPtr(0)); // null - REQUIRE(IsPlausibleShadowLightPtr(0x8)); // aligned, in range - REQUIRE(IsPlausibleShadowLightPtr(0x00007FFFFFFFFFF8ull)); // top of user-mode range - REQUIRE_FALSE(IsPlausibleShadowLightPtr(0x0000800000000000ull)); // first non-canonical - REQUIRE_FALSE(IsPlausibleShadowLightPtr(0xFFFFF80000000000ull)); // kernel-space garbage + REQUIRE(IsPlausibleShadowLightPtr(0x8)); // aligned, in range + REQUIRE(IsPlausibleShadowLightPtr(0x00007FFFFFFFFFF8ull)); // top of user-mode range + REQUIRE_FALSE(IsPlausibleShadowLightPtr(0x0000800000000000ull)); // first non-canonical + REQUIRE_FALSE(IsPlausibleShadowLightPtr(0xFFFFF80000000000ull)); // kernel-space garbage // Any non-8-byte alignment is rejected. for (std::uintptr_t off = 1; off < 8; ++off) From 8ec7a49704d4c17c174f1d783520f65c64ac6913 Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Sat, 30 May 2026 12:52:12 -0700 Subject: [PATCH 6/7] fix(llf,isl): harden extracted helpers against degenerate inputs 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) --- .../InverseSquareLighting/RadiusMath.h | 9 ++++++--- src/Features/LightLimitFix/ShadowCasterMath.h | 19 ++++++++++++------- tests/cpp/test_isl_radiusmath.cpp | 13 +++++++++++-- tests/cpp/test_shadowcaster_math.cpp | 15 ++++++++++----- 4 files changed, 39 insertions(+), 17 deletions(-) diff --git a/src/Features/InverseSquareLighting/RadiusMath.h b/src/Features/InverseSquareLighting/RadiusMath.h index 60ba6a02c2..071736c414 100644 --- a/src/Features/InverseSquareLighting/RadiusMath.h +++ b/src/Features/InverseSquareLighting/RadiusMath.h @@ -23,14 +23,17 @@ namespace ISLMath } // Radius at which the inverse-square falloff reaches `cutoff`. A cutoffOverride - // of exactly 1.0 means "use the default"; anything else overrides it. NaN - // results (degenerate inputs) clamp to 1.0 so a light never gets a bad radius. + // of exactly 1.0 means "use the default"; anything else overrides it. Any + // degenerate result clamps to 1.0 so a light never gets a bad radius: a + // negative sqrt argument yields NaN, an exact-zero argument yields 0 (callers + // divide by radius, so 0 would drive a 0/0 SmoothStep), and a zero + // cutoffOverride yields +inf -- a finite-and-positive check guards all three. inline float CalculateRadius(float intensity, bool shadowCaster, float cutoffOverride, float size) { float cutoff = shadowCaster ? DefaultShadowCasterCutoff : DefaultCutoff; cutoff = cutoffOverride == 1.f ? cutoff : cutoffOverride; const float radius = std::sqrt(ScaledUnitsSq * ((2 * intensity - cutoff * size * size) / (2 * cutoff))); - return std::isnan(radius) ? 1.f : radius; + return std::isfinite(radius) && radius > 0.0f ? radius : 1.0f; } inline float GetAttenuation(float distance, float radius, float size) diff --git a/src/Features/LightLimitFix/ShadowCasterMath.h b/src/Features/LightLimitFix/ShadowCasterMath.h index da8bb8e077..a158205ac9 100644 --- a/src/Features/LightLimitFix/ShadowCasterMath.h +++ b/src/Features/LightLimitFix/ShadowCasterMath.h @@ -8,23 +8,28 @@ namespace ShadowCasterManager { // A shadow-light accumulator slot can hold heap garbage between our prepass - // and the engine's read. Treat a pointer as plausible only if it is - // non-null, inside the x64 user-mode canonical range, and 8-byte aligned - // (BSShadowLight is pointer-aligned). ForEachShadowLight stops iterating at - // the first implausible entry rather than dereferencing garbage. + // and the engine's read. Treat a pointer as plausible only if it is at or + // above the low 64 KiB (the Windows x64 null-guard region is never a valid + // user mapping, so a near-null garbage value like 0x8 must be rejected -- + // dereferencing it is a guaranteed AV/CTD), inside the x64 user-mode + // canonical range, and 8-byte aligned (BSShadowLight is pointer-aligned). + // ForEachShadowLight stops iterating at the first implausible entry rather + // than dereferencing garbage. inline bool IsPlausibleShadowLightPtr(std::uintptr_t raw) noexcept { - return raw != 0 && raw < 0x0000800000000000ull && (raw & 0x7) == 0; + return raw >= 0x10000ull && raw < 0x0000800000000000ull && (raw & 0x7) == 0; } // 90th-percentile of the most-recent min(count, Window) frame-time samples // in `ring`. Percentile is order-independent, so the first `n` entries are // sampled directly (ring head/wraparound doesn't matter). Returns the 60fps - // fallback (16.67 ms) before any samples exist. + // fallback (16.67 ms) before any samples exist. A non-positive count (no + // samples, or a corrupt/negative value) takes the fallback -- a negative n + // would otherwise drive std::copy / std::nth_element out of bounds. template inline float FrameTimePercentile90(const float (&ring)[Window], int count) { - if (count == 0) + if (count <= 0) return 16.67f; const int n = std::min(count, Window); float tmp[Window]; diff --git a/tests/cpp/test_isl_radiusmath.cpp b/tests/cpp/test_isl_radiusmath.cpp index 67b2bf8a59..bcce5ddb74 100644 --- a/tests/cpp/test_isl_radiusmath.cpp +++ b/tests/cpp/test_isl_radiusmath.cpp @@ -37,10 +37,19 @@ TEST_CASE("cutoffOverride replaces the default regardless of shadow flag", "[isl REQUIRE(ISLMath::CalculateRadius(1.0f, true, 0.05f, 0.0f) == Approx(280.0f)); } -TEST_CASE("CalculateRadius clamps a NaN (negative-sqrt) result to 1", "[isl]") +TEST_CASE("CalculateRadius clamps degenerate results to 1", "[isl]") { - // intensity 0 with a large size makes the sqrt argument negative -> NaN -> 1. + // NaN: intensity 0 with a large size makes the sqrt argument negative. REQUIRE(ISLMath::CalculateRadius(0.0f, false, 1.0f, 10.0f) == Approx(1.0f)); + // Exact zero: 2*intensity == cutoff*size^2 (0.05 * 2^2 = 0.2, intensity 0.1) + // makes the sqrt argument 0. A 0 radius would drive a 0/0 SmoothStep downstream. + REQUIRE(ISLMath::CalculateRadius(0.1f, false, 1.0f, 2.0f) == Approx(1.0f)); + // +inf: a zero cutoffOverride divides by 2*cutoff == 0. isnan() alone misses + // this (sqrt(+inf) is +inf, not NaN); the finite check catches it. The cutoff + // is read through a volatile so MSVC can't constant-fold the literal /0 into a + // compile-time C4723 -- at runtime the float divide yields +inf as intended. + volatile float zeroCutoff = 0.0f; + REQUIRE(ISLMath::CalculateRadius(1.0f, false, zeroCutoff, 0.0f) == Approx(1.0f)); } TEST_CASE("GetAttenuation peaks at the source and vanishes past the radius", "[isl]") diff --git a/tests/cpp/test_shadowcaster_math.cpp b/tests/cpp/test_shadowcaster_math.cpp index d8bb3e5f9b..43d493d085 100644 --- a/tests/cpp/test_shadowcaster_math.cpp +++ b/tests/cpp/test_shadowcaster_math.cpp @@ -11,24 +11,29 @@ using Catch::Approx; using ShadowCasterManager::FrameTimePercentile90; using ShadowCasterManager::IsPlausibleShadowLightPtr; -TEST_CASE("IsPlausibleShadowLightPtr rejects null, misaligned, and non-canonical", "[scm]") +TEST_CASE("IsPlausibleShadowLightPtr rejects null, near-null, misaligned, and non-canonical", "[scm]") { REQUIRE_FALSE(IsPlausibleShadowLightPtr(0)); // null - REQUIRE(IsPlausibleShadowLightPtr(0x8)); // aligned, in range + REQUIRE_FALSE(IsPlausibleShadowLightPtr(0x8)); // near-null: below the 64 KiB floor + REQUIRE_FALSE(IsPlausibleShadowLightPtr(0xFFF8ull)); // aligned but still below the floor + REQUIRE(IsPlausibleShadowLightPtr(0x10000ull)); // at the floor, aligned -> plausible REQUIRE(IsPlausibleShadowLightPtr(0x00007FFFFFFFFFF8ull)); // top of user-mode range REQUIRE_FALSE(IsPlausibleShadowLightPtr(0x0000800000000000ull)); // first non-canonical REQUIRE_FALSE(IsPlausibleShadowLightPtr(0xFFFFF80000000000ull)); // kernel-space garbage - // Any non-8-byte alignment is rejected. + // Any non-8-byte alignment is rejected (use a base above the floor so the + // alignment check is what fails, not the minimum-address check). for (std::uintptr_t off = 1; off < 8; ++off) - REQUIRE_FALSE(IsPlausibleShadowLightPtr(0x1000 + off)); - REQUIRE(IsPlausibleShadowLightPtr(0x1000)); + REQUIRE_FALSE(IsPlausibleShadowLightPtr(0x10000 + off)); } TEST_CASE("FrameTimePercentile90 returns the 60fps fallback with no samples", "[scm]") { float ring[8]{}; REQUIRE(FrameTimePercentile90(ring, 0) == Approx(16.67f)); + // A negative count (corruption / future refactor) must not drive a negative + // n into std::copy / std::nth_element -- it takes the fallback too. + REQUIRE(FrameTimePercentile90(ring, -1) == Approx(16.67f)); } TEST_CASE("FrameTimePercentile90 picks the P90 element", "[scm]") From 688c96078abfe4fe004764dc43ba9ae16bb5126c Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Sat, 30 May 2026 12:58:02 -0700 Subject: [PATCH 7/7] test(scm): cover count-above-window; doc SanitizeFloat precondition 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) --- src/Features/LightLimitFix/SettingsSanitize.h | 5 +++++ tests/cpp/test_shadowcaster_math.cpp | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/src/Features/LightLimitFix/SettingsSanitize.h b/src/Features/LightLimitFix/SettingsSanitize.h index b476c9a180..e158d33174 100644 --- a/src/Features/LightLimitFix/SettingsSanitize.h +++ b/src/Features/LightLimitFix/SettingsSanitize.h @@ -12,6 +12,11 @@ namespace LightLimitFixSanitize // 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 are finite with lo <= hi. Only the value is + // untrusted; the bounds are compile-time constants at every call site (the + // feature's fixed setting ranges), so they are not re-validated here -- a + // NaN lo would defeat the fallback and hi < lo is std::clamp UB. inline float SanitizeFloat(float v, float lo, float hi) { return std::isfinite(v) ? std::clamp(v, lo, hi) : lo; diff --git a/tests/cpp/test_shadowcaster_math.cpp b/tests/cpp/test_shadowcaster_math.cpp index 43d493d085..47b49b2e3c 100644 --- a/tests/cpp/test_shadowcaster_math.cpp +++ b/tests/cpp/test_shadowcaster_math.cpp @@ -50,3 +50,11 @@ TEST_CASE("FrameTimePercentile90 honors count below the window size", "[scm]") // n = 5, idx = int(5*0.9) = 4 -> the largest of {10..50} = 50. REQUIRE(FrameTimePercentile90(ring, 5) == Approx(50.0f)); } + +TEST_CASE("FrameTimePercentile90 clamps count above the window size", "[scm]") +{ + // count > Window: std::min clamps n to Window (10) so the copy stays in + // bounds; all slots are sampled. Pins the clamp against a regression. + float ring[10] = { 5, 2, 9, 1, 7, 3, 10, 4, 8, 6 }; + REQUIRE(FrameTimePercentile90(ring, 99) == Approx(10.0f)); +}