test: add hlsl unit tests#1653
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a shader unit-test framework (runtime discovery, Catch2 runner), ~15 new HLSL test suites, CMake options/targets to build/run shader tests, Windows Graphics Tools detection, CI workflow/actions for shader-unit-tests, PBR math extraction into PBRMath.hlsli, and .gitignore/test artifact rules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/CI
participant CMake as CMake (configure)
participant Build as Build System
participant TestExe as shader_tests
participant Shaders as Shaders/Tests (HLSL)
participant CI as GitHub Actions
Dev->>CMake: cmake --preset ALL -DBUILD_SHADER_TESTS=ON
CMake->>Build: generate & build targets (shader_tests, run_shader_tests)
CI->>Build: build shader_tests
Build->>Shaders: copy / discover Test*.hlsl assets
Build->>TestExe: link ShaderTestFramework + Catch2
CI->>TestExe: execute shader_tests (--reporter compact)
TestExe->>Shaders: runtime discover tests (parse HLSL files)
TestExe->>TestExe: for each test -> run via ShaderTestFixture
TestExe-->>CI: report pass/fail results and exit code
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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.
Pull request overview
This PR adds comprehensive GPU-executed unit tests for HLSL shader code using ShaderTestFramework and Catch2. The tests validate math functions, color space conversions, BRDF calculations, and other shader utilities, providing 63 test cases with 268 GPU assertions across 7 shader modules. While marked "Do not merge," this establishes a solid testing foundation for the shader codebase.
Key changes:
- New test infrastructure with unified macro system and runtime test discovery
- 63 GPU-executed unit tests covering Math, Color, BRDF, FastMath, DisplayMapping, GBuffer, LightingCommon, and Random modules
- CMake integration with automatic Graphics Tools detection
- CI/CD workflow for automatic test execution on shader changes
- Production shader fixes: operator precedence clarification in Random.hlsli and half-precision literal consistency in GBuffer.hlsli
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/shaders/test_common.h | Common utilities for shader tests (executable path, shader mappings, fixture configuration) |
| tests/shaders/test_helpers_unified.h | Unified macro system for shader test declarations with simple, parallel, and batch variants |
| tests/shaders/runtime_test_discovery.h | Runtime HLSL test discovery by scanning for numthreads annotations |
| tests/shaders/runtime_discovered_tests.cpp | Auto-discovery test runner that finds and executes all HLSL tests |
| tests/shaders/minimal_test.cpp | Explicit main() function to work around CMake 4.0/Catch2 linking issues |
| tests/shaders/test_*.cpp | C++ test harnesses for Math, Color, BRDF, FastMath, DisplayMapping, GBuffer, LightingCommon, Random |
| package/Shaders/Tests/Test*.hlsl | HLSL test implementations with GPU-executed assertions |
| package/Shaders/Common/Random.hlsli | Fixed operator precedence with explicit parentheses, clarified magic constants, added default case |
| package/Shaders/Common/GBuffer.hlsli | Consistent half-precision literals (e.g., 1.0h instead of 1.0) |
| tests/shaders/CMakeLists.txt | Test build configuration with ShaderTestFramework/Catch2 integration |
| cmake/DetectGraphicsTools.cmake | Detects Windows Graphics Tools for D3D12 debug layer support |
| CMakeLists.txt | Added BUILD_SHADER_TESTS option and test subdirectory integration |
| .github/workflows/shader-unit-tests.yaml | CI workflow for shader tests on HLSL file changes |
| tests/shaders/README.md | Comprehensive documentation for test usage and troubleshooting |
| .gitignore | Excluded shader test build artifacts |
| tests/shaders/.gitignore | Excluded test-specific artifacts |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
cmake/DetectGraphicsTools.cmake (1)
21-21: Remove unused variable.The
DXGI_DEBUG_DLLvariable is set but never used in the detection logic. Consider removing it or implementing additional validation using this file if needed.🔎 Proposed fix
- set(DXGI_DEBUG_DLL "${WINDOWS_DIR}/System32/dxgidebug.dll") -tests/shaders/runtime_discovered_tests.cpp (1)
14-14: Consider clarifying the minimum expected test count.The assertion
tests.size() > 0ensures at least one test is found, but the comment "Should find at least some tests" is vague. Consider either:
- Adding a more specific minimum count if known (e.g.,
REQUIRE(tests.size() >= 50)based on actual test count)- Or improving the failure message to aid debugging when no tests are discovered
This helps catch issues where test discovery silently fails or test files are misconfigured.
🔎 Suggested improvement
- REQUIRE(tests.size() > 0); // Should find at least some tests + REQUIRE_MESSAGE(tests.size() > 0, "No HLSL tests discovered - check test file paths and SHADER_TEST registrations");Or if you know the expected minimum:
- REQUIRE(tests.size() > 0); // Should find at least some tests + // We expect at least 50+ tests across all modules (BRDF, Math, Color, etc.) + REQUIRE(tests.size() >= 50);.github/workflows/shader-unit-tests.yaml (2)
56-68: Graphics Tools requirement may be too strict for development workflows.The workflow fails (exit 1) if Graphics Tools are not detected. While appropriate for CI validation, this could prevent developers from running tests locally if they haven't installed the optional "Graphics Tools" Windows feature.
Consider adding a workflow input to make this check optional, or downgrade to a warning for
workflow_dispatchruns while keeping it mandatory for PR validation.🔎 Proposed enhancement
Add a workflow input and conditional check:
+ inputs: + require-graphics-tools: + description: 'Fail if Graphics Tools not found' + required: false + default: 'true' + type: choice + options: + - 'true' + - 'false' + jobs: shader-unit-tests: name: Run Shader Unit Tests @@ -64,7 +73,11 @@ } else { Write-Host "❌ Graphics Tools NOT found. Tests may fail." Write-Host "Note: GitHub windows-2022 runners should have Graphics Tools pre-installed." - exit 1 + if ("${{ github.event.inputs.require-graphics-tools || 'true' }}" -eq "true") { + exit 1 + } else { + Write-Host "⚠️ Continuing anyway (require-graphics-tools=false)" + } }
75-82: Cache key could cause unnecessary rebuilds on unrelated changes.The cache key includes
hashFiles('extern/**')which will invalidate the cache if ANY file underextern/changes, even non-build-affecting files like READMEs or test data.Consider narrowing to build-relevant files only (e.g.,
extern/**/*.{cpp,h,hpp,cmake,txt}).🔎 Refined cache key
- key: ${{ runner.os }}-shader-tests-msvc-${{ steps.msvc_version.outputs.version }}-${{ hashFiles('.gitmodules', 'extern/**', 'CMakePresets.json', 'vcpkg.json', 'tests/shaders/**') }} + key: ${{ runner.os }}-shader-tests-msvc-${{ steps.msvc_version.outputs.version }}-${{ hashFiles('.gitmodules', 'extern/**/*.{cpp,cxx,cc,c,h,hpp,hxx,cmake,txt}', 'CMakePresets.json', 'vcpkg.json', 'tests/shaders/**/*.{cpp,h,hlsl,hlsli}') }}This reduces false cache invalidations while still catching actual build changes.
package/Shaders/Tests/TestBRDF.hlsl (2)
134-152: Consider tightening the valid range tolerance.Lines 141-144 allow results in
[-0.01, 1.01]to accommodate approximation overshoot. While this accounts for numerical precision, it may mask actual implementation errors. Consider:
- Documenting why overshoot is expected for this specific approximation
- Or tightening to
[-0.001, 1.001]if the overshoot is purely floating-point error📝 Suggested documentation
- // Results should be in valid range (allow small overshoot due to approximation) + // Results should be in valid range [0,1] (allow 1% overshoot due to Lazarov polynomial approximation characteristics) ASSERT(IsTrue, brdf_smooth.x >= -0.01f && brdf_smooth.x <= 1.01f);
250-266: Very loose tolerance when comparing BRDF approximations.Line 265 allows a difference of 0.5 between Hirvonen and Lazarov approximations. Since both are approximating the same environmental BRDF integral, this tolerance seems excessive - they should agree within ~10-20% for most inputs.
Consider tightening to
< 0.2for adding a comment explaining why such a large divergence is acceptable.🔎 Tighter comparison
- // Should give similar-ish results - ASSERT(IsTrue, abs(brdf.x - brdf_lazarov.x) < 0.5f); + // Both approximations should agree within 20% for typical inputs + ASSERT(IsTrue, abs(brdf.x - brdf_lazarov.x) < 0.2f); + ASSERT(IsTrue, abs(brdf.y - brdf_lazarov.y) < 0.2f);Or if the large tolerance is intentional:
- // Should give similar-ish results + // Approximations can diverge significantly at extreme roughness/viewing angles ASSERT(IsTrue, abs(brdf.x - brdf_lazarov.x) < 0.5f);package/Shaders/Tests/TestRandom.hlsl (1)
259-269: Continuity tolerance seems too loose.Line 268 checks that Perlin noise values differ by less than 0.5 when sampled 0.01 units apart. Since Perlin noise should be C1 continuous (smooth gradient), a displacement of 0.01 should yield a much smaller difference (typically < 0.05 for standard implementations).
Consider tightening to verify actual smoothness:
🔎 Tighter continuity validation
- // Nearby positions should have similar values (continuity) - ASSERT(IsTrue, abs(noise1 - noise2) < 0.5f); + // Nearby positions should have similar values (C1 continuity) + // For 0.01 displacement, expect difference << 0.1 + ASSERT(IsTrue, abs(noise1 - noise2) < 0.1f);This better validates the smoothness property of Perlin noise.
tests/shaders/runtime_test_discovery.h (1)
202-224: Suggest more informative error messages.The error message "Test failed" on line 216 is generic. Consider capturing more details from the test result (if available) to help diagnose failures.
🔎 Potential improvement for error reporting
If
fixture.RunTest()provides failure details, capture them:if (!result) { - errorMsg = "Test failed"; + errorMsg = result.GetErrorMessage(); // or similar API if available return false; }If no API exists, at least include test name:
if (!result) { - errorMsg = "Test failed"; + errorMsg = "Test '" + test.name + "' failed"; return false; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.github/workflows/shader-unit-tests.yaml.gitignoreCMakeLists.txtcmake/DetectGraphicsTools.cmakepackage/Shaders/Common/GBuffer.hlslipackage/Shaders/Common/Random.hlslipackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestColor.hlslpackage/Shaders/Tests/TestDisplayMapping.hlslpackage/Shaders/Tests/TestFastMath.hlslpackage/Shaders/Tests/TestGBuffer.hlslpackage/Shaders/Tests/TestLightingCommon.hlslpackage/Shaders/Tests/TestMath.hlslpackage/Shaders/Tests/TestRandom.hlsltests/shaders/.gitignoretests/shaders/CMakeLists.txttests/shaders/README.mdtests/shaders/minimal_test.cpptests/shaders/runtime_discovered_tests.cpptests/shaders/runtime_test_discovery.htests/shaders/test_brdf.cpptests/shaders/test_color.cpptests/shaders/test_common.htests/shaders/test_displaymapping.cpptests/shaders/test_fastmath.cpptests/shaders/test_gbuffer.cpptests/shaders/test_helpers_unified.htests/shaders/test_lightingcommon.cpptests/shaders/test_math.cpptests/shaders/test_random.cpp
🧰 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:
tests/shaders/test_color.cpppackage/Shaders/Tests/TestLightingCommon.hlsltests/shaders/test_gbuffer.cpppackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestColor.hlsltests/shaders/test_brdf.cpptests/shaders/test_math.cpptests/shaders/minimal_test.cpptests/shaders/test_helpers_unified.htests/shaders/test_fastmath.cpppackage/Shaders/Common/GBuffer.hlslipackage/Shaders/Tests/TestGBuffer.hlsltests/shaders/test_random.cpppackage/Shaders/Tests/TestFastMath.hlsltests/shaders/runtime_discovered_tests.cpptests/shaders/runtime_test_discovery.htests/shaders/test_lightingcommon.cpptests/shaders/test_displaymapping.cpppackage/Shaders/Tests/TestDisplayMapping.hlslpackage/Shaders/Common/Random.hlslipackage/Shaders/Tests/TestMath.hlsltests/shaders/test_common.hpackage/Shaders/Tests/TestRandom.hlsl
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
tests/shaders/test_color.cpppackage/Shaders/Tests/TestLightingCommon.hlsltests/shaders/CMakeLists.txttests/shaders/README.mdtests/shaders/test_gbuffer.cpppackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestColor.hlsltests/shaders/test_brdf.cpptests/shaders/test_math.cpptests/shaders/minimal_test.cppCMakeLists.txttests/shaders/test_helpers_unified.htests/shaders/test_fastmath.cpppackage/Shaders/Common/GBuffer.hlslipackage/Shaders/Tests/TestGBuffer.hlslcmake/DetectGraphicsTools.cmaketests/shaders/test_random.cpppackage/Shaders/Tests/TestFastMath.hlsltests/shaders/runtime_discovered_tests.cpptests/shaders/runtime_test_discovery.htests/shaders/test_lightingcommon.cpptests/shaders/test_displaymapping.cpppackage/Shaders/Tests/TestDisplayMapping.hlslpackage/Shaders/Common/Random.hlslipackage/Shaders/Tests/TestMath.hlsltests/shaders/test_common.hpackage/Shaders/Tests/TestRandom.hlsl
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.gitignoretests/shaders/.gitignoretests/shaders/README.md.github/workflows/shader-unit-tests.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
.gitignoretests/shaders/test_color.cpppackage/Shaders/Tests/TestLightingCommon.hlsltests/shaders/.gitignoretests/shaders/README.mdtests/shaders/test_gbuffer.cpppackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestColor.hlsltests/shaders/test_brdf.cpptests/shaders/test_helpers_unified.htests/shaders/test_fastmath.cpppackage/Shaders/Common/GBuffer.hlslipackage/Shaders/Tests/TestGBuffer.hlsltests/shaders/test_random.cpppackage/Shaders/Tests/TestFastMath.hlsltests/shaders/runtime_discovered_tests.cpptests/shaders/runtime_test_discovery.htests/shaders/test_lightingcommon.cpp.github/workflows/shader-unit-tests.yamltests/shaders/test_displaymapping.cpppackage/Shaders/Tests/TestDisplayMapping.hlslpackage/Shaders/Tests/TestMath.hlsltests/shaders/test_common.hpackage/Shaders/Tests/TestRandom.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
.gitignoretests/shaders/.gitignoretests/shaders/README.mdtests/shaders/test_helpers_unified.htests/shaders/test_displaymapping.cpptests/shaders/test_common.h
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
tests/shaders/test_color.cpppackage/Shaders/Tests/TestLightingCommon.hlslpackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestColor.hlsltests/shaders/test_brdf.cpppackage/Shaders/Tests/TestGBuffer.hlsltests/shaders/test_lightingcommon.cpptests/shaders/test_displaymapping.cpppackage/Shaders/Tests/TestDisplayMapping.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to **/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py} : Do not include TODO/FIXME placeholders; provide complete, working solutions
Applied to files:
tests/shaders/.gitignore
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
package/Shaders/Tests/TestColor.hlsltests/shaders/test_lightingcommon.cpp
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
tests/shaders/test_math.cpptests/shaders/test_fastmath.cpppackage/Shaders/Common/GBuffer.hlslipackage/Shaders/Common/Random.hlslipackage/Shaders/Tests/TestMath.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
package/Shaders/Common/GBuffer.hlslipackage/Shaders/Common/Random.hlsli
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Common/GBuffer.hlslipackage/Shaders/Common/Random.hlsli
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
package/Shaders/Common/GBuffer.hlsli
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
tests/shaders/runtime_discovered_tests.cpptests/shaders/runtime_test_discovery.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). (4)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Agent
🔇 Additional comments (48)
.gitignore (1)
13-18: LGTM!The shader test artifact patterns are appropriate and consistent with the existing .gitignore structure.
tests/shaders/.gitignore (1)
1-12: LGTM!Comprehensive coverage of shader test build artifacts and test results. The patterns align well with the testing infrastructure being introduced.
CMakeLists.txt (3)
49-54: LGTM!The BUILD_SHADER_TESTS option is well-integrated into the existing CMake configuration, with clear status messages for developers.
859-866: LGTM!The conditional inclusion of shader tests follows CMake best practices and integrates cleanly with the existing build system.
877-881: LGTM!The user-facing instructions for running shader tests are clear and helpful for developers.
tests/shaders/minimal_test.cpp (1)
1-8: LGTM!The explicit main() function correctly addresses the CMake 4.0 compatibility issue with Catch2WithMain. The implementation is minimal and correct.
tests/shaders/CMakeLists.txt (3)
50-86: LGTM!The shader test executable configuration is well-structured with proper asset management, dependency tracking for HLSL test files, and correct linking order.
88-94: LGTM!Good choice to pin Catch2 to a specific version (v3.11.0) for build reproducibility, and the workaround for CMake 4.0 linking issues is properly documented.
103-114: LGTM!CTest integration is properly configured with reasonable timeout (5 minutes) and appropriate labels for test organization.
package/Shaders/Tests/TestMath.hlsl (3)
1-26: LGTM!TestMathConstants provides comprehensive validation of mathematical constants with appropriate tolerances and relationship checks.
28-49: LGTM!TestEpsilonConstants thoroughly validates epsilon values with exact value checks and ordering relationships.
51-80: LGTM!TestIdentityMatrix provides exhaustive verification of the identity matrix structure, checking all diagonal and off-diagonal elements.
package/Shaders/Common/GBuffer.hlsli (1)
11-37: Tests already validate half-precision encoding behavior across edge cases.The comprehensive test coverage in TestGBuffer.hlsl confirms that half-precision literals are the intended design (as noted in the test file comment: "GBuffer uses half types"). Tests validate normal encoding/decoding round-trip accuracy, cardinal directions, angled normals, and the OctWrap function with appropriate relaxed tolerances (0.01h–0.02h) for half-precision quantization. No further verification needed.
tests/shaders/test_common.h (1)
37-45: Good error handling for missing shader directory.The validation and detailed error message with paths will help diagnose build configuration issues quickly.
package/Shaders/Tests/TestGBuffer.hlsl (2)
1-4: Good practice: consistent use of half types.The comment clarifies the use of half types throughout to avoid conversion warnings, aligning with the GBuffer implementation.
90-100: Verify the large tolerance for vanilla normal encoding.The tolerance of
0.2h(20%) on lines 99-100 seems very large for testing the encoded center point(0.5, 0.5). This might mask issues with the encoding algorithm. Consider verifying whether this large tolerance is necessary or if the vanilla encoding implementation has inherent precision limitations.cmake/DetectGraphicsTools.cmake (1)
27-45: Excellent diagnostic messaging for missing Graphics Tools.The comprehensive warning messages with step-by-step installation instructions and PowerShell alternative will save developers significant troubleshooting time.
tests/shaders/test_lightingcommon.cpp (1)
1-6: LGTM!The test registration follows the established pattern and clearly identifies the test case and entry point.
tests/shaders/test_color.cpp (1)
1-34: LGTM! Comprehensive color test coverage.The test suite covers a wide range of color operations including luminance calculations, color space conversions, gamma handling, and ambient occlusion. The consistent use of SHADER_TEST_SIMPLE with descriptive names and tags will make test failures easy to diagnose.
tests/shaders/test_gbuffer.cpp (1)
1-18: LGTM! Well-structured GBuffer test suite.The tests provide good coverage of normal encoding variants including octahedral encoding, the OctWrap helper, and legacy vanilla encoding. The consistent tagging will enable selective test execution.
tests/shaders/test_random.cpp (1)
1-58: LGTM! Excellent random number generation test coverage.The test suite comprehensively covers PCG random generation, floating-point output ranges, various noise functions, quasi-random sequences, and hashing. The granular test cases will help isolate issues in specific random generation algorithms.
tests/shaders/test_brdf.cpp (1)
1-50: LGTM! Thorough BRDF component test suite.The tests provide comprehensive coverage of BRDF components essential for physically-based rendering, including multiple diffuse models, Fresnel approximations, distribution functions, visibility terms, and environment BRDF variants. The detailed categorization will facilitate targeted testing of specific rendering components.
tests/shaders/test_math.cpp (1)
1-14: LGTM!Clean test registration with good test organization and descriptive names. The separation into constants, epsilon, and matrix tests provides clear test granularity.
tests/shaders/test_displaymapping.cpp (1)
1-46: LGTM!Excellent test coverage for display mapping with well-organized roundtrip tests for color space conversions. The tag hierarchy ([displaymapping][colorspace], [displaymapping][pq], etc.) enables flexible test filtering.
.github/workflows/shader-unit-tests.yaml (1)
110-112: LGTM! Test execution is properly scoped.Good use of ctest's
-Rfilter to run only shader tests, and--output-on-failureensures diagnostic output is available when tests fail.package/Shaders/Tests/TestBRDF.hlsl (2)
5-17: LGTM! Clean constant validation test.The Lambert diffuse test properly validates the constant 1/π value with appropriate tolerance and verifies deterministic behavior.
19-54: Excellent Fresnel test coverage.Comprehensive validation covering:
- Normal incidence (should return F0)
- Grazing angle (should approach 1.0)
- Intermediate angles with interpolation
- Monotonicity as angle increases
- Metallic materials with high F0
This is a model example of thorough BRDF testing.
package/Shaders/Tests/TestLightingCommon.hlsl (1)
5-51: LGTM! Comprehensive shininess-to-roughness conversion tests.Excellent test coverage including:
- Known conversion values with appropriate tolerance
- Monotonicity verification across multiple ranges
- Range validation for typical and extreme inputs
- Direct formula verification with tight tolerance (0.0001)
The progressive testing from basic properties to formula validation is well-structured.
tests/shaders/test_fastmath.cpp (1)
1-66: LGTM! Comprehensive FastMath test coverage.Good organization covering:
- Newton-Raphson approximations at multiple refinement levels (NR0, NR1, NR2) for reciprocal square root, square root, and reciprocal
- Both fast and standard trigonometric approximations
The progressive refinement testing (NR0→NR1→NR2) allows validation of accuracy improvements with iteration count.
package/Shaders/Tests/TestRandom.hlsl (2)
5-26: LGTM! Excellent probabilistic test design.The PCG test properly handles the non-deterministic nature of pseudo-random output:
- Validates determinism for same seed (line 18)
- Avoids false positives by checking "at least one non-zero" rather than "all different" (line 22)
- Verifies state mutation (line 25)
This is a good example of testing random number generators without over-constraining.
45-78: Good statistical validation approach.The f1 range test validates multiple properties:
- Strict range bounds [0, 1) for each sample
- Distribution variety (not constant output)
- Reasonable spread (> 0.1 range)
Consider adding a test for uniform distribution characteristics if relevant for the use case.
package/Shaders/Common/Random.hlsli (4)
54-62: LGTM! Good clarity improvement.Converting hex literals to decimal with inline comments preserves readability while maintaining the original values for reference.
64-103: LGTM! Consistent style improvements.The decimal literal conversions with hex comments are applied consistently throughout the murmur3 implementation.
155-211: LGTM! Enhanced operator precedence clarity.Adding explicit parentheses around bitwise mask operations improves readability without changing behavior.
271-309: LGTM! Better defensive programming.The switch to unsigned hash handling and addition of a default return case ensures defined behavior for all code paths, which is especially important for shader code.
package/Shaders/Tests/TestDisplayMapping.hlsl (3)
1-9: LGTM! Good isolation of test dependencies.The stub implementations isolate DisplayMapping tests from ISHDR dependencies. Note that the stubs may not represent production behavior, but this is appropriate for focused unit testing.
10-75: LGTM! Comprehensive RangeCompress test coverage.The tests validate key mathematical properties (zero point, monotonicity, asymptotic bounds, threshold continuity) and component-wise behavior. The tolerance values are appropriate for shader float precision.
77-237: LGTM! Thorough color space conversion testing.The tests validate round-trip accuracy with appropriate tolerances for each color space, test known values (white point, black), and verify PQ constants. The looser tolerance for ICtCp (0.02f) correctly accounts for PQ encoding precision.
package/Shaders/Tests/TestColor.hlsl (3)
44-123: LGTM! Good coverage of color space conversions.The tests validate round-trip accuracy for YCoCg and Gamma/Linear conversions with appropriate tolerances, and verify saturation edge cases including boundary conditions.
125-199: LGTM! Basic validation of lighting functions.The tests cover fundamental properties: AO luminance progression, specular AO range validation, and non-negativity constraints.
169-183: The 0.2f tolerance is intentionally loose because the three functions implement different standard luminance formulas.The functions use coefficients from different color standards: RGBToLuminance uses Rec. 709 (0.2125, 0.7154, 0.0721), RGBToLuminanceAlternative uses NTSC/PAL (0.3, 0.59, 0.11), and RGBToLuminance2 uses another NTSC variant (0.299, 0.587, 0.114). These are intentionally different, not merely variations.
For the test color (0.6, 0.4, 0.3), the actual differences are ~0.013, which is far below the 0.2f tolerance. The loose tolerance accommodates legitimate standard variations while still catching catastrophic failures. This appears to be a deliberate design choice rather than an oversight.
tests/shaders/runtime_test_discovery.h (3)
24-48: LGTM! Robust camelCase conversion.The implementation correctly handles edge cases including consecutive capitals and uses proper unsigned char casts for character classification.
50-139: LGTM! Clear naming and tagging utilities.The module name extraction and tag inference are straightforward and use appropriate data structures (std::set for uniqueness).
141-173: Limitation: Only discovers tests with [numthreads(1,1,1)].The regex pattern specifically matches
[numthreads(1,1,1)], so tests with different thread group configurations (e.g., 8,8,1 for parallel tests) won't be discovered. This may be intentional for runtime discovery, but should be documented.Is this limitation intentional? If tests with other thread group counts should be discovered, the regex should be generalized:
std::regex numthreadsPattern(R"(\[numthreads\s*\(\s*\d+\s*,\s*\d+\s*,\s*\d+\s*\)\s*\])");However, you'd need to extract the actual thread counts from the match to populate TestFunction correctly.
tests/shaders/test_helpers_unified.h (1)
8-53: LGTM! Well-designed test macro interface.The unified SHADER_TEST macro and its convenience variants provide a clean, consistent interface for shader tests. The documentation is clear and examples are helpful.
package/Shaders/Tests/TestFastMath.hlsl (3)
5-66: LGTM! Well-structured reciprocal sqrt tests.The tests validate Newton-Raphson refinement levels with progressively tighter error bounds and include positivity checks.
68-181: LGTM! Pragmatic tolerance choices for GPU approximations.The tests appropriately relax tolerances to account for GPU hardware variations in fast math approximations, with clear documentation of the adjustments. The "ballpark" approach for NR0 is sensible given the expected error characteristics.
183-350: LGTM! Comprehensive trigonometric approximation tests.The tests appropriately validate known values, boundary behavior, range constraints, and mathematical properties (monotonicity, symmetry) with tolerances suitable for fast approximations.
Pin dependency to commit 447807e (Dec 14, 2025) instead of floating 'main' branch to ensure reproducible builds and prevent unexpected breakage from upstream changes. Enable Git long path support to avoid Windows 260-character path limit during ShaderTestFramework clone. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
alandtse
force-pushed
the
hlsl_test
branch
2 times, most recently
from
8a5ddb2 to
2d652cb
Compare
- Add missing shader directory include flag to GenerateShaderTests function - Remove unused DXGI_DEBUG_DLL variable from DetectGraphicsTools.cmake Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Add shader unit tests as inline job in build.yaml and create reusable workflow file for future use. Changes: - Add shader-unit-tests job to build.yaml (inline implementation) - Create _reusable-shader-unit-tests.yaml for potential future refactoring - Remove old shader-unit-tests.yaml (replaced by inline + reusable versions) - Match shader-validation trigger conditions and permissions - Skip when no HLSL files changed via check-changes integration - Update README to reference new workflow location Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/build.yaml (1)
567-576: Consider including MSVC version in cache key for consistency.The cache key for shader tests omits the MSVC version, unlike the
cpp-build(line 211) andshader-validation(line 436) jobs. While this may be intentional for simplicity, it could lead to cache hits with incompatible compiled binaries if the MSVC version changes between runs.Proposed cache key improvement
To align with other jobs, you could extract and include the MSVC version. However, this adds complexity, so only implement if cache invalidation issues arise:
+ - name: Get MSVC version + if: steps.check-hlsl.outputs.skip != 'true' + id: msvc_version + shell: pwsh + run: | + # (same version extraction logic as cpp-build job) + - name: Cache CMake build output if: steps.check-hlsl.outputs.skip != 'true' uses: actions/cache@v4 with: path: build/ALL - key: ${{ runner.os }}-shader-tests-${{ hashFiles('.gitmodules', 'extern/**', 'CMakePresets.json', 'vcpkg.json', 'tests/shaders/**') }} + key: ${{ runner.os }}-shader-tests-msvc-${{ steps.msvc_version.outputs.version }}-${{ hashFiles('.gitmodules', 'extern/**', 'CMakePresets.json', 'vcpkg.json', 'tests/shaders/**') }} restore-keys: | ${{ runner.os }}-shader-tests-cmake/DetectGraphicsTools.cmake (1)
24-24: Minor redundancy: CACHE and PARENT_SCOPE both used.The script sets
GRAPHICS_TOOLS_INSTALLEDas a CACHE variable (lines 24, 46) and also exports it to PARENT_SCOPE (line 67). While not harmful, the PARENT_SCOPE export is redundant since CACHE variables are already globally accessible across CMake files.The PARENT_SCOPE export can be removed for clarity, as the CACHE variable already provides the necessary visibility:
- # Export for use in other CMake files - set(GRAPHICS_TOOLS_INSTALLED ${GRAPHICS_TOOLS_INSTALLED} PARENT_SCOPE) -However, keeping both is a common defensive pattern and doesn't cause issues.
Also applies to: 46-46, 67-67
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yamlcmake/DetectGraphicsTools.cmaketests/shaders/README.mdtests/shaders/test_helpers_unified.h
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/shaders/README.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
cmake/DetectGraphicsTools.cmaketests/shaders/test_helpers_unified.h
**/*.{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:
tests/shaders/test_helpers_unified.h
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
tests/shaders/test_helpers_unified.h
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
tests/shaders/test_helpers_unified.h
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to **/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py} : Do not include TODO/FIXME placeholders; provide complete, working solutions
Applied to files:
tests/shaders/test_helpers_unified.h
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
tests/shaders/test_helpers_unified.h
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
tests/shaders/test_helpers_unified.h
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
tests/shaders/test_helpers_unified.h.github/workflows/build.yaml
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
tests/shaders/test_helpers_unified.h
🔇 Additional comments (8)
tests/shaders/test_helpers_unified.h (5)
1-7: LGTM! Clean header structure.The includes are minimal and appropriate for the test infrastructure.
8-39: Well-designed macro with clear documentation.The SHADER_TEST macro provides excellent abstraction over the ShaderTestFramework boilerplate. The shader include directory handling (lines 32-35) correctly ensures shaders can reference other shader files.
45-53: Useful convenience wrappers.The SHADER_TEST_SIMPLE and SHADER_TEST_PARALLEL macros provide sensible defaults for common use cases while maintaining the ability to use SHADER_TEST directly for custom thread group configurations.
68-80: Clean data structure and helper macros.The ShaderTestEntry struct with its helper macros provides a simple, type-safe way to define batch test configurations.
82-107: Excellent batch test generation with addressed fix.The previous concern about missing shader include directory has been properly addressed (lines 86 and 93). The GenerateShaderTests template now correctly includes the
-Iflag with the shader directory path, matching the pattern used in the SHADER_TEST macro. The implementation efficiently computes the shader directory once and reuses it for all test entries..github/workflows/build.yaml (2)
624-624: LGTM! Dependency correctly updated.The
prereleasejob now depends onshader-unit-tests, ensuring tests pass before publishing pre-release builds.
788-788: LGTM! Dependency correctly updated.The
releasejob now depends onshader-unit-tests, ensuring tests pass before publishing release builds.cmake/DetectGraphicsTools.cmake (1)
1-73: LGTM! Graphics Tools detection is well-implemented.The script provides robust detection of Windows Graphics Tools with:
- Multiple environment variable fallbacks for Windows directory resolution
- Clear, actionable error messages with manual and automated setup instructions
- Appropriate platform-specific handling
- Proper variable export for downstream CMake usage
The implementation correctly gates shader test execution based on D3D12 debug layer availability, which aligns with the workflow's verification step.
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build.yaml (1)
552-564: The Graphics Tools verification will fail on standard windows-2022 runners.As flagged in previous reviews, GitHub's windows-2022 runners do not include Graphics Tools or DirectX SDK by default. The hard exit at line 563 will cause this job to fail unless Graphics Tools are explicitly installed in a prior step.
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
601-609: Consider using lukka/run-cmake for consistency.Lines 601-609 use raw
cmakecommands, while the cpp-build (lines 235-239) and shader-validation (lines 463-468) jobs use thelukka/run-cmake@v10action. Using the action provides better error handling and consistency across the workflow.🔎 Optional refactor to use lukka/run-cmake action
- - name: Configure CMake with shader tests - if: steps.check-hlsl.outputs.skip != 'true' - run: | - cmake --preset ALL -DBUILD_SHADER_TESTS=ON - - - name: Build shader tests + - name: Configure and build shader tests if: steps.check-hlsl.outputs.skip != 'true' - run: | - cmake --build build/ALL --target shader_tests --config Release + uses: lukka/run-cmake@v10 + with: + configurePreset: ALL + configurePresetAdditionalArgs: "['-DBUILD_SHADER_TESTS=ON']" + buildPreset: ALL + buildPresetAdditionalArgs: "['--target shader_tests', '--config Release']"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.github/workflows/build.yaml
⏰ 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). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (5)
.github/workflows/build.yaml (5)
238-238: LGTM! Correctly excludes shader tests from the cpp-build job.This prevents unnecessary shader test compilation during the main C++ build, since shader tests are now handled by the dedicated
shader-unit-testsjob.
466-466: LGTM! Correctly excludes shader tests from the shader-validation job.Consistent with line 238, this ensures shader tests aren't built during shader compilation validation.
616-624: LGTM! Proper test artifact upload on failure.The upload configuration appropriately captures test results for debugging and uses reasonable retention settings.
629-629: LGTM! Correctly gates releases on shader test success.Adding
shader-unit-teststo the dependency chains of both prerelease and release jobs ensures shader tests pass before artifacts are published. Note that these jobs will be blocked until the Graphics Tools issue (lines 552-564) is resolved.Also applies to: 793-793
611-614: The test naming pattern is correct. The CMakeLists.txt registers the test withadd_test(NAME ShaderTests ...), which exactly matches the ctest filter-R ShaderTestsin the workflow. The command will correctly discover and run the tests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/build.yaml (1)
204-214: Same local action reference issue applies here.The
check-hlsl-changesandsetup-build-environmentactions are referenced before any checkout step exists in this job.
🧹 Nitpick comments (1)
.github/actions/setup-build-environment/action.yaml (1)
56-61: Cleanup ofdummy.cppmay be skipped on error.If the MSVC version is not found, the
throwon line 59 exits immediately, leavingdummy.cppin the workspace. Consider moving cleanup before the throw or using atry/finallypattern.🔎 Proposed fix
if ($version) { Add-Content -Path $env:GITHUB_OUTPUT -Value "version=$version" -Encoding UTF8 + Remove-Item "dummy.cpp" -ErrorAction SilentlyContinue } else { + Remove-Item "dummy.cpp" -ErrorAction SilentlyContinue throw "MSVC version not found" } - Remove-Item "dummy.cpp" -ErrorAction SilentlyContinue
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/check-hlsl-changes/action.yaml.github/actions/setup-build-environment/action.yaml.github/workflows/build.yaml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Windows, use BuildRelease.bat ALL as the primary build preset; do not cancel builds or shader validation
Applied to files:
.github/workflows/build.yaml
⏰ 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). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (7)
.github/actions/setup-build-environment/action.yaml (2)
68-75: Cache key composition looks well-designed.The cache key includes MSVC version, multiple suffix sources for flexibility, and hashes of relevant configuration files. The restore-keys provide appropriate fallback granularity.
77-91: Good defensive handling for cross-runner cache invalidation.The drive letter mismatch detection correctly handles scenarios where GitHub-hosted runners may mount workspaces on different drives between runs, preventing CMake configuration errors from stale absolute paths.
.github/actions/check-hlsl-changes/action.yaml (1)
15-33: LGTM - Clean conditional logic for HLSL job gating.The action correctly handles both PR and non-PR events, defaulting to running HLSL jobs when the input is not explicitly set to skip.
.github/workflows/build.yaml (4)
287-337: The shader-unit-tests job now inherits proper caching from the setup-build-environment action.The previous review comment about missing MSVC version in the cache key is addressed by using the
setup-build-environmentcomposite action, which includes MSVC version detection and incorporates it into the cache key (line 72 of action.yaml).The job flow is well-structured: change detection → environment setup → configure → build → test → artifact upload on failure.
338-341: Good quality gate - prerelease now depends on shader unit tests.Adding
shader-unit-teststo the needs array ensures shader tests must pass before prereleases are created from PRs.
503-505: Release job correctly gates on shader-unit-tests.The release job now requires shader unit tests to pass alongside the existing cpp-build, shader-validation, and feature-audit jobs.
129-133: Local composite action reference requires prior checkout.Referencing
./.github/actions/setup-build-environmentrequires the repository to be checked out first. However, the checkout step is inside the composite action itself. GitHub Actions cannot resolve local action paths until the code is available.🔎 Proposed fix - add checkout before using local action
steps: + - name: Checkout code for local actions + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.ref || github.ref }} + repository: ${{ github.event.pull_request.head.repo.full_name || github.repository }} + - name: Setup Build Environment id: setup uses: ./.github/actions/setup-build-environment with: cache-key-suffix: "cpp"Then remove or make conditional the checkout step inside the composite action to avoid duplicate checkouts.
⛔ Skipped due to learnings
Learnt from: CR Repo: doodlum/skyrim-community-shaders PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-08-17T18:37:35.839Z Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yaml (1)
120-126: Align workflow dispatch input with job purpose.The condition checks
github.event.inputs.validate-shaders(line 125), but this job runs shader unit tests (ctest), not validation. The input name and description (lines 19-23) refer to "HLSL shader validation," which may confuse users. Consider renaming the input totest-shadersorrun-shader-testsand updating its description to reflect that it controls shader unit testing, or update the description to cover both validation and testing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yaml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Windows, use BuildRelease.bat ALL as the primary build preset; do not cancel builds or shader validation
Applied to files:
.github/workflows/build.yaml
⏰ 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). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (6)
.github/workflows/build.yaml (6)
59-59: LGTM!The
shader-tests-should-buildoutput correctly triggers on shader test, HLSL, CMake, and CI changes, following the same pattern ashlsl-should-build.
86-86: LGTM!Correctly separates shader test files into a dedicated
shader_testsgroup and excludes them from C++ change detection, maintaining clean separation of concerns.Also applies to: 106-107
143-156: LGTM!The CMake configuration, build, and test execution steps are well-structured. Using the ALL preset with
BUILD_SHADER_TESTS=ON, building the specificshader_teststarget, and running ctest with the ShaderTests filter and--output-on-failureare all appropriate choices for CI shader testing.
158-166: LGTM!The test results artifact upload on failure is properly configured. Using the standard ctest output path (
build/ALL/Testing/**), 7-day retention, and appropriate failure conditions ensures test diagnostics are captured when needed.
171-171: The cpp-build job exists and correctly produces the version output.The
cpp-buildjob is defined at line 117 and explicitly declares theversionoutput (line 132) via theget_versionstep, which extracts the version from CMakeCache.txt. Theprereleaseandreleasejobs correctly declare their dependencies oncpp-build, and all references toneeds.cpp-build.outputs.versionare valid. No action required.
131-141: The referenced code and composite actions do not exist in the repository.The code snippet shown at lines 131-141 in the review comment does not match the actual content of
.github/workflows/build.yamlon the dev branch. The referenced composite actions (./.github/actions/check-hlsl-changesand./.github/actions/setup-build-environment) do not exist in the repository. Lines 131-141 of the actual build.yaml contain different content related to job outputs and the cpp-build job definition, with no composite action usage at those locations.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
294-347: LGTM with a suggestion for consistency.The shader-unit-tests job is well-structured with proper conditional execution, artifact uploads, and test result reporting. The use of
-R ShaderTestsin ctest provides focused test execution.💡 Optional: Consider using lukka/run-cmake for consistency
For consistency with the
cpp-buildandshader-validationjobs, consider usinglukka/run-cmake@v10instead of direct cmake commands:- - name: Configure CMake with shader tests - if: steps.check-hlsl.outputs.skip != 'true' - run: | - cmake --preset ALL -DBUILD_SHADER_TESTS=ON - - - name: Build shader tests + - name: Build and test shaders if: steps.check-hlsl.outputs.skip != 'true' - run: | - cmake --build build/ALL --target shader_tests --config Release - - - name: Run shader unit tests - if: steps.check-hlsl.outputs.skip != 'true' - run: | - ctest --test-dir build/ALL -C Release --output-on-failure -R ShaderTests + uses: lukka/run-cmake@v10 + with: + configurePreset: ALL + configurePresetAdditionalArgs: "['-DBUILD_SHADER_TESTS=ON']" + buildPreset: ALL + buildPresetAdditionalArgs: "['--target shader_tests']" + testPreset: # If a test preset exists for ShaderTestsThe current approach is functional, so this is purely for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yaml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Windows, use BuildRelease.bat ALL as the primary build preset; do not cancel builds or shader validation
Applied to files:
.github/workflows/build.yaml
⏰ 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). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (7)
.github/workflows/build.yaml (7)
59-59: LGTM!The new
shader-tests-should-buildoutput correctly combines shader test file changes with HLSL, CMake, and CI changes. The inclusion of the failure fallback ensures tests run when change detection fails.
86-86: LGTM!Properly separates shader test files from C++ changes and creates a dedicated pattern for shader tests. This ensures shader test changes trigger the appropriate test job without unnecessarily triggering C++ builds.
Also applies to: 106-107
211-221: LGTM!The refactoring to use composite actions for HLSL change detection and build environment setup improves consistency across jobs. The explicit
-DBUILD_SHADER_TESTS=OFFensures shader validation doesn't build unnecessary test artifacts. The matrix-specific cache suffix properly isolates Flatrim and VR validation caches.Also applies to: 258-258
351-351: LGTM!Correctly adds
shader-unit-teststo the prerelease dependencies, ensuring shader tests must pass before publishing a prerelease build for PRs.
515-515: LGTM!Correctly adds
shader-unit-teststo the release dependencies, ensuring all shader tests pass before creating or updating releases. This maintains quality gates for tagged releases.
143-143: Excellent separation of build concerns.The explicit control of
BUILD_SHADER_TESTSper job (OFF for cpp-build and shader-validation, ON for shader-unit-tests) is well-designed. This prevents unnecessary test artifact compilation in validation and release builds while ensuring tests are built when needed.Also applies to: 258-258, 326-326
133-137: The file.github/workflows/build.yamland the referenced composite action.github/actions/setup-build-environmentdo not exist in this repository.The
.githubdirectory is not present in the repository at all. This review comment cannot be applied to the current codebase.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
338-352: Consider using lukka/run-cmake for consistency with other jobs.The shader-unit-tests job uses direct
cmakeandctestcommands, while cpp-build and shader-validation uselukka/run-cmake@v10. While the current approach works, using the same action would improve consistency and maintainability.🔎 Example refactor using lukka/run-cmake
- - name: Configure CMake with shader tests - if: steps.check-hlsl.outputs.skip != 'true' - run: | - cmake --preset ALL -DBUILD_SHADER_TESTS=ON - - - name: Build shader tests - if: steps.check-hlsl.outputs.skip != 'true' - run: | - cmake --build build/ALL --target shader_tests --config Release - - - name: Run shader unit tests - if: steps.check-hlsl.outputs.skip != 'true' - run: | - ctest --test-dir build/ALL -C Release --output-on-failure -R ShaderTests + - name: Build and run shader tests + if: steps.check-hlsl.outputs.skip != 'true' + uses: lukka/run-cmake@v10 + with: + configurePreset: ALL + configurePresetAdditionalArgs: "['-DBUILD_SHADER_TESTS=ON']" + buildPreset: ALL + buildPresetAdditionalArgs: "['--target shader_tests']" + testPreset: ALL + testPresetAdditionalArgs: "['-R ShaderTests', '--output-on-failure']"Note: This assumes a test preset exists in CMakePresets.json. If not, a separate ctest step may still be needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/setup-build-environment/action.yaml.github/workflows/build.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/actions/setup-build-environment/action.yaml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Windows, use BuildRelease.bat ALL as the primary build preset; do not cancel builds or shader validation
Applied to files:
.github/workflows/build.yaml
⏰ 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). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (7)
.github/workflows/build.yaml (7)
59-59: Good addition of shader-tests-should-build output.The trigger conditions correctly include shader test changes, HLSL changes, CMake changes, and CI changes, ensuring shader unit tests run when relevant code is modified.
86-86: Clean separation of shader tests from C++ build changes.Excluding
tests/shaders/**from the cpp pattern and tracking it separately in the shader_tests pattern provides proper isolation for shader test changes.Also applies to: 106-107
150-150: Appropriate to disable shader tests in the cpp-build job.Since shader tests have a dedicated
shader-unit-testsjob, disabling them in cpp-build avoids redundant test execution and keeps build jobs focused.
222-232: Good refactoring to use check-hlsl-changes and setup-build-environment actions.The conditional execution based on HLSL changes and matrix-aware cache key suffix (
validation-${{ matrix.config.name }}) are appropriate for the shader-validation job.
305-362: Well-structured shader-unit-tests job with good conditional execution.The new shader-unit-tests job properly checks for relevant changes, uses the setup-build-environment action, and uploads test results on failure. The absence of a matrix (unlike shader-validation) is appropriate since shader unit tests are configuration-agnostic.
366-366: Good addition of shader-unit-tests to release dependencies.Making both prerelease and release jobs depend on shader-unit-tests establishes an important quality gate, ensuring shader tests pass before artifacts are published.
Also applies to: 530-530
140-145: The setup-build-environment action properly handles MSVC version in the cache key. The action detects the MSVC compiler version viacl.exe /Bvand includes it directly in the cache key generation (${{ steps.msvc_version.outputs.version }}), which prevents cache conflicts across different MSVC versions. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/build.yaml (2)
308-314: Misleading workflow input reuse for shader unit tests.The job condition at lines 312-314 reuses
github.event.inputs.validate-shadersto control both shader validation and shader unit tests. Since these are distinct operations (compilation validation vs runtime unit tests), consider either:
- Adding a dedicated
test-shadersinput for manual dispatch- Renaming the existing input to something more generic like
run-shader-checksThis improves clarity when manually triggering workflows.
🔎 Proposed fix to add dedicated input
In the
workflow_dispatchinputs section (around lines 19-23), add a new input:validate-shaders: description: "Run HLSL shader validation for manual dispatch" type: boolean required: false default: true + test-shaders: + description: "Run HLSL shader unit tests for manual dispatch" + type: boolean + required: false + default: trueThen update the shader-unit-tests condition:
if: > always() && !cancelled() && ((github.event_name == 'pull_request_target' && !github.event.pull_request.draft) || (github.event_name == 'workflow_dispatch' && - github.event.inputs.validate-shaders == 'true') || + github.event.inputs.test-shaders == 'true') || (github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v')))
347-350: Consider adding a timeout to the ctest command.The
ctestcommand at line 350 lacks a timeout, which could cause the job to hang indefinitely if a shader test deadlocks or enters an infinite loop. Consider adding the--timeoutflag.🔎 Proposed fix to add timeout
- name: Run shader unit tests if: steps.check-hlsl.outputs.skip != 'true' run: | - ctest --test-dir build/ALL -C Release --output-on-failure -R ShaderTests + ctest --test-dir build/ALL -C Release --output-on-failure --timeout 300 -R ShaderTestsAdjust the timeout value (300 seconds shown) based on expected test duration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yaml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Windows, use BuildRelease.bat ALL as the primary build preset; do not cancel builds or shader validation
Applied to files:
.github/workflows/build.yaml
⏰ 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). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
.github/workflows/build.yaml (4)
59-59: LGTM: Clean separation of shader test changes from cpp build triggers.The addition of
shader-tests-should-buildoutput and the exclusion oftests/shaders/**from thecpppattern correctly isolates shader test changes from C++ build triggers while ensuring the new shader-unit-tests job runs when appropriate.Also applies to: 86-86, 106-107
140-145: LGTM: Refactor to reusable action and explicit test exclusion.The refactor to use a reusable
setup-build-environmentaction improves maintainability, and explicitly settingBUILD_SHADER_TESTS=OFFcorrectly avoids building shader tests during the C++ plugin build.Also applies to: 150-150
222-232: LGTM: Consistent refactor with smart change detection.The addition of HLSL change detection via the
check-hlsl-changesaction and refactor tosetup-build-environmentis consistent with the cpp-build job. The matrix-specific cache key suffix and explicitBUILD_SHADER_TESTS=OFFare both appropriate.Also applies to: 269-269
365-365: LGTM: Shader unit tests now gate releases.Adding
shader-unit-teststo theneedslist ensures shader tests pass before creating prereleases and releases. This is appropriate for maintaining code quality, though it means test reliability becomes critical for the release pipeline.Also applies to: 529-529
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (14)
package/Shaders/Common/FrameBuffer.hlsli (1)
141-141: Optional refactor for clarity.The change from
any(uv < float2(0, 0) || uv > max.xy)to two separateany()checks is logically equivalent and makes the boundary conditions slightly more explicit. Both implementations are correct.package/Shaders/Tests/TestFrameBuffer.hlsl (2)
14-20: Remove unused test constants.The constants
FLOAT16_EPSILON,APPROX_TOLERANCE, andNEAR_ZEROare defined but never used in the test implementations. OnlyEXACT_TOLERANCEis actually used.🔎 Proposed cleanup
namespace TestConstants { - static const float FLOAT16_EPSILON = 0.001f; - static const float APPROX_TOLERANCE = 0.01f; static const float EXACT_TOLERANCE = 0.0001f; - static const float NEAR_ZERO = 0.0001f; }
101-119: Consider verifying the expected value for unit input.Line 116-118 tests
ToSRGBColor(float3(1.0, 1.0, 1.0))but only checks for non-NaN and non-negative. Consider adding an assertion to verify the result is close to 1.0 (since pow(1, x) = 1 for any x).🔎 Suggested enhancement
// Test one float3 srgbOne = FrameBufferTest::ToSRGBColor(float3(1.0, 1.0, 1.0)); ASSERT(IsTrue, all(!isnan(srgbOne))); + ASSERT(IsTrue, all(abs(srgbOne - 1.0) < TestConstants::EXACT_TOLERANCE)); ASSERT(IsTrue, all(srgbOne >= 0.0));package/Shaders/Tests/TestFastMath.hlsl (2)
33-50: Consider adding consistent positivity checks.The NR0 variants test for positive results, but NR1 and NR2 variants of both
fastRcpSqrtandfastSqrtomit this check. Since these mathematical operations should always return positive values for positive inputs, adding positivity assertions would improve consistency and catch potential implementation bugs.Example for TestFastRcpSqrtNR1
// Should be within ~0.2% error as documented float error = RelativeError(fast, exact); ASSERT(IsTrue, error < 0.003f); + + // Result should be positive + ASSERT(IsTrue, fast > 0.0f); }Apply similar checks to
TestFastRcpSqrtNR2,TestFastSqrtNR1, andTestFastSqrtNR2.Also applies to: 52-69, 92-108, 110-126
16-18: Consider using array size calculation instead of hardcoded loop bounds.Multiple test functions use hardcoded
5for loop bounds when iterating over test arrays. Using the array length directly would improve maintainability if test values are added or removed.Example refactor
- float testValues[5] = { 1.0f, 4.0f, 0.25f, 100.0f, 0.01f }; + static const float testValues[] = { 1.0f, 4.0f, 0.25f, 100.0f, 0.01f }; + static const int numTests = sizeof(testValues) / sizeof(testValues[0]); - for (int i = 0; i < 5; i++) + for (int i = 0; i < numTests; i++)Alternatively, if the test framework supports it, use a macro like
ARRAYSIZE(testValues).Also applies to: 38-40, 57-59, 75-77, 96-98, 114-116, 132-134, 159-161, 178-180
package/Shaders/Tests/TestColor.hlsl (1)
169-171: Consider using epsilon comparison for floating-point equality.Line 171 uses
AreEqualfor exact equality, which may be fragile with floating-point arithmetic. Consider using an epsilon-based comparison similar to other tests.🔎 Suggested improvement
- // With full AO (1.0), result should be 1.0 - float fullAOResult = Color::SpecularAOLagarde(1.0f, 1.0f, 0.5f); - ASSERT(AreEqual, fullAOResult, 1.0f); + // With full AO (1.0), result should be 1.0 + float fullAOResult = Color::SpecularAOLagarde(1.0f, 1.0f, 0.5f); + ASSERT(IsTrue, abs(fullAOResult - 1.0f) < 0.001f);package/Shaders/Tests/TestBRDF.hlsl (3)
286-290: Very loose tolerance for approximation comparison.The tolerance of 0.5f when comparing Lazarov and Hirvonen approximations is quite large. This essentially allows them to differ by up to 50% of the possible range, which might not catch cases where one approximation is significantly worse than the other.
Consider tightening this tolerance if both approximations should give reasonably similar results:
- // Should give similar-ish results - ASSERT(IsTrue, abs(brdf.x - brdf_lazarov.x) < 0.5f); + // Should give similar results (both are approximations of the same function) + ASSERT(IsTrue, abs(brdf.x - brdf_lazarov.x) < 0.2f);
662-676: Floating-point loop control variable.Using a floating-point variable as a loop control can lead to unexpected behavior due to accumulation of rounding errors. While this works for simple cases like
vdoth -= 0.1f, it's generally better practice to use integer loop counters.🔎 Proposed refactor using integer loop counter
- // As VdotH decreases (angle increases), Fresnel should increase - for (float vdoth = 1.0f; vdoth >= 0.0f; vdoth -= 0.1f) + // As VdotH decreases (angle increases), Fresnel should increase + for (int i = 0; i <= 10; i++) { + float vdoth = 1.0f - i * 0.1f; float current = BRDF::F_Schlick(F0, vdoth).x; // Check monotonicity (allow small tolerance for floating point) if (vdoth < 0.99f) { ASSERT(IsTrue, current >= prev - TestConstants::FLOAT16_EPSILON); } // Check physical bounds ASSERT(IsTrue, current >= F0.x - TestConstants::FLOAT16_EPSILON); ASSERT(IsTrue, current <= 1.0f + TestConstants::FLOAT16_EPSILON); prev = current; }
688-700: Floating-point loop control variable.Same issue as in
TestFresnelMonotonicity: using a floating-point loop control variable can accumulate rounding errors.🔎 Proposed refactor using integer loop counter
- for (float roughness = 0.1f; roughness <= 1.0f; roughness += 0.1f) + for (int i = 1; i <= 10; i++) { + float roughness = i * 0.1f; float d = BRDF::D_GGX(roughness, NdotH); // Distribution peak should decrease as roughness increases ASSERT(IsTrue, d <= prev + TestConstants::FLOAT16_EPSILON); // Should always be positive and finite ASSERT(IsTrue, d > 0.0f); ASSERT(IsTrue, !isinf(d)); prev = d; }package/Shaders/Tests/TestGBuffer.hlsl (1)
37-65: Consider using thelength()intrinsic for consistency.Line 62 manually computes the vector length. While functionally correct, using
length(decoded)would be more readable and consistent with thenormalize()calls elsewhere.🔎 Suggested fix
- half length = sqrt(decoded.x * decoded.x + decoded.y * decoded.y + decoded.z * decoded.z); - ASSERT(IsTrue, abs(length - 1.0h) < 0.02h); // Relaxed tolerance for half precision + half len = length(decoded); + ASSERT(IsTrue, abs(len - 1.0h) < 0.02h); // Relaxed tolerance for half precisiontests/shaders/runtime_test_discovery.h (1)
53-63: Consider usingstd::filesystem::pathmethods for extension handling.Using
path.stem()andpath.extension()would be more robust than manual substring operations.🔎 Suggested approach
inline std::string extractModuleName(const std::string& filename) { - std::string name = filename; - if (name.find("Test") == 0) { - name = name.substr(4); - } - if (name.length() >= 5 && name.substr(name.length() - 5) == ".hlsl") { - name = name.substr(0, name.length() - 5); - } - return name; + std::filesystem::path p(filename); + std::string name = p.stem().string(); + if (name.find("Test") == 0) { + name = name.substr(4); + } + return name; }package/Shaders/Tests/TestLightingCommon.hlsl (1)
87-102: Consider tightening the smoothness threshold.The smoothness check uses
diff < 0.5, which is quite loose (half the entire [0,1] range). In practice, with a shininess step of 10.0, the roughness difference should be much smaller (typically < 0.1). A tighter threshold likediff < 0.2would more effectively catch discontinuities or formula errors while still allowing for the natural gradient of the function.🔎 Suggested tighter threshold
// Should not have huge jumps (smoothness) float diff = abs(curr - prev); - ASSERT(IsTrue, diff < 0.5f); + ASSERT(IsTrue, diff < 0.2f); // Tighter threshold for better discontinuity detectionpackage/Shaders/Tests/TestPBR.hlsl (2)
1-11: Consider adding synchronization checks for duplicated PBR code.The manual duplication of PBR namespace (lines 11-101) avoids constant buffer dependencies but creates maintenance risk if
Common/PBR.hlslidiverges. Consider adding a comment in the original PBR.hlsli file referencing these tests, or adding a verification script to detect inconsistencies.Would you like me to generate a verification script that compares the duplicated definitions with the source file to detect drift?
506-516: Consider expanding edge case coverage for microfacet tests.This test only validates typical production-safe ranges. Consider adding tests for extreme cases like zero roughness, grazing angles (NdotL/NdotV/NdotH near 0), or perpendicular geometry (NdotH = 0) to match the thoroughness of the microflakes edge case tests (lines 520-546).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
package/Shaders/Common/FrameBuffer.hlslipackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestColor.hlslpackage/Shaders/Tests/TestDisplayMapping.hlslpackage/Shaders/Tests/TestFastMath.hlslpackage/Shaders/Tests/TestFrameBuffer.hlslpackage/Shaders/Tests/TestGBuffer.hlslpackage/Shaders/Tests/TestLightingCommon.hlslpackage/Shaders/Tests/TestMath.hlslpackage/Shaders/Tests/TestPBR.hlslpackage/Shaders/Tests/TestRandom.hlslpackage/Shaders/Tests/TestSphericalHarmonics.hlsltests/shaders/CMakeLists.txttests/shaders/README.mdtests/shaders/runtime_test_discovery.h
✅ Files skipped from review due to trivial changes (1)
- tests/shaders/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- package/Shaders/Tests/TestRandom.hlsl
- package/Shaders/Tests/TestMath.hlsl
🧰 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:
package/Shaders/Tests/TestFrameBuffer.hlslpackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestLightingCommon.hlslpackage/Shaders/Tests/TestColor.hlslpackage/Shaders/Tests/TestSphericalHarmonics.hlslpackage/Shaders/Tests/TestPBR.hlslpackage/Shaders/Tests/TestFastMath.hlslpackage/Shaders/Tests/TestGBuffer.hlslpackage/Shaders/Tests/TestDisplayMapping.hlslpackage/Shaders/Common/FrameBuffer.hlslitests/shaders/runtime_test_discovery.h
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Tests/TestFrameBuffer.hlslpackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestLightingCommon.hlslpackage/Shaders/Tests/TestColor.hlslpackage/Shaders/Tests/TestSphericalHarmonics.hlslpackage/Shaders/Tests/TestPBR.hlslpackage/Shaders/Tests/TestFastMath.hlslpackage/Shaders/Tests/TestGBuffer.hlslpackage/Shaders/Tests/TestDisplayMapping.hlslpackage/Shaders/Common/FrameBuffer.hlslitests/shaders/runtime_test_discovery.htests/shaders/CMakeLists.txt
🧠 Learnings (7)
📓 Common learnings
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Tests/TestFrameBuffer.hlslpackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestLightingCommon.hlslpackage/Shaders/Tests/TestColor.hlslpackage/Shaders/Tests/TestSphericalHarmonics.hlslpackage/Shaders/Tests/TestPBR.hlslpackage/Shaders/Tests/TestFastMath.hlslpackage/Shaders/Tests/TestGBuffer.hlslpackage/Shaders/Tests/TestDisplayMapping.hlslpackage/Shaders/Common/FrameBuffer.hlsli
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Tests/TestFrameBuffer.hlslpackage/Shaders/Tests/TestBRDF.hlslpackage/Shaders/Tests/TestLightingCommon.hlslpackage/Shaders/Tests/TestColor.hlslpackage/Shaders/Tests/TestSphericalHarmonics.hlslpackage/Shaders/Tests/TestPBR.hlslpackage/Shaders/Tests/TestGBuffer.hlslpackage/Shaders/Tests/TestDisplayMapping.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
package/Shaders/Tests/TestFrameBuffer.hlslpackage/Shaders/Common/FrameBuffer.hlsli
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Tests/TestFrameBuffer.hlslpackage/Shaders/Common/FrameBuffer.hlsli
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
tests/shaders/runtime_test_discovery.h
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
tests/shaders/CMakeLists.txt
⏰ 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). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (48)
package/Shaders/Tests/TestFrameBuffer.hlsl (3)
26-43: Function isolation approach is sound.The isolated function copies with mocked constants are a pragmatic solution to the constant buffer size constraints. The implementations correctly match their originals, and the risk of drift is acknowledged in the header comments.
49-95: IsOutsideFrame tests are comprehensive.The test coverage includes basic boundary checks, extreme values, and property-based validation (monotonicity and symmetry). The boundary semantics correctly treat
maxUVas an inclusive upper bound.
121-154: ToSRGBColor edge case and property tests are well-designed.The tests appropriately cover negative values (handled via
abs()), small values, HDR range, and mathematical properties (monotonicity and homogeneity). This provides good confidence in the gamma conversion implementation.package/Shaders/Tests/TestFastMath.hlsl (2)
1-3: LGTM!The includes are properly structured and correctly reference the FastMath utilities and test framework.
11-366: Excellent test coverage for FastMath approximations.The test suite comprehensively covers the FastMath utilities with:
- Progressive testing of Newton-Raphson iterations (NR0, NR1, NR2) showing error reduction
- Appropriate boundary value testing for trigonometric functions
- Well-justified relaxed tolerances accounting for GPU hardware variations
- Good use of monotonicity checks for fast approximations
- Proper
@tagsannotations for test categorizationThe test structure is clear and consistent across all 17 test functions.
package/Shaders/Tests/TestColor.hlsl (8)
5-43: LGTM!Comprehensive luminance testing with proper edge cases (white, black), coefficient verification for Rec. 709, and bounds checking. The test logic is sound and the epsilon values are appropriate.
45-69: LGTM!Good coverage of YCoCg roundtrip conversion with appropriate test colors and tight epsilon tolerance.
71-95: LGTM!Well-designed saturation tests covering desaturation, preservation, and over-saturation edge cases with appropriate constraints.
97-127: LGTM!Proper gamma conversion roundtrip testing for both regular and TrueLinear variants with appropriate tolerance.
129-152: LGTM!MultiBounceAO test properly validates the luminance progression across different AO levels with appropriate bounds checking.
188-189: Verify the 0.2f tolerance is intentional.The tolerance of 0.2 (20% deviation) between luminance calculation variants is significantly larger than other tests (0.001f-0.01f). Ensure this is intentional if the algorithms differ substantially, or tighten the tolerance if they should be more consistent.
192-207: LGTM!Appropriate non-negativity testing for Diffuse and Light functions with good edge case coverage (black input).
3-3: No action needed. The include path/Test/STF/ShaderTestFramework.hlsliis consistent across all 12 test files in the repository. While the ShaderTestFramework.hlsli file does not yet exist, the include path follows the established convention and is not inconsistent with other includes.Likely an incorrect or invalid review comment.
package/Shaders/Tests/TestBRDF.hlsl (3)
159-163: Good tolerance for approximations.The test correctly allows small overshoot beyond [0,1] for approximation functions, which is appropriate since these are mathematical approximations that may not strictly preserve bounds.
512-542: Excellent edge case coverage for GGX.The test thoroughly covers critical edge cases including near-zero roughness, maximum roughness, and boundary angles. The checks for NaN and infinity are particularly important for robust shader code.
1-701: Comprehensive BRDF test suite with good edge case coverage.This is a well-structured unit test file that thoroughly tests various BRDF components. The test organization is logical, progressing from basic functions to edge cases and property tests. The use of tolerance constants is appropriate for GPU shader testing.
Strengths:
- Excellent edge case coverage (lines 512-650)
- Good use of property-based testing (monotonicity, symmetry)
- Appropriate tolerance handling for approximations
- Clear test naming with tags
Minor areas for improvement:
- Some tests make fragile assumptions about minimum differences between values
- Floating-point loop control variables could be replaced with integer counters
- One weakened assertion that might benefit from restoration with appropriate tolerance
Overall, this is high-quality test code that will help ensure BRDF function correctness.
package/Shaders/Tests/TestGBuffer.hlsl (2)
1-35: Well-structured roundtrip test for cardinal normals.The test correctly validates encoding/decoding reversibility for axis-aligned normals with appropriate half-precision tolerances. Good coverage of the six cardinal directions.
93-120: Verify vanilla encoding behavior with horizontal normals (z=0).The
testNormalsarray at lines 107-111 includes normals withz=0. IfEncodeNormalVanillauses a formula liken.xy / (sqrt(8 - 8*n.z) + tiny), horizontal normals may produce edge-case values. The wide tolerance[-10, 10]suggests awareness, but consider documenting this explicitly or testing the expected range more precisely.package/Shaders/Tests/TestDisplayMapping.hlsl (4)
1-9: Good use of stubs to isolate DisplayMapping tests.The stub declarations for
GetTonemapFactorHejlBurgessDawsonandParamallow testing DisplayMapping functions without pulling in the full ISHDR dependencies. This is a clean testing pattern.
10-78: Comprehensive range compression tests with good coverage.The tests validate:
- Zero input behavior
- Monotonic increase
- Asymptotic bound (< 1.0)
- Threshold-based compression (identity below, compress above)
- Per-component float3 processing
Well-structured assertions.
80-177: Roundtrip tests for color space conversions are thorough.Good coverage of Linear↔PQ, RGB↔XYZ, XYZ↔LMS, and RGB↔ICtCp with appropriately relaxed tolerances for PQ encoding (0.02f) vs tighter tolerances for linear matrix transforms (0.001f).
196-212: PQ constant validation adds valuable sanity checks.Testing that ST.2084 constants are within expected ranges helps catch accidental modifications to the specification values.
tests/shaders/CMakeLists.txt (2)
17-25: ShaderTestFramework is now pinned to a specific commit.Good improvement from the previous review - pinning to commit
447807eaensures reproducible builds.
96-102: Catch2 is properly pinned to a release tag.Using
v3.11.0ensures stable builds. Good practice.tests/shaders/runtime_test_discovery.h (2)
134-151: Edge case:[numthreads]and function on the same line won't be detected.If a test is written as
[numthreads(1,1,1)] void TestFoo() { ... }on a single line, the current logic reads the next line for the function pattern, missing it. This is unlikely given typical HLSL formatting but worth documenting or handling.
188-219: Test execution with error handling looks solid.The
runTestfunction properly captures exceptions and formats error messages, with console output for immediate visibility during test runs.package/Shaders/Tests/TestSphericalHarmonics.hlsl (5)
5-19: Well-organized tolerance constants for different precision levels.The
TestConstantsnamespace provides clear, documented tolerance levels that appropriately account for float16 precision limitations vs exact mathematical operations.
79-91: Good use ofall()intrinsic for vector comparison.Line 86 correctly uses
all(zero == 0.0)for checking all components simultaneously, followed by individual component assertions for detailed failure messages.
149-169: Variable naming avoids type shadowing.Using
sh2Valinstead ofsh2avoids conflict with thesh2type alias. Good practice for readability.
303-322: Edge case tests validate robustness with degenerate inputs.Testing with unnormalized (
float3(2,2,2)) and zero (float3(0,0,0)) directions verifies the SH functions don't produce NaN. This is valuable for catching potential division-by-zero issues.
324-375: Mathematical property tests (linearity, orthogonality) add confidence.Testing that
a*sh1 + a*sh2 == a*(sh1 + sh2)and verifying distinct outputs for cardinal directions provides higher-level validation beyond unit tests.package/Shaders/Tests/TestLightingCommon.hlsl (2)
5-52: LGTM! Comprehensive test coverage for ShininessToRoughness.The test function provides excellent coverage with:
- Known value verification with appropriate tolerances
- Monotonicity checks at multiple points
- Range validation across various shininess values
- Edge case testing for mirror-like surfaces
- Explicit formula verification with tight tolerance
All mathematical expectations are correct based on the formula
roughness = (2/(shininess+2))^0.25.
77-81: The test comment is accurate, but the test lacks output validation.The
ShininessToRoughnessimplementation does useabs()as the comment states:pow(abs(2.0 / (shininess + 2.0)), 0.25). However,abs()is applied to the division result, not to the shininess value itself. For negative shininess like-10.0f, this produces a valid result (~0.707) rather than NaN, but it differs from the positive equivalent (ShininessToRoughness(10.0f)≈ 0.639). The test only checks for non-NaN/non-infinite results without validating the actual output—consider adding an assertion for the expected value, e.g.,ASSERT(IsTrue, roughness_neg >= 0.0f && roughness_neg <= 1.0f);to ensure the result remains in the valid range.package/Shaders/Tests/TestPBR.hlsl (15)
13-23: LGTM!The PBR constants are well-defined with sensible physical ranges. MinRoughness prevents BRDF singularities, and all values use explicit float suffixes.
25-40: LGTM!Flag constants are correctly defined as unique powers of 2, allowing proper bitwise operations.
42-62: LGTM!TerrainFlags are well-organized with a clear pattern (6 tiles × 3 properties), using unique bit positions.
64-71: LGTM!Standard Cook-Torrance microfacet BRDF implementation using GGX distribution, Smith joint visibility, and Schlick Fresnel approximation.
73-80: LGTM!Correct sheen/fabric BRDF implementation using Charlie distribution with Neubelt visibility term.
82-90: LGTM!Implements anisotropic IOR calculation for hair rendering, averaging longitudinal and azimuthal components with a reasonable cos2PhiH assumption.
97-100: LGTM!Standard Gaussian distribution correctly implemented with proper normalization. Edge case handling (B=0) is addressed in the edge case tests.
105-136: LGTM!Comprehensive test coverage validating physical values, monotonicity, range constraints, and identity case.
140-159: LGTM!Well-structured test validating physical plausibility, determinism, and integration with IORToF0.
163-196: LGTM!Thorough validation of Gaussian distribution properties including symmetry, peak behavior, and width parameter effects.
200-259: LGTM!Excellent test coverage validating physical constraints, Fresnel behavior, angular dependencies, roughness effects, and material variations.
263-314: LGTM!Solid test coverage for Charlie/Neubelt sheen BRDF, validating non-negativity, roughness behavior, and colored specular support.
318-388: LGTM!Both tests provide solid validation of PBR constants (range checks) and flags (uniqueness, bit operations).
457-476: LGTM!Good defensive testing of edge cases including unphysical values (negative IOR, zero IOR), ensuring robust error handling.
520-546: LGTM!Comprehensive edge case testing including zero specular, HDR colors, and grazing angles for Charlie distribution validation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
package/Shaders/Tests/TestPBR.hlsl (3)
6-6: ShaderTestFramework.hlsli include path issue already flagged.This include path has been identified in a previous review as missing and inconsistent with the project's
/Shaders/...pattern.
90-93: Parameter name typo already flagged.The parameter name "IOF" has been identified in a previous review as a typo that should be "IOR" (Index of Refraction).
496-500: Division-by-zero handling and assertion logic issue already flagged.The B=0 edge case test and its incorrect assertion logic (OR instead of AND) have been identified in a previous review.
🧹 Nitpick comments (4)
package/Shaders/Tests/TestFastMath.hlsl (3)
35-52: Consider adding positivity checks for consistency.TestFastRcpSqrtNR0 verifies that the result is positive (line 31), but the NR1 and NR2 variants omit this check. For consistency and completeness, consider adding the same positivity assertion to these variants.
🔎 Suggested additions
For TestFastRcpSqrtNR1, after line 50:
float error = RelativeError(fast, exact); ASSERT(IsTrue, error < 0.003f); + + // Result should be positive + ASSERT(IsTrue, fast > 0.0f); }For TestFastRcpSqrtNR2, after line 69:
float error = RelativeError(fast, exact); ASSERT(IsTrue, error < 0.00001f); + + // Result should be positive + ASSERT(IsTrue, fast > 0.0f); }Also applies to: 54-71
94-110: Consider adding positivity checks for consistency.TestFastSqrtNR0 includes a positivity check (line 90), but the NR1 and NR2 variants don't. Adding this assertion would improve test coverage consistency.
🔎 Suggested additions
For TestFastSqrtNR1, after line 108:
float error = RelativeError(fast, exact); ASSERT(IsTrue, error < 0.003f); + + // Result should be positive + ASSERT(IsTrue, fast > 0.0f); }For TestFastSqrtNR2, after line 126:
float error = RelativeError(fast, exact); ASSERT(IsTrue, error < 0.00001f); + + // Result should be positive + ASSERT(IsTrue, fast > 0.0f); }Also applies to: 112-128
157-174: Consider adding positivity checks for consistency.TestFastRcpNR0 verifies positivity (line 144), but NR1 and NR2 variants don't. Adding this check would make the test suite more consistent.
🔎 Suggested additions
For TestFastRcpNR1, after line 172:
float error = RelativeError(fast, exact); ASSERT(IsTrue, error < 0.02f); // Relaxed from 0.005f to 0.02f (2%) - GPU implementations vary + + // Result should be positive + ASSERT(IsTrue, fast > 0.0f); }For TestFastRcpNR2, after line 190:
float error = RelativeError(fast, exact); ASSERT(IsTrue, error < 0.00001f); // Relaxed from 0.000001f to 0.00001f + + // Result should be positive + ASSERT(IsTrue, fast > 0.0f); }Also applies to: 176-192
package/Shaders/Tests/TestPBR.hlsl (1)
8-99: Consider extracting production PBR utilities to a dedicated header file.The PBR namespace (constants, flags, and utility functions) appears to contain production-quality code that could be reused by other shaders. Test files typically contain only test code and test helpers. Consider creating a new
/Shaders/Common/PBR.hlslifile for these utilities so they can be imported by both production shaders and test files.This would improve:
- Code reusability across the shader codebase
- Separation of concerns (production code vs. test code)
- Maintainability (production utilities in predictable locations)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package/Shaders/Common/BRDF.hlslipackage/Shaders/Tests/TestFastMath.hlslpackage/Shaders/Tests/TestPBR.hlsl
✅ Files skipped from review due to trivial changes (1)
- package/Shaders/Common/BRDF.hlsli
🧰 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:
package/Shaders/Tests/TestFastMath.hlslpackage/Shaders/Tests/TestPBR.hlsl
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Tests/TestFastMath.hlslpackage/Shaders/Tests/TestPBR.hlsl
🧠 Learnings (6)
📓 Common learnings
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Tests/TestFastMath.hlslpackage/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to **/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py} : Do not include TODO/FIXME placeholders; provide complete, working solutions
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
⏰ 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). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
package/Shaders/Tests/TestFastMath.hlsl (2)
6-11: LGTM! Division by zero guard properly implemented.The RelativeError helper correctly handles near-zero exact values by returning absolute error, preventing division by zero. This addresses the concern from the previous review.
194-368: LGTM! Comprehensive trigonometric function tests.The trigonometric test functions provide excellent coverage:
- Boundary value testing for special angles
- Range validation with appropriate margins for fast approximations
- Property testing (monotonicity for ACos/ASin, symmetry for ATan)
- Well-documented relaxed tolerances for GPU hardware variations
The test design appropriately balances precision expectations with the practical realities of fast approximations.
package/Shaders/Tests/TestPBR.hlsl (2)
101-451: Excellent test coverage for PBR utilities.The test implementations are comprehensive and well-structured:
- Physical constraints validation (IOR ranges, F0 bounds, non-negativity)
- Monotonicity checks for IOR and Gaussian distributions
- Edge case testing (identity IOR, grazing angles, distribution widths)
- Flag uniqueness and combination verification
The tests effectively validate both correctness and numerical stability of the PBR utility functions.
453-474: Good edge case coverage with NaN/Inf validation.The edge case tests appropriately validate numerical stability:
- Invalid/extreme IOR values (zero, negative, very large)
- Extreme distribution parameters (very narrow/wide, far from peak)
- Zero and HDR specular colors
- Grazing angle conditions
The NaN/Inf checks ensure the functions degrade gracefully rather than producing undefined results.
Also applies to: 476-495, 502-544
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
package/Shaders/Tests/TestPBR.hlsl (2)
90-93: Parameter name typo: "IOF" should be "IOR".
6-6: ShaderTestFramework.hlsli include path issue persists.The include path
/Test/STF/ShaderTestFramework.hlslireferences a file that doesn't exist in the repository. This will cause compilation failures.
🧹 Nitpick comments (1)
package/Shaders/Tests/TestPBR.hlsl (1)
504-516: Consider renaming or expanding this test.The function is named
TestSpecularMicrofacetEdgeCasesbut only tests typical production-safe values. Consider either:
- Renaming to
TestSpecularMicrofacetBasic- Adding actual edge case tests (e.g., NdotL=0, NdotV=0, roughness at min/max bounds)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yamlpackage/Shaders/Common/PBR.hlslipackage/Shaders/Tests/TestPBR.hlsltests/shaders/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
tests/shaders/CMakeLists.txtpackage/Shaders/Common/PBR.hlslipackage/Shaders/Tests/TestPBR.hlsl
**/*.{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:
package/Shaders/Common/PBR.hlslipackage/Shaders/Tests/TestPBR.hlsl
🧠 Learnings (9)
📓 Common learnings
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
.github/workflows/build.yamlpackage/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
.github/workflows/build.yamltests/shaders/CMakeLists.txt
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
.github/workflows/build.yamlpackage/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Windows, use BuildRelease.bat ALL as the primary build preset; do not cancel builds or shader validation
Applied to files:
.github/workflows/build.yaml
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to **/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py} : Do not include TODO/FIXME placeholders; provide complete, working solutions
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
⏰ 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). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (21)
package/Shaders/Common/PBR.hlsli (1)
116-118: LGTM! Division-by-zero guard is correctly implemented.The fix properly protects both uses of
Bin the Gaussian formula. Using1e-6as the minimum is appropriate for this floating-point context, preventing numerical instability while maintaining the function's mathematical behavior for very narrow distributions.package/Shaders/Tests/TestPBR.hlsl (10)
103-136: Solid test coverage for IORToF0.The tests appropriately verify physical correctness (glass/water/diamond F0 values), monotonicity, valid output range, and the identity case. The tolerance values are reasonable for floating-point comparisons.
138-159: LGTM!Good coverage of the HairIOR function including range validation, expected value verification, determinism, and composition with IORToF0.
161-196: LGTM!Comprehensive tests for Gaussian distribution properties: positivity, symmetry, peak behavior, and the inverse relationship between width and peak height.
198-259: LGTM!Excellent test coverage for the GGX microfacet BRDF: physical constraints (non-negativity), Fresnel bounds, grazing angle behavior, roughness variation, and metallic material handling.
261-314: LGTM!Good test coverage for the Charlie distribution used in fabric/sheen materials, including comparisons with GGX behavior.
316-344: LGTM!Thorough validation of PBR constant ranges ensuring correct ordering and physical validity.
346-453: LGTM!Comprehensive flag tests verifying uniqueness (no collisions), non-zero values, and correct bitwise combinability for both PBR flags and terrain flags.
498-502: Previous issue addressed: B=0 edge case test is now correct.The assertion now correctly uses
&&and the comment accurately reflects the clamping behavior inHairGaussian.
518-546: LGTM!Good edge case coverage including zero specular color, HDR values, and grazing angles.
9-101: The review comment is based on a false premise. There is nopackage/Shaders/Common/PBR.hlslifile in the repository. The PBR namespace definitions in TestPBR.hlsl are test-specific, not duplicates of production code. The file's own comment (line 8) explicitly explains why these definitions are here: "PBR constants and flags (can't be in BRDF.hlsli as they're PBR-specific, not general BRDF)". This is intentional test isolation, not a maintenance issue.Likely an incorrect or invalid review comment.
tests/shaders/CMakeLists.txt (5)
4-10: LGTM: Clean opt-out pattern.The
BUILD_SHADER_TESTSoption with early return provides a clean way to disable shader tests when needed.
17-23: LGTM: Dependency pinning addresses previous security concern.The ShaderTestFramework is now pinned to a specific commit rather than the mutable
mainbranch, which addresses the supply-chain risk flagged in previous reviews. Thecore.longpaths=trueconfiguration is a good addition for Windows compatibility.
79-88: LGTM: Correct count calculation.The use of
list(LENGTH HLSL_TEST_FILES HLSL_TEST_COUNT)properly addresses the previous review comment aboutCMAKE_MATCH_COUNT. The test file enumeration and logging provide clear visibility into discovered tests.
112-123: LGTM: Proper CTest integration.The test registration correctly uses generator expressions for the working directory, includes a reasonable timeout, and applies appropriate labels for test organization. The compact reporter will produce CI-friendly output.
38-43: The referenced filetests/shaders/CMakeLists.txtand theDetectGraphicsToolsmodule do not exist in this repository. The repository contains only shader features and packages, with no CMake build system or test infrastructure. This review comment cannot be applied to the current codebase.Likely an incorrect or invalid review comment.
.github/workflows/build.yaml (5)
59-59: LGTM! Shader test change detection properly configured.The new
shader-tests-should-buildoutput correctly combines shader test file changes with HLSL, CMake, and CI changes. Excludingtests/shaders/**from the cpp file group prevents false positives in C++ build triggering.Also applies to: 86-86, 106-107
150-150: LGTM! BUILD_SHADER_TESTS flag correctly configured across jobs.The flag is explicitly set to
OFFincpp-buildandshader-validationto avoid unnecessary test compilation, and set toONonly inshader-unit-tests. This ensures tests are built and run only where needed.Also applies to: 269-269, 343-343
365-365: LGTM! Release dependencies correctly updated.Both
prereleaseandreleasejobs now depend onshader-unit-tests, ensuring shader unit tests pass before publishing releases. This adds an important quality gate to the release process.Also applies to: 529-529
331-336: MSVC version is already included in the cache key.The
setup-build-environmentaction properly detects the MSVC compiler version and includes it in the cache key construction:${{ runner.os }}-cmake-msvc-${{ steps.msvc_version.outputs.version }}-${{ inputs.cache-key-suffix }}-.... This ensures that caches are not shared across different MSVC versions, preventing build failures or test inconsistencies.Likely an incorrect or invalid review comment.
305-361: The shader unit tests are correctly designed to run without a matrix strategy. The test files (TestMath.hlsl, TestColor.hlsl, TestBRDF.hlsl, TestFastMath.hlsl, etc.) contain unit tests for shared utility functions and do not reference any VR or Flatrim-specific defines. These are pure math and utility unit tests that produce identical results regardless of shader configuration. Theshader-validationjob appropriately uses a matrix to test both Flatrim and VR configurations because production shaders contain VR-specific code (register layouts, instance IDs, buffer offsets). Unit tests for the underlying shared math utilities need only run once.
|
Looks good, I like that it has documentation. I need to test that the tests successfully pass first. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
package/Shaders/Tests/TestPBR.hlsl (1)
5-6: Verify the include paths resolve correctly in the build system.The include paths use absolute-style paths (
/Shaders/...and/Test/STF/...). Ensure the build system configures the appropriate include directories so these paths resolve correctly during shader compilation.Run the following to verify the include structure:
#!/bin/bash # Check if ShaderTestFramework.hlsli exists and verify include path patterns echo "=== Searching for ShaderTestFramework.hlsli ===" fd -t f "ShaderTestFramework.hlsli" echo "" echo "=== Checking include patterns in test files ===" rg -n "#include" package/Shaders/Tests/ 2>/dev/null | head -30
🧹 Nitpick comments (1)
package/Shaders/Tests/TestPBR.hlsl (1)
410-421: Consider expanding edge case coverage.The test name suggests edge cases, but currently only tests typical parameter values. Consider adding tests for:
- Minimum roughness (0.04)
- Maximum roughness (1.0)
- Extreme grazing angles (NdotV ≈ 0)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package/Shaders/Common/PBR.hlslipackage/Shaders/Common/PBRMath.hlslipackage/Shaders/Tests/TestPBR.hlsl
🧰 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:
package/Shaders/Common/PBR.hlslipackage/Shaders/Tests/TestPBR.hlslpackage/Shaders/Common/PBRMath.hlsli
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Common/PBR.hlslipackage/Shaders/Tests/TestPBR.hlslpackage/Shaders/Common/PBRMath.hlsli
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Common/PBR.hlslipackage/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Common/PBR.hlslipackage/Shaders/Common/PBRMath.hlsli
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
package/Shaders/Common/PBR.hlslipackage/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
package/Shaders/Common/PBR.hlsli
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Common/PBR.hlsli
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
package/Shaders/Common/PBR.hlslipackage/Shaders/Common/PBRMath.hlsli
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
package/Shaders/Common/PBR.hlsli
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to **/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py} : Do not include TODO/FIXME placeholders; provide complete, working solutions
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
package/Shaders/Tests/TestPBR.hlsl
⏰ 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). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (28)
package/Shaders/Common/PBR.hlsli (1)
9-9: LGTM! Clean extraction of PBR utilities for testability.The inclusion of
PBRMath.hlsliproperly enables the refactored PBR utilities (constants, flags, and pure math functions) to be tested independently while maintaining the same functionality in this production file.package/Shaders/Tests/TestPBR.hlsl (15)
9-41: Well-structured IOR-to-F0 tests with good coverage.The test covers physically meaningful materials (glass, water, diamond), monotonicity properties, valid output ranges, and the identity case. The tolerances are appropriate for floating-point comparisons.
44-64: Good test coverage for HairIOR function.The determinism test is particularly valuable to ensure the function has no unintended side effects or state dependencies.
67-101: Thorough Gaussian distribution property tests.The tests validate key mathematical properties: symmetry, peak location, width-amplitude relationship, and non-negativity. Good use of tolerances for floating-point comparisons.
104-164: Excellent BRDF property validation.The tests cover physical constraints (non-negativity), Fresnel behavior (grazing angle increase, bounds), roughness effects, and material variation (dielectric vs metallic). This provides good confidence in the microfacet model implementation.
167-219: Good coverage of Charlie distribution behavior.The tests appropriately validate the Charlie microflake model's unique properties, including its grazing-angle behavior which differs from GGX. The comparison with the GGX model helps verify both implementations produce valid results.
222-249: Good constant consistency validation.Testing that constants are within expected ranges and maintain proper relationships (min < max, non-zero where required) helps catch inadvertent changes to critical material parameters.
252-293: Solid bitfield flag uniqueness validation.The nested loop approach ensures no two flags share the same value, which is critical for correct bitwise flag operations. The combination test verifies the expected OR/AND behavior.
296-358: Comprehensive terrain flag validation.The test ensures all terrain tile flag categories (PBR, displacement, glint) are unique and can be combined correctly. The cross-category check on line 332 is particularly valuable.
361-381: Important edge case robustness tests.Testing invalid inputs (IOR=0, negative IOR) ensures the function degrades gracefully rather than producing NaN/Inf. This is important for shader stability.
384-407: Good edge case handling with proper B=0 protection.The B=0 test correctly expects the function to handle the degenerate case gracefully. The assertion logic properly uses
&&to verify the result is both not-NaN and not-Inf.
424-451: Good edge case coverage for microflakes model.The tests appropriately cover zero specular, HDR values, and grazing angles which are important edge cases for fabric/sheen materials.
454-484: Good wetness direct lighting test coverage.The light color preservation test (lines 476-479) and the black light test (lines 481-483) are particularly valuable for verifying correct color handling.
487-519: Comprehensive wetness indirect lighting tests.The horizon occlusion test (lines 508-512) is particularly valuable as it validates an important visual quality feature that prevents specular reflections from appearing below the surface horizon.
522-554: Good edge case coverage for wetness functions.Tests appropriately validate behavior at roughness extremes and with HDR light values.
557-584: Good property-based wetness tests.The tests validate expected material behavior properties rather than just implementation details, which makes them more robust to implementation changes.
package/Shaders/Common/PBRMath.hlsli (12)
1-6: Good header structure with proper include guard.The header follows project conventions with a clear include guard and includes the necessary dependencies (Math.hlsli, BRDF.hlsli).
9-19: Well-defined PBR constants with appropriate values.The MinRoughness of 0.04 is a standard choice to prevent singularities in GGX-based BRDFs. The use of
static constwithin namespaces follows project conventions. Based on learnings, this is the appropriate pattern for complex constants.
21-36: Proper bitfield flag design.All flags use unique bit positions (1 << 0 through 1 << 12), enabling proper bitwise combination and testing.
38-58: Well-organized terrain flag layout.The flags are logically grouped by type (PBR, Displacement, Glint) with clear bit range separation, making the structure easy to understand and maintain.
60-76: Well-documented GGX microfacet implementation.The function correctly implements the standard Cook-Torrance specular BRDF with GGX distribution. The documentation clearly explains each parameter.
78-93: Appropriate Charlie microflake implementation for sheen materials.The combination of Charlie distribution with Neubelt visibility is appropriate for fabric/sheen materials. The documentation clearly describes the use case.
95-105: Standard Marschner hair IOR implementation.The function implements the effective IOR calculation for hair with a reasonable assumption of average orientation (cos2PhiH = 0.5).
107-113: Correct Fresnel F0 implementation.The standard formula for converting IOR to F0 (reflectance at normal incidence) is correctly implemented. The parameter naming issue from the past review has been addressed.
115-124: Robust Gaussian implementation with division-by-zero protection.The
max(B, 1e-6)guard on line 122 prevents division by zero for degenerate cases. This addresses the concern raised in previous review.
126-148: Good wetness specular implementation.The wetnessF0 of 0.02 is physically appropriate for a thin water film. The use of EPSILON_DOT_CLAMP prevents numerical issues at grazing angles.
150-173: Well-implemented wetness indirect specular with horizon occlusion.The horizon occlusion technique (lines 165-170) is properly implemented with the Marmoset reference. This prevents specular reflections from appearing below the geometry horizon.
1-176: Excellent refactoring for testability.This header cleanly extracts pure PBR math functions and constants, enabling comprehensive unit testing without game-specific dependencies. The documentation is thorough, and the implementations follow standard PBR practices. This directly supports the PR objective of adding HLSL unit tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CMakeLists.txt (2)
861-868: Consider adding directory existence check for robustness.The
add_subdirectory(tests/shaders)call assumes the directory exists. While this is likely true for this PR, adding an existence check would make the build more robust and provide a clearer error message if the directory is missing.🔎 Proposed enhancement
if(BUILD_SHADER_TESTS) message(STATUS "Adding shader tests subdirectory") + if(NOT EXISTS "${CMAKE_SOURCE_DIR}/tests/shaders/CMakeLists.txt") + message(FATAL_ERROR "BUILD_SHADER_TESTS is ON but tests/shaders directory is missing or incomplete") + endif() enable_testing() # Enable CTest integration for shader tests add_subdirectory(tests/shaders) endif()
879-883: Use generic build directory placeholder instead of hardcoded path.The hardcoded path "build/ALL" in the usage messages may not match the user's actual build directory. Using a generic placeholder or showing the actual CMAKE_BINARY_DIR would be more accurate.
🔎 Proposed fix
if(BUILD_SHADER_TESTS) message("To run shader tests:") - message(" cmake --build build/ALL --target shader_tests") - message(" ctest --test-dir build/ALL -R ShaderTests") + message(" cmake --build <build-dir> --target shader_tests") + message(" ctest --test-dir <build-dir> -R ShaderTests") endif()Or use the actual binary directory:
if(BUILD_SHADER_TESTS) message("To run shader tests:") - message(" cmake --build build/ALL --target shader_tests") - message(" ctest --test-dir build/ALL -R ShaderTests") + message(" cmake --build ${CMAKE_BINARY_DIR} --target shader_tests") + message(" ctest --test-dir ${CMAKE_BINARY_DIR} -R ShaderTests") endif()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
CMakeLists.txt
🧠 Learnings (1)
📓 Common learnings
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
🔇 Additional comments (1)
CMakeLists.txt (1)
49-49: LGTM!The BUILD_SHADER_TESTS option is properly declared with a clear description and reasonable default value (ON). The message output is consistent with other build options in the project.
Also applies to: 54-54
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/shaders/CMakeLists.txt (1)
96-96: Clarify the CMake version reference.The comment mentions "CMake 4.0 linking issues," but CMake 4.0 has not been released. Update this to reference the actual CMake version causing the issue (likely CMake 3.x) or describe the specific linking problem more clearly.
🔎 Suggested fix
-# Add Catch2 for testing (but not Catch2WithMain - we provide our own main() due to CMake 4.0 linking issues) +# Add Catch2 for testing (but not Catch2WithMain - we provide our own main() due to known linking issues with Catch2WithMain)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txttests/shaders/CMakeLists.txttests/shaders/README.mdtests/shaders/runtime_discovered_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/shaders/runtime_discovered_tests.cpp
- CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
tests/shaders/CMakeLists.txttests/shaders/README.md
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
tests/shaders/CMakeLists.txttests/shaders/README.md
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
tests/shaders/CMakeLists.txt
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
tests/shaders/CMakeLists.txt
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
tests/shaders/README.md
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
tests/shaders/README.md
⏰ 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). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (8)
tests/shaders/README.md (3)
1-221: Excellent documentation for shader testing framework.This is comprehensive and well-structured documentation that covers all essential aspects: quick start, test writing patterns, troubleshooting, and CI integration. The examples are clear and helpful for developers getting started with shader unit tests.
201-203: Clarify the CMake version reference.The section header "CMake 4.0 Compatibility" is misleading since CMake 4.0 has not been released. Clarify which CMake version this refers to (likely CMake 3.x) or describe the specific linking issue more accurately.
🔎 Suggested fix
-**CMake 4.0 Compatibility:** +**CMake Linking Compatibility:** -If you encounter `unresolved external symbol main` errors, this is due to a known incompatibility between CMake 4.0 and Catch2's `Catch2WithMain` target. The build has been updated to work around this by providing an explicit `main()` function. +If you encounter `unresolved external symbol main` errors, this is due to a known linking issue with Catch2's `Catch2WithMain` target in recent CMake versions. The build has been updated to work around this by providing an explicit `main()` function.Likely an incorrect or invalid review comment.
174-176: Remove the reference to-DAUTO_OPEN_OPTIONAL_FEATURES.This CMake option is not defined in the project. The
tests/shaders/CMakeLists.txtonly definesBUILD_SHADER_TESTS. Either implement the option or remove it from the documentation.⛔ Skipped due to learnings
Learnt from: CR Repo: doodlum/skyrim-community-shaders PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-08-17T18:37:35.839Z Learning: On Windows, use BuildRelease.bat ALL as the primary build preset; do not cancel builds or shader validationtests/shaders/CMakeLists.txt (5)
4-10: Clean option pattern with clear messaging.The
BUILD_SHADER_TESTSoption is well-defined with a descriptive message, and the early return pattern keeps the rest of the file clean when tests are disabled.
17-23: Good practice: dependency pinned to specific commit.The ShaderTestFramework dependency is now pinned to a specific commit hash for build reproducibility, which addresses supply-chain concerns and ensures consistent builds across environments.
79-88: HLSL file tracking implemented correctly.The use of
list(LENGTH)to count test files is correct, and marking them asHEADER_FILE_ONLYensures they trigger rebuilds without attempting compilation. The status messages provide useful feedback during configuration.
108-123: CTest integration is well-configured.The test registration with CTest is properly configured with appropriate timeout (5 minutes) and labels for organization. The compact reporter format is suitable for CI environments.
97-102: Catch2 v3.11.0 is a valid official release (published September 30, 2025) and is the latest stable version as of December 2025. No security advisories found.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/shaders/README.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
tests/shaders/README.md
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
tests/shaders/README.md
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
tests/shaders/README.md
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
tests/shaders/README.md
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
tests/shaders/README.md
⏰ 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). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (1)
tests/shaders/README.md (1)
157-163: Approved: Previous graphics tools installation issue has been resolved.The documentation now provides the correct direct PowerShell command (
Enable-WindowsOptionalFeature) instead of referencing a non-existent script. This fix makes Option 1 actionable and functional for users.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.