feat: raytracing#1658
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces comprehensive ray tracing feature for DirectX 12 including shader pipelines, GPU resource management, runtime infrastructure, and denoising system. Adds HLSL shaders for ray generation, geometry, lighting, and denoising; C++ abstractions for DX12 resources, descriptor management, and pipelines; integration hooks with existing rendering systems; and build dependencies for DirectX Shader Compiler and D3D12 Memory Allocator. Changes
Sequence DiagramssequenceDiagram
participant Engine as Skyrim Engine
participant RT as RT Feature
participant D3D12 as DirectX 12
participant GPU as GPU
Engine->>RT: PostPostLoad()
RT->>D3D12: Initialize() / InitD3D12()
D3D12->>D3D12: CreatePipelines() / CreateRootSignature()
RT->>D3D12: SetupResources()
D3D12->>D3D12: CreateDescriptorHeaps() / CompileShaders()
Note over Engine,GPU: Per-Frame Rendering
Engine->>RT: DrawRTGI()
RT->>RT: UpdateDynamicSkinning()
RT->>D3D12: DispatchRays(RayGeneration shader)
D3D12->>GPU: Trace rays through TLAS/BLAS
GPU->>GPU: RayGeneration → ClosestHit/Miss/AnyHit
GPU->>GPU: Evaluate BRDF / Sample Lights
GPU-->>D3D12: Output: GI Radiance
RT->>D3D12: RenderShadows() (Shadow pass)
D3D12->>GPU: Dispatch ShadowAnyHit / ShadowMiss
GPU-->>D3D12: Output: Shadow Mask
RT->>D3D12: SHaRCPipeline::Resolve()
D3D12->>GPU: Compute shader: Cache resolution
GPU-->>D3D12: Output: Resolved radiance cache
RT->>D3D12: SVGFPipeline::Denoise()
D3D12->>GPU: Temporal pass (history blend)
GPU-->>D3D12: Blended radiance
D3D12->>GPU: Variance pass (3×3 neighbors)
GPU-->>D3D12: Variance texture
D3D12->>GPU: Spatial pass (A-Trous)
GPU-->>D3D12: Denoised output
RT->>Engine: Output: Denoised GI + Shadows
sequenceDiagram
participant App as Application
participant Shape as Shape Loader
participant Buf as Buffer Mgmt
participant RT as RT Pipelines
App->>Shape: BuildMesh(geometry, transform)
Shape->>Shape: ParseVertexLayout()
Shape->>Shape: CalculateVectors(normals/tangents)
Shape->>Buf: CreateBuffers(name)
Buf->>Buf: Allocate GPU buffers (Vertex/Triangle)
Buf->>Buf: Upload vertex data
Shape->>Shape: BuildMaterial(shader data)
Shape->>Shape: ExtractTextures()
Shape->>Shape: GetPBRShaderFlags()
App->>RT: CompileShaders()
RT->>RT: CompileComputeShaders()
RT->>RT: CompileRTGIShaders()
Note over App,RT: Model Ready
App->>RT: UpdateRenderSize()
RT->>Buf: SetupOutputRT() / SetupSharedRT()
Buf->>Buf: Create output textures & UAVs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Actionable Suggestions
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/DeferredCompositeCS.hlsl (1)
115-165: UninitializedssgiIlwhen RT ambient is disabled.When
RTis defined andSharedData::raytracingSettings.Ambient <= 0.0, the block containingSampleSSGI(lines 122) is skipped, butssgiIlis still used on line 164. This results in using an uninitialized variable.🔎 Proposed fix - initialize ssgiIl and conditionally apply
#if defined(SSGI) float ssgiAo; float3 ssgiIl; +# if defined(RT) + ssgiIl = 0; + ssgiAo = 1; +# endif # if defined(RT) if (SharedData::raytracingSettings.Ambient > 0.0) { # endifAlternatively, move line 164 inside the RT-guarded block:
# if defined(RT) } + linDiffuseColor += ssgiIl * linAlbedo; +# else + linDiffuseColor += ssgiIl * linAlbedo; # endif - linDiffuseColor += ssgiIl * linAlbedo; #endif
🟡 Minor comments (14)
src/Features/Raytracing/Utils.h-118-134 (1)
118-134: Missing case forDXGI_FORMAT_BC4_SNORM.
IsShareableFormatreturnsfalseforBC4_SNORM, butGetCompatibleFormathas no case for it and returns the original unshareable format. This may cause issues if BC4_SNORM textures are encountered. Also, remove unreachablebreakstatements.🔎 Suggested fix
static inline DXGI_FORMAT GetCompatibleFormat(DXGI_FORMAT format, bool recompress) { switch (format) { case DXGI_FORMAT_BC4_UNORM: return recompress ? DXGI_FORMAT_BC1_UNORM : DXGI_FORMAT_R8_UNORM; - break; + case DXGI_FORMAT_BC4_SNORM: + return recompress ? DXGI_FORMAT_BC1_SNORM : DXGI_FORMAT_R8_SNORM; case DXGI_FORMAT_BC7_UNORM: return recompress ? DXGI_FORMAT_BC3_UNORM : DXGI_FORMAT_R8G8B8A8_UNORM; - break; case DXGI_FORMAT_BC7_UNORM_SRGB: return recompress ? DXGI_FORMAT_BC3_UNORM_SRGB : DXGI_FORMAT_R8G8B8A8_UNORM_SRGB; - break; default: return format; - break; } }features/Raytracing/Shaders/Raytracing/Includes/Types/Skinning.hlsli-28-32 (1)
28-32: Missing bounds validation in GetBone.The
GetBonefunction performs bit-shifting based onidxwithout validating thatidx < 4. Invalid index values would produce incorrect results. Consider adding bounds checking or documenting the expected index range [0,3].Suggested fix
uint GetBone(uint idx) { + // Assumes idx in range [0,3] uint shift = idx * 8; return (bone >> shift) & 0xFF; }Or with clamping:
uint GetBone(uint idx) { - uint shift = idx * 8; + uint shift = min(idx, 3) * 8; return (bone >> shift) & 0xFF; }features/Raytracing/Shaders/Raytracing/Includes/MonteCarlo.hlsli-11-11 (1)
11-11: Minor typo: "bellow" → "below".- // The following functions bellow all come from NVidia + // The following functions below all come from NVidiasrc/Hooks.cpp-310-314 (1)
310-314: Check D3D11 device creation result before initializing D3D12.The
InitD3D12call occurs unconditionally afterptrD3D11CreateDeviceAndSwapChain, but if device creation fails,*ppDevicemay be invalid. Consider checkingretbefore proceeding.🔎 Proposed fix
- if (globals::features::raytracing.loaded) { - globals::features::raytracing.InitD3D12(*ppDevice, *ppImmediateContext, pAdapter); - } - - return ret; + if (SUCCEEDED(ret) && globals::features::raytracing.loaded) { + globals::features::raytracing.InitD3D12(*ppDevice, *ppImmediateContext, pAdapter); + } + + return ret;src/Features/Raytracing/ShaderUtils.cpp-1-1 (1)
1-1: Remove#pragma oncefrom source file.
#pragma onceis an include guard for header files and has no effect in.cppfiles. This should be removed.🔎 Proposed fix
-#pragma once - #include "ShaderUtils.h"src/Features/Raytracing/ShaderUtils.cpp-66-70 (1)
66-70: Check return value ofDxcCreateInstancefor compiler args.The
DxcCreateInstancecall forIDxcCompilerArgsdoesn't verify success, which could lead to null pointer access on line 69-70.🔎 Proposed fix
winrt::com_ptr<IDxcCompilerArgs> compilerArgs; - DxcCreateInstance(CLSID_DxcCompilerArgs, IID_PPV_ARGS(&compilerArgs)); + if (FAILED(DxcCreateInstance(CLSID_DxcCompilerArgs, IID_PPV_ARGS(&compilerArgs)))) { + logger::error("Failed to create DxcCompilerArgs"); + return; + } compilerArgs->AddArguments(args, _countof(args));features/Raytracing/Shaders/Raytracing/Includes/RT/CommonRT.hlsli-112-112 (1)
112-112: Typo in include guard closing comment.The closing comment says
COMMONRT_HLSIbut should beCOMMONRT_HLSLto match the opening guard.🔎 Proposed fix
-#endif // COMMONRT_HLSI +#endif // COMMONRT_HLSLfeatures/Raytracing/Shaders/Raytracing/FeatureData.hlsli-1-2 (1)
1-2: Typo in include guard macro name.The include guard uses
HLSIinstead ofHLSLI, which is inconsistent with the file extension and could cause confusion or conflicts with other headers.🔎 Proposed fix
-#ifndef RT_FEATUREDATA_HLSI -#define RT_FEATUREDATA_HLSI +#ifndef RT_FEATUREDATA_HLSLI +#define RT_FEATUREDATA_HLSLIfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Registers.hlsli-1-2 (1)
1-2: Typo in include guard name.The include guard uses
SVGF_REGISTERS_HLSIbut should likely beSVGF_REGISTERS_HLSLIto match the file extension convention.🔎 Proposed fix
-#ifndef SVGF_REGISTERS_HLSI -#define SVGF_REGISTERS_HLSI +#ifndef SVGF_REGISTERS_HLSLI +#define SVGF_REGISTERS_HLSLIsrc/Features/Raytracing/Pipelines/SHaRCPipeline.cpp-1-1 (1)
1-1:#pragma onceshould not be used in .cpp files.This directive is only meaningful in header files to prevent multiple inclusions. In a .cpp file, it has no effect and indicates a likely copy-paste error or misunderstanding.
🔎 Proposed fix
-#pragma once - #include "SHaRCPipeline.h"src/Features/Raytracing/RTPipelineBuilder.h-183-190 (1)
183-190: Typo: "Writting" → "Writing" in debug log messages.- logger::debug("[RT] Writting Raygen Records"); + logger::debug("[RT] Writing Raygen Records"); writeRecords(rayGenNames, shaderBindingTable.RayGen); - logger::debug("[RT] Writting Miss Records"); + logger::debug("[RT] Writing Miss Records"); writeRecords(missNames, shaderBindingTable.Miss); - logger::debug("[RT] Writting HitGroup Records"); + logger::debug("[RT] Writing HitGroup Records"); writeRecords(hitGroupNames, shaderBindingTable.HitGroup);src/Features/Raytracing/BSCubemapCamera.h-28-28 (1)
28-28: Typo in parameter name:apSceneNonde→apSceneNode- static BSCubeMapCamera* Create(NiNode* apSceneNonde, float afViewDistance, UInt32 auiTextureSize = 0, D3DFORMAT aeFormat = D3DFMT_UNKNOWN); + static BSCubeMapCamera* Create(NiNode* apSceneNode, float afViewDistance, UInt32 auiTextureSize = 0, D3DFORMAT aeFormat = D3DFMT_UNKNOWN);src/Features/Raytracing.h-194-204 (1)
194-204: Update placeholder feature summary before release.The feature summary contains placeholder text ("This is a terse description", "Cheese"). Update with actual feature description and subfeatures before merging.
src/Features/Raytracing.h-744-747 (1)
744-747: InitializeshadowLightto nullptr.
shadowLightis a raw pointer that is not initialized. If accessed beforeBSShadowDirectionalLight_RenderShadowmapshook assigns it, this could cause undefined behavior.🔎 Proposed fix
- RE::BSShadowDirectionalLight* shadowLight; + RE::BSShadowDirectionalLight* shadowLight = nullptr;
🧹 Nitpick comments (43)
features/Raytracing/Shaders/Raytracing/Includes/Types/ShadowsFrameData.hlsli (3)
15-16: Inconsistent indentation (tabs vs. spaces).Lines 15-16 use tab characters while the rest of the file uses spaces. Consider converting these to spaces for consistency.
4-17: Add documentation for struct fields and padding rationale.The struct lacks comments explaining the purpose of each field and the rationale for the two padding matrices (
Pad0,Pad1). Consider adding:
- A brief description of the struct's purpose
- Comments for each field explaining what it represents
- An explanation of why the padding matrices are needed (e.g., to meet constant buffer alignment requirements)
This improves maintainability and helps future developers understand the design.
18-20: Document the 256-byte alignment requirement.The static assertion enforces a 256-byte alignment constraint but doesn't explain why. Consider adding a comment explaining this requirement (e.g., DirectX 12 constant buffer alignment for optimal performance).
src/Features/Raytracing/Types.h (3)
13-16: Consider making the float constructor explicit.The non-explicit float constructor allows implicit conversions that may cause unintended precision loss when passing float values to half parameters. Making it explicit prevents accidental narrowing conversions.
🔎 Proposed fix
- half(const float& fv) + explicit half(const float& fv) { v = DirectX::PackedVector::XMConvertFloatToHalf(fv); }
49-136: Consider consistent operator support across half vector types.
half3providesoperator+=(line 99-105), buthalf2andhalf4do not. If arithmetic operations are useful forhalf3, they might be equally valuable forhalf2andhalf4to maintain a uniform interface.🔎 Proposed additions for half2 and half4
For
half2:half2& operator+=(const half2& rhs) { x += rhs.x; y += rhs.y; return *this; }For
half4:half4& operator+=(const half4& rhs) { x += rhs.x; y += rhs.y; z += rhs.z; w += rhs.w; return *this; }
138-163: Consider consistent comparison operators for uint vector types.
uint2providesoperator==andoperator!=(lines 143-144), butuint3anduint4do not. Adding these operators touint3anduint4would provide a uniform interface and could be useful for comparisons in collections or algorithms.🔎 Proposed additions for uint3 and uint4
For
uint3:bool operator==(const uint3&) const = default; bool operator!=(const uint3&) const = default;For
uint4:bool operator==(const uint4&) const = default; bool operator!=(const uint4&) const = default;features/Raytracing/Shaders/Raytracing/CubeToHemiCS.hlsl (1)
6-6: Consider parameterizing the resolution.The hardcoded
Resolution = 512limits flexibility. If this shader needs to work with different output sizes, consider passing the resolution via a constant buffer.🔎 Example parameterization approach
+cbuffer ResolutionBuffer : register(b0) +{ + uint Resolution; +} + -static const uint Resolution = 512;src/Features/Raytracing/Utils.h (4)
8-16: Consider usingstatic_castand clamping inputs.C-style casts are less safe than
static_cast. Additionally, values outside the 0-1 range will produce unexpected results due to truncation after masking.🔎 Suggested improvement
static inline uint PackUByte4(float4 unpacked) { - auto x = (uint)(unpacked.x * 255.0f) & 0xFF; - auto y = (uint)(unpacked.y * 255.0f) & 0xFF; - auto z = (uint)(unpacked.z * 255.0f) & 0xFF; - auto w = (uint)(unpacked.w * 255.0f) & 0xFF; + auto x = static_cast<uint>(std::clamp(unpacked.x, 0.0f, 1.0f) * 255.0f); + auto y = static_cast<uint>(std::clamp(unpacked.y, 0.0f, 1.0f) * 255.0f); + auto z = static_cast<uint>(std::clamp(unpacked.z, 0.0f, 1.0f) * 255.0f); + auto w = static_cast<uint>(std::clamp(unpacked.w, 0.0f, 1.0f) * 255.0f); return (w << 24) | (z << 16) | (y << 8) | x; }
97-116: Remove unreachablebreakstatements.The
breakstatements after eachreturnare dead code and will never execute.🔎 Suggested fix
static inline bool IsShareableFormat(DXGI_FORMAT format) { switch (format) { case DXGI_FORMAT_BC4_UNORM: - return false; - break; case DXGI_FORMAT_BC4_SNORM: - return false; - break; case DXGI_FORMAT_BC7_UNORM: - return false; - break; case DXGI_FORMAT_BC7_UNORM_SRGB: return false; - break; default: return true; - break; } }
190-191: Remove commented-out code.Per coding guidelines, commented-out code should be removed. If this logic is needed in the future, it can be retrieved from version control.
🔎 Suggested fix
- //if (pathLower.ends_with("_d.dds")) - // return true; - if (pathLower.ends_with("_n.dds"))
214-222: Addinlinefor consistency with other functions.These functions are marked
staticbut lackinline, unlike all other functions in this header. Addinginlinemaintains consistency and clarifies intent.🔎 Suggested fix
-static uint32_t DivideRoundUp(uint32_t x, uint32_t divisor) +static inline uint32_t DivideRoundUp(uint32_t x, uint32_t divisor) { return (x + divisor - 1) / divisor; } -static uint32_t DivideRoundUp(uint32_t x, float divisor) +static inline uint32_t DivideRoundUp(uint32_t x, float divisor) { return static_cast<uint32_t>(ceil(x / divisor)); }features/Raytracing/Shaders/Raytracing/Shadows/RayGeneration.hlsl (2)
43-46: Consider named constants for ray parameters.The magic numbers (0.1f offset, 0.01f TMin, 1e30 TMax) are standard for ray tracing but would benefit from named constants for maintainability and documentation.
Suggested named constants
Add to a common header:
#define SHADOW_RAY_OFFSET 0.1f #define RAY_TMIN 0.01f #define RAY_TMAX 1e30Then use:
- ray.Origin = positionWS + Direction * 0.1f; + ray.Origin = positionWS + Direction * SHADOW_RAY_OFFSET; ray.Direction = Direction; - ray.TMin = 0.01f; - ray.TMax = 1e30; + ray.TMin = RAY_TMIN; + ray.TMax = RAY_TMAX;
51-51: Complex ray flags could use documentation.The combination of four ray flags on a single line reduces readability. Consider adding a comment explaining the rationale or extracting to a named constant.
Suggested improvement
+ // Shadow rays: accept first hit, skip closest hit shader, cull non-opaque, skip procedurals TraceRay(Scene, RAY_FLAG_ACCEPT_FIRST_HIT_AND_END_SEARCH | RAY_FLAG_SKIP_CLOSEST_HIT_SHADER | RAY_FLAG_CULL_NON_OPAQUE | RAY_FLAG_SKIP_PROCEDURAL_PRIMITIVES, 0xFF, 0, 0, 0, ray, payload);Or define a constant:
#define SHADOW_RAY_FLAGS (RAY_FLAG_ACCEPT_FIRST_HIT_AND_END_SEARCH | RAY_FLAG_SKIP_CLOSEST_HIT_SHADER | RAY_FLAG_CULL_NON_OPAQUE | RAY_FLAG_SKIP_PROCEDURAL_PRIMITIVES)features/Raytracing/Shaders/Raytracing/RTShadowsCS.hlsl (2)
40-43: Consider named constants for ray parameters.Same magic numbers as RayGeneration.hlsl (0.1f offset, 0.01f TMin, 1e30 TMax). These should be extracted to shared named constants for consistency and maintainability across the raytracing shaders.
53-60: Unconventional shadow mask encoding.The shadow mask uses black (0,0,0) for shadow and red (1,0,0) for no shadow, storing only in the red channel. Consider using a more conventional single-channel output with 0.0/1.0 values for shadow/no-shadow, which would be clearer and potentially more efficient.
Suggested alternative
if (q.CommittedStatus() == COMMITTED_TRIANGLE_HIT) { - ShadowMask[id] = float4(0.0f, 0.0f, 0.0f, 1.0f); + ShadowMask[id] = float4(0.0f, 0.0f, 0.0f, 1.0f); // Shadow } else { - ShadowMask[id] = float4(1.0f, 0.0f, 0.0f, 1.0f); + ShadowMask[id] = float4(1.0f, 1.0f, 1.0f, 1.0f); // No shadow }Or use single channel if the texture format supports it.
features/Raytracing/Shaders/Raytracing/Includes/Types/FrameData.hlsli (1)
22-24: Alignment assertion is inconsistent withalignas(16).The struct uses
alignas(16)but the static_assert only checkssizeof(SHaRCFrameData) % 4 == 0. Consider strengthening the assertion to match the alignment requirement.🔎 Suggested improvement
#ifdef __cplusplus -static_assert(sizeof(SHaRCFrameData) % 4 == 0); +static_assert(sizeof(SHaRCFrameData) % 16 == 0); #endiffeatures/Raytracing/Shaders/Raytracing/Includes/MonteCarlo.hlsli (1)
386-391: Remove or document commented-out code.The commented-out PDF calculation code creates maintenance burden. Either remove it if superseded, or add a comment explaining why it's preserved for reference.
features/Raytracing/Shaders/Raytracing/Includes/RT/ShadowPayload.hlsli (1)
4-7: Consider using a more explicit type for the shadow test result.The
float missedmember works functionally, but abooloruintwould more clearly express the binary nature of shadow testing (hit/miss). In DXR, payload sizes are optimized, and a smaller type might be more appropriate.🔎 Proposed refactor
struct ShadowPayload { - float missed; + bool missed; };features/Raytracing/Shaders/Raytracing/GI/AnyHit.hlsl (2)
21-26: Consider using computed LOD for texture sampling.The shader samples at LOD 0 regardless of ray hit distance. For distant hits, this can cause aliasing and unnecessary texture bandwidth. Consider computing an appropriate LOD based on ray differential or distance.
🔎 Suggested improvement
- float alpha = Textures[NonUniformResourceIndex(material.BaseTexture)].SampleLevel(BaseSampler, texCoord, 0).a; + // Compute LOD based on ray distance or use SampleGrad with ray differentials + float lod = log2(RayTCurrent() * 0.1); // Example: adjust multiplier as needed + float alpha = Textures[NonUniformResourceIndex(material.BaseTexture)].SampleLevel(BaseSampler, texCoord, lod).a;Based on learnings, ensure proper texture sampling in raytracing shaders.
23-23: Hardcoded alpha cutoff may not match game materials.The 0.5 alpha threshold is hardcoded, but different materials in the game may use different alpha test thresholds. Consider storing the alpha test threshold in the Material struct and using it here for consistency with the rasterization path.
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TempAccCS.hlsl (1)
5-9: Implement the temporal accumulation shader.The compute shader body is empty. This is a critical component of the SVGF denoiser that accumulates samples across frames. The shader should:
- Read current frame samples
- Reproject previous frame data
- Validate reprojection using depth/normal tests
- Blend current and previous samples with appropriate weights
- Write accumulated output
Would you like me to provide a reference implementation or open an issue to track this work?
features/Raytracing/Shaders/Raytracing/GI/Miss.hlsl (1)
3-7: Implement miss shader to return environment lighting.The miss shader is empty, so rays that miss geometry will contribute no lighting. For Global Illumination, the miss shader should:
- Sample the sky/environment map
- Return appropriate radiance based on ray direction
- Consider sun/ambient contributions
This is critical for realistic GI as missed rays represent incoming light from the environment.
Would you like me to provide a reference implementation that samples the game's sky data, or open an issue to track this work?
features/Raytracing/Shaders/Raytracing/ConvertNormalGlossCS.hlsl (1)
12-22: Add bounds checking to prevent out-of-bounds texture access.The compute shader does not validate that
idis within texture dimensions before reading/writing. If dispatch dimensions exceed texture size, this causes undefined behavior.🔎 Add bounds check
+uint2 texDim; +NormalRoughness.GetDimensions(texDim.x, texDim.y); + [numthreads(8, 8, 1)] void main(uint2 id : SV_DispatchThreadID) { + if (any(id >= texDim)) return; + const unorm half3 normalGlossiness = NormalGlossiness[id].xyz; const snorm half3 normalWS = normalize(ViewToWorldVector(GBuffer::DecodeNormal(normalGlossiness.xy), FrameBuffer::CameraViewInverse[0])); NormalRoughness[id] = half4(normalWS, 1.0f - normalGlossiness.z);features/Raytracing/Shaders/Raytracing/CopyDepthCS.hlsl (1)
4-8: Add bounds checking for texture dimensions.The shader assumes dispatch dimensions match texture dimensions. Add a bounds check to prevent out-of-bounds access when dimensions don't match.
🔎 Add bounds check
+uint2 texDim; +DepthOut.GetDimensions(texDim.x, texDim.y); + [numthreads(8, 8, 1)] void main(uint2 id : SV_DispatchThreadID) { + if (any(id >= texDim)) return; DepthOut[id] = DepthIn[id]; }features/Raytracing/Shaders/Raytracing/PT/RayGeneration.hlsl (2)
29-37: Consider reducingray.TMinfor close geometry.A
TMinof 0.1 units may cause near-camera geometry to be missed, potentially creating visual holes for objects close to the camera. Consider a smaller value (e.g., 0.001–0.01) unless there's a specific self-intersection mitigation reason.
39-42: Remove or implement commented reflectance texture logic.The commented
ReflectanceTextureassignment suggests incomplete functionality. Per coding guidelines, avoid leaving TODO/placeholder code—either implement or remove it.src/Features/Raytracing/ShaderUtils.cpp (1)
11-14: Consider handling COM lifecycle more carefully.
CoInitializeExis called butCoUninitializeis never called. While this may work if COM is already initialized elsewhere, repeated calls without matching uninitialize can cause reference count issues. Consider using RAII or checking the return value forS_FALSE(already initialized).features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (1)
180-183: Minor formatting inconsistency.Line 183 has a space before
#endifwhich is inconsistent with the rest of the file.🔎 Proposed fix
#if defined(SHARC) SharcState sharcState; SharcHitData sharcHitData; - #endif +#endiffeatures/Raytracing/Shaders/Raytracing/ShadowsRT.hlsl (1)
49-49:RAY_FLAG_SKIP_CLOSEST_HIT_SHADERmakes the ClosestHit shader dead code.The
RAY_FLAG_SKIP_CLOSEST_HIT_SHADERflag prevents theClosestHitshader from executing. Sincepayload.missedis initialized to0.0and only theMissshader sets it to1.0, the logic is correct for shadow testing. However, theClosestHitshader (lines 60-64) will never be invoked and can be removed.🔎 Proposed simplification
Remove the unused ClosestHit shader:
-[shader("closesthit")] -void ClosestHit(inout Payload payload, in BuiltInTriangleIntersectionAttributes attribs) -{ - payload.missed = 0.0f; -}features/Raytracing/Shaders/Raytracing/Includes/Types/Instance.hlsli (1)
4-8:Dataarray is not zero-initialized.The default constructor
LightData() = defaultleavesData[4]uninitialized. IfCountis less than 16, the unused portions ofDatacontain garbage which could cause issues when the struct is copied to GPU memory or compared.🔎 Proposed fix
struct LightData { - uint Count; - uint Data[4]; + uint Count = 0; + uint Data[4] = {0, 0, 0, 0};Or initialize in the constructor:
- LightData() = default; + LightData() : Count(0), Data{0, 0, 0, 0} {}features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli (1)
99-106: Consider explicit rounding inUnpackMAO.Line 102 implicitly truncates
packed * 65535.0to uint. For robustness against floating-point precision issues, use explicit rounding.🔎 Proposed fix
void UnpackMAO(float packed, out float metalness, out float ao) { - uint metalnessAO = packed * 65535.0; + uint metalnessAO = (uint)round(packed * 65535.0); metalness = saturate((metalnessAO & 0xFF) / 255.0f); ao = saturate(((metalnessAO >> 8) & 0xFF) / 255.0f); }src/Features/Raytracing/Allocator.h (1)
17-18: Default move operations may lead to double-free.After moving an
Allocation, the source object retains a validallocatorpointer. IfFreeAllocation()is called on the moved-from object (e.g., by a destructor), it would free the slot again.🔎 Proposed fix
- Allocation(Allocation&&) = default; - Allocation& operator=(Allocation&&) = default; + Allocation(Allocation&& other) noexcept + : index(other.index), allocator(other.allocator) + { + other.allocator = nullptr; + } + + Allocation& operator=(Allocation&& other) noexcept + { + if (this != &other) { + index = other.index; + allocator = other.allocator; + other.allocator = nullptr; + } + return *this; + }And guard
FreeAllocation():void FreeAllocation() const { if (allocator) allocator->Free(index); }features/Raytracing/Shaders/Raytracing/SkinningCS.hlsl (1)
61-84: Commented-out SKINNED block has bugs that will surface when enabled.Two issues in the commented code that should be fixed before uncommenting:
- Line 63:
skinning[v]uses undefined variablev— should beskinning[id.x]- Line 65:
boneMatrix += ...uses+=on an uninitializedfloat3x4; initialize to zero firstSuggested fix for when this block is enabled
if (updateData.flags & SKINNED) { - Skinning vertSkinning = skinning[v]; + Skinning vertSkinning = skinning[id.x]; - float3x4 boneMatrix; + float3x4 boneMatrix = (float3x4)0; uint boneMatrixBase = id.y * MAX_BONES;src/Features/Raytracing/Shape.cpp (1)
580-585: Division-by-zero handling produces unusual tangent scaling.When
f == 0, settingf = 1.0fand then computingf = 1.0f / fresults inf = 1.0f, which means degenerate UV triangles get full-scale tangent contribution. Consider setting the reciprocal directly or skipping the triangle's tangent contribution entirely.Suggested clarification
float f = deltaUV1.x * deltaUV2.y - deltaUV2.x * deltaUV1.y; - if (f == 0.0f) - f = 1.0f; - - f = 1.0f / f; + if (abs(f) < 1e-8f) + continue; // Skip degenerate UV triangles + f = 1.0f / f;features/Raytracing/Shaders/Raytracing/Includes/Surface.hlsli (1)
192-196: Minor formatting inconsistency in preprocessor directives.-# ifdef DEBUG_WHITE_FURNACE +#ifdef DEBUG_WHITE_FURNACE surface.Albedo = float3(1.0f, 1.0f, 1.0f); -# else +#else surface.Albedo = albedo; - # endif +#endifsrc/Features/Raytracing/HeapManager.h (1)
9-13: Unused conceptEnumUInt32- either apply it or remove it.The concept is defined but the template declarations use plain
typename Tinstead:template <typename T> // Could use: template <EnumUInt32 T> struct DescriptorDescEither apply the concept to enforce type constraints or remove the unused definition.
features/Raytracing/Shaders/Raytracing/Includes/RT/SHaRC/SharcCommon.h (1)
310-312: Pre-increment inmin()expression - consider separating for clarity.sharcState.cacheIndices[0] = cacheIndex; sharcState.sampleWeights[0] = SharcSampleWeight(1.0f / materialDemodulation); - sharcState.pathLength = min(++sharcState.pathLength, SHARC_PROPAGATION_DEPTH - 1); + sharcState.pathLength++; + sharcState.pathLength = min(sharcState.pathLength, SHARC_PROPAGATION_DEPTH - 1);However, since this is vendor code from NVIDIA, keeping the original form may be preferable for easier upstream updates.
src/Features/Raytracing/Shape.h (3)
21-28: Consider namespacing theFlagsenum to avoid collisions.Defining
Flagsat global scope risks naming collisions with other code. Consider wrapping it in a namespace or moving it inside theShapeclass.🔎 Suggested refactor
-enum Flags : uint8_t +namespace ShapeFlags { - None = 0, - Alpha = 1 << 0, - Dynamic = 1 << 1, - Skinned = 1 << 2 -}; -DEFINE_ENUM_FLAG_OPERATORS(Flags); + enum Flags : uint8_t + { + None = 0, + Alpha = 1 << 0, + Dynamic = 1 << 1, + Skinned = 1 << 2 + }; + DEFINE_ENUM_FLAG_OPERATORS(Flags); +} +using ShapeFlags::Flags;
84-101: Inefficient O(n×m) flag matching via string comparison.The nested loop compares enum names via
magic_enumfor every flag pair. This runs on everyGetShaderFlags()call and could be slow. Consider a compile-time mapping or direct flag translation.🔎 Suggested alternative using direct flag checks
ShaderFlags GetShaderFlags() const { auto shaderFlagsLocal = ShaderFlags::None; - - const auto& entries = magic_enum::enum_entries<ShaderFlags>(); - const auto& originalEntries = magic_enum::enum_entries<RE::BSShaderProperty::EShaderPropertyFlag>(); - - for (const auto& [flag, name] : entries) { - for (const auto& [originalFlag, originalName] : originalEntries) { - if (shaderFlags.any(originalFlag) && name == originalName) { - shaderFlagsLocal |= flag; - break; - } - } - } - + if (shaderFlags.any(RE::BSShaderProperty::EShaderPropertyFlag::kTempRefraction)) + shaderFlagsLocal |= ShaderFlags::kTempRefraction; + if (shaderFlags.any(RE::BSShaderProperty::EShaderPropertyFlag::kVertexAlpha)) + shaderFlagsLocal |= ShaderFlags::kVertexAlpha; + // ... add remaining flags return shaderFlagsLocal; }
161-188: Remove commented-out dead code.The commented-out constructor, destructor, and
Clonemethod add noise. If these are no longer needed, remove them; if they're planned features, track them in an issue instead.features/Raytracing/Shaders/Raytracing/Includes/RT/SHaRC/HashGridCommon.h (2)
229-246: Unusedsuccessfulvariables in entry functions.
HashMapInsertEntryandHashMapFindEntryboth assign tosuccessfulbut never use the result. If the success status isn't needed, consider removing the variable or adding a comment explaining the intent.🔎 Proposed fix
HashGridIndex HashMapInsertEntry(in HashMapData hashMapData, float3 samplePosition, float3 sampleNormal, HashGridParameters gridParameters) { HashGridIndex cacheIndex = HASH_GRID_INVALID_CACHE_INDEX; const HashGridKey hashKey = HashGridComputeSpatialHash(samplePosition, sampleNormal, gridParameters); - bool successful = HashMapInsert(hashMapData, hashKey, cacheIndex); + HashMapInsert(hashMapData, hashKey, cacheIndex); // Result intentionally ignored return cacheIndex; } HashGridIndex HashMapFindEntry(in HashMapData hashMapData, float3 samplePosition, float3 sampleNormal, HashGridParameters gridParameters) { HashGridIndex cacheIndex = HASH_GRID_INVALID_CACHE_INDEX; const HashGridKey hashKey = HashGridComputeSpatialHash(samplePosition, sampleNormal, gridParameters); uint hashCollisionsNum; - bool successful = HashMapFind(hashMapData, hashKey, cacheIndex, hashCollisionsNum); + HashMapFind(hashMapData, hashKey, cacheIndex, hashCollisionsNum); // Result intentionally ignored return cacheIndex; }
289-296: UnusedgridLevelvariable in debug function.
gridLevelis computed but never used inHashGridDebugHashCollisions.🔎 Proposed fix
float3 HashGridDebugHashCollisions(float3 samplePosition, float3 sampleNormal, HashGridParameters gridParameters, HashMapData hashMapData) { HashGridKey hashKey = HashGridComputeSpatialHash(samplePosition, sampleNormal, gridParameters); - uint gridLevel = HashGridGetLevel(samplePosition, gridParameters); HashGridIndex cacheIndex = HASH_GRID_INVALID_CACHE_INDEX; uint hashCollisionsNum; HashMapFind(hashMapData, hashKey, cacheIndex, hashCollisionsNum);src/Features/Raytracing.h (1)
305-308: Remove unnecessaryconstqualifier on return type.
const boolas a return type is redundant since the returned value is a copy. Theconsthas no effect.🔎 Proposed fix
- const bool Active() + bool Active() const { return loaded && settings.Enabled; - }; + }Note: Also made the method
constsince it doesn't modify state, and removed the trailing semicolon after the closing brace.
| MaterialData GetData() | ||
| { | ||
| return MaterialData( | ||
| BaseColor, EffectColor, | ||
| TexCoordOffsetScale, | ||
| RoughnessScale, SpecularLevel, | ||
| BaseTexture->GetIndex(), | ||
| NormalTexture->GetIndex(), | ||
| EffectTexture->GetIndex(), | ||
| RMAOSTexture->GetIndex(), | ||
| GetShaderType(), | ||
| GetShaderFlags(), | ||
| static_cast<uint16_t>(Feature), | ||
| PBRFlags.underlying()); | ||
| } |
There was a problem hiding this comment.
Potential null pointer dereference on texture allocations.
GetData() calls GetIndex() on BaseTexture, NormalTexture, EffectTexture, and RMAOSTexture without verifying they are non-null. If any allocation failed or was never set, this will crash.
🔎 Proposed defensive check
MaterialData GetData()
{
+ // Ensure all texture allocations are valid
+ if (!BaseTexture || !NormalTexture || !EffectTexture || !RMAOSTexture) {
+ // Handle error: return default or throw
+ return MaterialData{};
+ }
return MaterialData(
BaseColor, EffectColor,
TexCoordOffsetScale,
RoughnessScale, SpecularLevel,
BaseTexture->GetIndex(),
NormalTexture->GetIndex(),
EffectTexture->GetIndex(),
RMAOSTexture->GetIndex(),
GetShaderType(),
GetShaderFlags(),
static_cast<uint16_t>(Feature),
PBRFlags.underlying());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MaterialData GetData() | |
| { | |
| return MaterialData( | |
| BaseColor, EffectColor, | |
| TexCoordOffsetScale, | |
| RoughnessScale, SpecularLevel, | |
| BaseTexture->GetIndex(), | |
| NormalTexture->GetIndex(), | |
| EffectTexture->GetIndex(), | |
| RMAOSTexture->GetIndex(), | |
| GetShaderType(), | |
| GetShaderFlags(), | |
| static_cast<uint16_t>(Feature), | |
| PBRFlags.underlying()); | |
| } | |
| MaterialData GetData() | |
| { | |
| // Ensure all texture allocations are valid | |
| if (!BaseTexture || !NormalTexture || !EffectTexture || !RMAOSTexture) { | |
| // Handle error: return default or throw | |
| return MaterialData{}; | |
| } | |
| return MaterialData( | |
| BaseColor, EffectColor, | |
| TexCoordOffsetScale, | |
| RoughnessScale, SpecularLevel, | |
| BaseTexture->GetIndex(), | |
| NormalTexture->GetIndex(), | |
| EffectTexture->GetIndex(), | |
| RMAOSTexture->GetIndex(), | |
| GetShaderType(), | |
| GetShaderFlags(), | |
| static_cast<uint16_t>(Feature), | |
| PBRFlags.underlying()); | |
| } |
🤖 Prompt for AI Agents
In src/Features/Raytracing/Shape.h around lines 120-134, GetData() dereferences
BaseTexture, NormalTexture, EffectTexture and RMAOSTexture without null checks
which can crash if any are null; update the method to defensively check each
texture pointer before calling GetIndex() and use a safe fallback index (e.g. an
invalid/empty-texture index or 0) or log/assert and handle the error path so
MaterialData is constructed with valid indices; ensure the checks are concise
(conditional retrieves or helper that returns default index when null) and
preserve existing returned fields and ordering.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl (2)
1-4: Missing Color.hlsli include for color space conversions.The file does not include
"Common/Color.hlsli", which providesColor::GammaToTrueLinear()and related color space conversion functions required for proper linear space handling in the raytracing pipeline. This omission is directly related to the unresolved color space conversion issue in lines 19-21 (see duplicate comment below).🔎 Add the missing include
#include "Raytracing/Includes/Common.hlsli" #include "Common/FrameBuffer.hlsli" #include "Common/GBuffer.hlsli" +#include "Common/Color.hlsli"
19-21: Critical: Color space conversion still missing (duplicate issue).This issue was previously flagged but remains unresolved. The Albedo texture is multiplied by
(1.0f - metallic)without converting from gamma to linear space. The raytracing pipeline operates in true linear space, as confirmed byGI/RayGeneration.hlslwhich usesColor::GammaToTrueLinear()when loading albedo.Based on learnings, proper linear color space handling is essential for raytracing shaders.
The previous review comment provides the correct fix and includes instructions for AI agents to apply it.
🧹 Nitpick comments (9)
src/Features/Raytracing.h (9)
379-379: Address or remove TODO comment.Per coding guidelines, avoid leaving TODO placeholders. Either rename
DiffuseBRDFtoReflectanceModelnow, or create an issue to track this task and remove the comment.Would you like me to open an issue to track this renaming task?
630-631: Remove commented-out code.Per coding guidelines, remove commented-out code rather than leaving it in the codebase. If this frame checking logic might be needed later, document the reason in a comment or commit message.
669-669: Address or track TODO for skinned meshes.Per coding guidelines, avoid leaving TODO placeholders without tracking.
Would you like me to open an issue to track the skinned mesh handling implementation?
626-687: Consider extracting update logic into smaller methods.The
Instance::Updatemethod is quite long (~60 lines) with multiple nested conditionals and responsibilities (frame checking, dynamic vertex updates, skinned mesh handling).Suggested refactor
Consider extracting helper methods:
bool UpdateDynamicShape(Shape* shape); bool UpdateSkinnedShape(Shape* shape); void QueueShapeForUpdate(const eastl::string& path, Shape* shape, Flags updateFlags);This would improve readability and testability.
965-972: Remove commented-out code.Per coding guidelines, remove commented-out code rather than leaving it in place.
1234-1250: Remove commented-out template code.Per coding guidelines, remove commented-out code blocks. If this
Load3DBasetemplate might be needed later, track it in an issue instead.
1387-1411: Replace generic function names with descriptive names.The hooks
sub_7FF62400F840andsub_7FF62400F3D0have non-descriptive names that appear to be raw addresses. Document what these functions do or use more meaningful names.If the actual function names are unknown, add comments explaining their purpose and when they're called.
903-996: Consider extracting texture conversion logic.The
ID3D11Device_CreateTexture2D::thunkmethod is ~90 lines long with complex texture format conversion, decompression, and recompression logic.Suggested refactor
Extract helper methods such as:
static bool ShouldShareTexture(const D3D11_TEXTURE2D_DESC* pDesc, const D3D11_SUBRESOURCE_DATA* pInitialData); static HRESULT ConvertTextureFormat(const D3D11_TEXTURE2D_DESC* pDesc, const D3D11_SUBRESOURCE_DATA* pInitialData, D3D11_TEXTURE2D_DESC& outDesc, eastl::vector<D3D11_SUBRESOURCE_DATA>& outData); static HRESULT CreateSharedTexture(ID3D11Device* device, ID3D11Texture2D* texture);This would improve readability and testability.
1309-1326: Consider reducing log verbosity in hot paths.Several hooks contain
logger::infoorlogger::warncalls that might be invoked frequently during gameplay (e.g., in Enable/Disable/Clone methods). Consider usinglogger::traceorlogger::debugfor less critical information, or gating these with a debug flag.Also applies to: 1348-1376, 1391-1406
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing.cppsrc/Features/Raytracing.h
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing.h
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Raytracing.h
🧠 Learnings (4)
📓 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.
📚 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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (3)
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl (2)
5-10: LGTM: Texture declarations follow codebase conventions.The texture declarations use appropriate types (
unorm half4,snorm half4) for bandwidth optimization and follow the established convention of hardcoded register slots.
15-17: LGTM: Normal and roughness processing is correct.The normal decoding, world-space transformation, and glossiness-to-roughness conversion (standard PBR:
roughness = 1.0 - glossiness) are implemented correctly. The use of half precision is appropriate for raytracing performance optimization.src/Features/Raytracing.h (1)
207-207: Verify VR support decision.The feature explicitly returns
falseforSupportsVR(). Given this is a raytracing feature for Skyrim which has a VR version, verify this is intentional and document the reason if VR support is not feasible.Is there a technical limitation preventing VR support, or is this planned for future implementation?
| #define SHARC | ||
| #define DLSS_RR |
There was a problem hiding this comment.
Move feature flags to build system or wrap in include guards.
Unconditional #define directives in a header will affect all translation units that include this file. This can lead to unintended behavior if other code expects these features to be conditionally enabled.
Recommended approach
Move these to CMakeLists.txt as compile definitions:
target_compile_definitions(${PROJECT_NAME} PRIVATE SHARC DLSS_RR)Or wrap them with include guards if they must remain here:
+#ifndef RAYTRACING_FEATURE_FLAGS_DEFINED
+#define RAYTRACING_FEATURE_FLAGS_DEFINED
#define SHARC
#define DLSS_RR
+#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define SHARC | |
| #define DLSS_RR | |
| #ifndef RAYTRACING_FEATURE_FLAGS_DEFINED | |
| #define RAYTRACING_FEATURE_FLAGS_DEFINED | |
| #define SHARC | |
| #define DLSS_RR | |
| #endif |
🤖 Prompt for AI Agents
In src/Features/Raytracing.h around lines 3 to 4, the unconditional "#define
SHARC" and "#define DLSS_RR" will leak feature flags into every translation
unit; remove these raw defines from the header and instead add them as compile
definitions in your build system (e.g., target_compile_definitions for the
project target) or, if they must remain in-source, guard them with include
checks (wrap each define with an #ifndef ... #define ... #endif) so they only
define when not already provided by the build configuration.
|
|
||
| #include "Raytracing/Denoiser/SVGF/SVGF.hlsli" | ||
|
|
||
| #define NTDDI_VERSION NTDDI_WINBLUE |
There was a problem hiding this comment.
Avoid redefining system macros.
Defining NTDDI_VERSION in the middle of a header can conflict with platform headers that may have already been included or will be included later.
Move this to a PCH or ensure it's defined before any Windows headers are included, or remove it if it's not strictly necessary.
🤖 Prompt for AI Agents
In src/Features/Raytracing.h around line 46, the header unconditionally defines
NTDDI_VERSION which can clash with other platform headers; remove this macro
from this header and instead define it once in a central location that is
included before any Windows/platform headers (e.g., the project precompiled
header or a top-level config header), or if absolutely required keep a guarded
definition in that central header (use a #ifndef check there) so the macro is
not redefined mid-translation unit.
| # pragma warning(pop) | ||
| #endif | ||
|
|
||
| using namespace magic_enum::bitwise_operators; |
There was a problem hiding this comment.
Remove using namespace directive from header.
The using namespace magic_enum::bitwise_operators; directive in a header pollutes the global namespace for all translation units that include this file.
Recommended fix
Remove the directive and use explicit qualification:
-using namespace magic_enum::bitwise_operators;Then in implementation files that need it:
using namespace magic_enum::bitwise_operators;Or use explicit magic_enum::bitwise_operators::operator| calls where needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using namespace magic_enum::bitwise_operators; |
🤖 Prompt for AI Agents
In src/Features/Raytracing.h around line 64, remove the global using directive
"using namespace magic_enum::bitwise_operators;" because it pollutes every
translation unit that includes this header; instead delete that line and update
callers to either qualify bitwise operators with
magic_enum::bitwise_operators::operator| (or other operators) or add "using
namespace magic_enum::bitwise_operators;" only in the .cpp files that require it
so the header no longer exports the namespace globally.
| virtual inline std::string GetFeatureModLink() override { return MakeNexusModURL("999999"); } | ||
| virtual inline std::pair<std::string, std::vector<std::string>> GetFeatureSummary() override | ||
| { | ||
| return { | ||
| "This is a terse description.", | ||
| { | ||
| "This is a subfeature.", | ||
| "This is another subfeature.", | ||
| "Cheese.", | ||
| } | ||
| }; |
There was a problem hiding this comment.
Update placeholder metadata before release.
The feature summary contains placeholder text ("This is a terse description.", "Cheese.") and the mod link uses a placeholder ID "999999".
These should be updated with actual descriptions and a valid Nexus mod ID before the feature is released.
🤖 Prompt for AI Agents
In src/Features/Raytracing.h around lines 193 to 203, the GetFeatureModLink and
GetFeatureSummary methods contain placeholder metadata
(MakeNexusModURL("999999") and placeholder summary strings like "This is a terse
description." and "Cheese."); replace the Nexus ID with the real mod ID and
update the summary/ subfeature strings to the final concise description and real
subfeatures for this raytracing feature, ensuring the returned strings
accurately reflect the feature's purpose and remove any placeholder words before
release.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl (1)
20-22: Unusedaovariable.The
aovalue is extracted viaUnpackMAObut never used. If ambient occlusion is intentionally unused in this conversion pass (handled elsewhere in the RT pipeline), consider using a discard pattern to clarify intent:- float metallic, ao; - UnpackMAO(GNMAO[id].z, metallic, ao); + float metallic, _; + UnpackMAO(GNMAO[id].z, metallic, _);Alternatively, if AO is planned for future use, a brief comment would help clarify.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
🧠 Learnings (10)
📓 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.
📚 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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
🔇 Additional comments (1)
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl (1)
1-22: Color space conversion is correctly applied.The
Color::GammaToTrueLinearconversion on the albedo input (line 22) properly addresses the linear color space requirements for the raytracing pipeline. This aligns with howGI/RayGeneration.hlslhandles albedo, ensuring consistent color space handling throughout the RT subsystem. Based on learnings, this follows the expected pattern for linear lighting system compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli (1)
84-97: Fix confirmed:Get2Dnow correctly propagates seed advancement.The function signature has been updated to
inout uint seed, which resolves the issue flagged in the previous review. The seed advancement inSamplerCorenow correctly persists across calls.
🧹 Nitpick comments (1)
features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli (1)
33-36: Optional: Remove redundant outer clamp.The outer
clampis redundant sincesaturate(x)already ensuresx ∈ [0,1], and the linear interpolationmin + x * (max - min)mathematically stays within[min, max]. The clamp is defensive but unnecessary.🔎 Proposed simplification
float Remap(float x, float min, float max) { - return clamp(min + saturate(x) * (max - min), min, max); + return min + saturate(x) * (max - min); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/Includes/Common.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:
features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli
🧠 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.
📚 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:
features/Raytracing/Shaders/Raytracing/Includes/Common.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:
features/Raytracing/Shaders/Raytracing/Includes/Common.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 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:
features/Raytracing/Shaders/Raytracing/Includes/Common.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:
features/Raytracing/Shaders/Raytracing/Includes/Common.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 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:
features/Raytracing/Shaders/Raytracing/Includes/Common.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:
features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli
🔇 Additional comments (8)
features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli (8)
9-12: LGTM!The screen-to-view depth conversion follows standard perspective projection mathematics.
14-31: LGTM!The coordinate space transformation functions are correctly implemented.
ViewToWorldVectorappropriately uses only the 3×3 portion of the matrix for direction vectors.
43-55: LGTM!The octahedral normal decoding is correctly implemented with appropriate DX11 fallback for platforms that don't support the
selectintrinsic. The final negation aligns with the project's coordinate system conventions.
57-69: LGTM!The normal mapping implementation correctly transforms tangent-space normals to world space and re-orthogonalizes the tangent basis using Gram-Schmidt orthogonalization. The handedness application ensures proper bitangent orientation.
71-82: LGTM!The hash function is well-chosen from the hash-prospector collection and properly documented with its source and quality metrics.
99-106: LGTM!The bit unpacking correctly extracts metalness and ambient occlusion from the packed format, with appropriate normalization to [0,1] range.
108-120: LGTM!The shadow terminator softening is correctly implemented following the Disney approach from the cited SIGGRAPH 2019 paper. The epsilon prevents division by zero, and the smoothing function provides the expected gradual transition.
7-7: DefineGAME_UNIT_TO_Mbefore using it in the macro.The macro
M_TO_GAME_UNITon line 7 depends onGAME_UNIT_TO_M, which is not defined in this file or anywhere in the codebase. This will cause a compilation error. Define the constant in this file or ensure it is included from another header that must be included beforeCommon.hlsli.⛔ Skipped due to learnings
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.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: 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: 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: 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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (5)
64-65: Consider usingRAY_TMAXconsistently.
sourceRay.TMaxis set to1e30here, butRAY_TMAXis used on line 231 for secondary rays. Using the same constant improves maintainability.Also,
TMin = 0.1fdiffers from the0.01fused for bounce rays (line 230). If intentional for camera near-plane handling, a brief comment would clarify this.Suggested change
- sourceRay.TMin = 0.1f; - sourceRay.TMax = 1e30; + sourceRay.TMin = 0.1f; // Larger TMin to avoid near-plane artifacts + sourceRay.TMax = RAY_TMAX;
109-109: Magic number for depth bias.The value
0.99998is a depth bias to avoid precision issues, but it's undocumented. Consider extracting to a named constant for clarity.Suggested change
- const float depth = DepthTexture[idx] * 0.99998; + static const float DEPTH_BIAS = 0.99998f; // Bias to avoid depth precision artifacts + const float depth = DepthTexture[idx] * DEPTH_BIAS;
146-147: Typo:bitangetWS→bitangentWS.Suggested fix
- float3 tangentWS, bitangetWS; - CreateOrthonormalBasis(normalWS, tangentWS, bitangetWS); + float3 tangentWS, bitangentWS; + CreateOrthonormalBasis(normalWS, tangentWS, bitangentWS);
279-282: Remove or document commented-out code.Several blocks of commented-out code remain (footprint validation on lines 279-282, radiance threshold on lines 318-319). Per coding guidelines, avoid leaving TODO/FIXME placeholders. Either remove these or, if they represent intentional future work, track them in an issue.
Also applies to: 318-319
344-349: Commented-out EnvBRDF alternative and known limitation.The comment "Needs linear and PT doesn't have linear :(" on line 344 documents a known limitation. If this is intentional and tracked, that's fine. Otherwise, consider filing an issue to track the path-tracing linear color space support for the Lazarov approximation.
The current
EnvBRDFApproxHirvonenfallback appears functional.Would you like me to open an issue to track adding linear color space support for the path-tracing EnvBRDF calculation?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing.cppsrc/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
🧠 Learnings (3)
📓 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: 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-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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
⏰ 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 (3)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (2)
338-342: Color space handling looks correct.The
radianceaccumulated from tracing is in linear space. For non-path-tracing mode,mainColoris correctly converted viaColor::GammaToTrueLinearbefore adding linear radiance. This aligns with proper linear lighting practices.
285-289: Verify whether cached SHaRC radiance requires additional BRDF weighting.The comment "We probably have to apply BRDF here" indicates uncertainty about the correct usage. Since
SharcGetCachedRadianceis from an external library not available in this repository, consult the SHaRC documentation to confirm whether the cached radiance is already BRDF-weighted or if the current path's BRDF contribution should be applied separately.src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp (1)
11-16: No action required. The descriptor table correctly excludes SHaRCLock because the resolve compute shader (SharcResolveCS.hlsl) uses only three UAV registers (u0-u2) corresponding to SHaRCHashEntries, SHaRCAccumulation, and SHaRCResolved. The lock buffer is intentionally used only in the ray tracing path via external handles, as evidenced by the CreateUAVs method comments ("UAVs for resolve" vs "UAVs for RT") and the fact that the lock buffer UAV is created only for RT, not for the resolve path.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
package/Shaders/Lighting.hlsl (2)
2380-2386: Minor formatting inconsistency in preprocessor directive.Line 2383 has inconsistent spacing (
# elsewith a tab after#) compared to other preprocessor directives in this file.🔎 Suggested fix
# if defined(RT) float3 dirLightColorMultiplier = SharedData::InInterior ? SharedData::raytracingSettings.InteriorDirectional : 1; -# else +# else float3 dirLightColorMultiplier = 1; # endif
3203-3231: Incorrect closing comment and inconsistent formatting.
Line 3231's comment says
// !defined(RT)but it closes the#if defined(RT)block, so it should read// defined(RT).The block has inconsistent indentation—some lines use tabs with varying depth, others use spaces (e.g., lines 3209-3212 vs 3214).
🔎 Suggested fixes
-# endif // !defined(RT) +# endif // defined(RT)For indentation consistency, align the preprocessor directives and code blocks to match the file's convention (tab-based with consistent depth).
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (3)
64-64: Document the rationale for different TMin values.The primary ray uses
TMin = 0.1f, while secondary rays useTMin = 0.01f(line 230). If this difference is intentional (e.g., camera near-plane vs. surface self-intersection), consider adding a comment or using named constants likePRIMARY_RAY_TMINandSECONDARY_RAY_TMINfor clarity.
109-109: Add a comment explaining the depth scale factor.The magic number
0.99998applied to depth lacks explanation. Document why this slight reduction is necessary (e.g., avoiding precision issues, ensuring samples remain within valid depth range).Example clarification
- const float depth = DepthTexture[idx] * 0.99998; + // Scale depth slightly to avoid precision issues at far plane + const float depth = DepthTexture[idx] * 0.99998;
228-230: Consider consolidating ray offset strategy.Both the ray origin offset (
surface.GeomNormal * 0.01f) andTMin = 0.01fare used to prevent self-intersection. While this provides extra safety, the dual approach may be redundant. Additionally, the hardcoded0.01fvalue may not scale well across different scene sizes—consider using a scale-relative offset similar to howFrame.SHaRC.SceneScaleis referenced elsewhere.src/Features/Raytracing/Shape.cpp (2)
388-406: Extract repeated texture validation into a helper.The pattern
texture != nullptr && texture != defaultTextureis repeated four times (lines 388, 393, 398, 403). Extracting a helper improves maintainability.🔎 Proposed refactor
// Add helper function before GetPBRShaderFlags static bool HasNonDefaultTexture(ID3D11Texture2D* texture, ID3D11Texture2D* defaultTexture) { return texture != nullptr && texture != defaultTexture; }Then replace the four blocks:
-const bool hasEmissive = pbrMaterial->emissiveTexture != nullptr && pbrMaterial->emissiveTexture != graphicsState->GetRuntimeData().defaultTextureBlack; +const bool hasEmissive = HasNonDefaultTexture(pbrMaterial->emissiveTexture, graphicsState->GetRuntimeData().defaultTextureBlack); if (hasEmissive) { pbrFlags.set(PBRShaderFlags::HasEmissive); } -const bool hasDisplacement = pbrMaterial->displacementTexture != nullptr && pbrMaterial->displacementTexture != graphicsState->GetRuntimeData().defaultTextureBlack; +const bool hasDisplacement = HasNonDefaultTexture(pbrMaterial->displacementTexture, graphicsState->GetRuntimeData().defaultTextureBlack); // ... and so on for hasFeaturesTexture0 and hasFeaturesTexture1
248-249: UseGetFeature()virtual method instead oftypeidfor type checking.The
BSLightingShaderMaterialPBRclass already has a virtualGetFeature()override that returns a customFEATUREconstant. Replace thetypeidcheck withshaderMaterial->GetFeature() == BSLightingShaderMaterialPBR::FEATUREto avoid runtime type identification and match the pattern already used elsewhere in the function (line 62).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslfeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslpackage/Shaders/Lighting.hlslsrc/Features/Raytracing/Shape.cpp
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslfeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslpackage/Shaders/Lighting.hlslsrc/Features/Raytracing/Shape.cpp
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslfeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslfeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslpackage/Shaders/Lighting.hlslsrc/Features/Raytracing/Shape.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Raytracing/Shape.cpp
🧠 Learnings (11)
📓 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: 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-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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslfeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslpackage/Shaders/Lighting.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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslfeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslpackage/Shaders/Lighting.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 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:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslpackage/Shaders/Lighting.hlsl
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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 (14)
package/Shaders/Lighting.hlsl (5)
345-350: LGTM! Output channel addition is properly guarded.The
#elifordering ensures SNOW takes priority for SV_Target7 (usingParameters), with RT usingGeomNormalMetalnessAOas a fallback. This separation is appropriate since snow rendering has different data requirements than raytracing.
2100-2103: LGTM! Pre-metallic base color capture for RT.Correctly captures the base color before the metallic darkening operation (
baseColor.xyz *= 1 - material.Metallic). This is essential for path tracing which needs the true surface albedo rather than the darkened diffuse component.
2196-2199: LGTM! RT environment map contribution control.This allows the raytracing settings to modulate environment map reflections, enabling proper blending between baked reflections and ray-traced results.
2747-2750: LGTM! RT ambient scaling.Allows raytracing settings to modulate the ambient contribution, which is useful when RT global illumination replaces or supplements traditional ambient lighting.
3166-3172: LGTM! Conditional RT albedo output.The guard correctly matches the scope where
trueBaseColoris defined. The ternary allows users to choose between true surface albedo (for path tracing) and the processed output albedo.features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl (4)
1-4: LGTM!The includes are appropriate and provide all necessary utilities for the shader. The inclusion of
Common/Color.hlsliensures proper color space conversion functions are available.
16-18: LGTM!The normal transformation and roughness conversion logic is correct:
- Properly decodes normals from GBuffer format
- Transforms to world space using the camera inverse matrix
- Normalizes the result to ensure unit-length
- Correctly converts glossiness to roughness (1.0 - glossiness)
20-23: LGTM!The diffuse component calculation correctly applies color space conversion:
- Properly unpacks metallic and AO from the packed GNMAO texture
- Applies
Color::GammaToTrueLinear()to albedo RGB before multiplying by(1.0f - metallic)- Preserves the alpha channel
- Ensures output is in true linear space as required by the raytracing pipeline
This addresses the color space conversion requirements noted in the past review comments.
Based on learnings about linear lighting system requirements.
6-11: No changes required—register bindings are appropriate.Each compute shader in the raytracing pipeline operates in its own local register namespace. Register assignments (t0-t2 for inputs, u0-u1 for outputs) in ConvertTexturesCS.hlsl follow the established codebase convention of hardcoded slots and do not conflict with other shaders in the feature.
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (5)
28-49: LGTM: SHaRC progressive update strategy.The 5×5 block sampling with per-frame cycling (25-frame period to cover all pixels) is an efficient approach for amortizing radiance cache updates without impacting every frame. The boundary check prevents out-of-bounds access.
336-352: LGTM: Output averaging and buffer writes.The sample averaging, envBRDF calculation using Hirvonen approximation, and multi-buffer output (radiance, specular albedo, hit distance) are correctly implemented. Ensure
MAX_SAMPLESandMAX_BOUNCESare appropriately defined in the included headers.
1-19: No review comment needed. The original comment requesting register/buffer conflict verification is not applicable to this file.RayGeneration.hlsl does not directly declare GPU resources with register bindings—it only accesses textures and buffers through included headers. Additionally, hardcoded shader resource slot numbers are the established convention in this codebase and should not be flagged as an issue.
339-341: Line 339 (PATH_TRACING branch) has inconsistent color space output compared to the separate PT shader.The PATH_TRACING branch outputs linear space (
direct + radiancewithout conversion), whilePT/RayGeneration.hlslline 39 appliesTrueLinearToGammato convert to gamma space. This inconsistency is partially documented by the comment on line 342 ("Needs linear and PT doesn't have linear :("). Clarify whether line 339 should also applyTrueLinearToGammafor consistency with the PT shader, or document why PATH_TRACING intentionally outputs linear space.
285-289: Verify BRDF application for cached radiance.The inline comment "We probably have to apply BRDF here" indicates uncertainty about correctness. In a physically-based path tracer, cached radiance must be properly weighted:
- If
sharcRadiancestores incident radiance, it should be multiplied by the BRDF and geometry term before accumulation.- If it stores outgoing radiance (already BRDF-weighted at cache time), the current approach may be correct.
The SHaRC header files are not available in the repository for inspection. Clarify the semantics of
SharcGetCachedRadianceagainst the cache storage convention inSharcUpdateHit(line 301) to ensure the BRDF is applied consistently. Incorrect handling will produce visual artifacts, especially on specular or rough surfaces.
| if (vertexCount != vertices.size()) | ||
| logger::error("[RT] Shape::CreateBuffers - VertexCount: {}, Vertices Size: {}", vertexCount, vertices.size()); |
There was a problem hiding this comment.
Handle vertex count mismatch more defensively.
If vertexCount != vertices.size(), this indicates a logic error in BuildMesh. Simply logging and continuing could cause out-of-bounds access when the buffer is used. Consider asserting in debug builds or returning early in release builds.
🔎 Suggested improvement
if (vertexCount != vertices.size())
- logger::error("[RT] Shape::CreateBuffers - VertexCount: {}, Vertices Size: {}", vertexCount, vertices.size());
+{
+ logger::critical("[RT] Shape::CreateBuffers - VertexCount: {}, Vertices Size: {}", vertexCount, vertices.size());
+ assert(false && "Vertex count mismatch");
+ return; // or throw, depending on error handling strategy
+}🤖 Prompt for AI Agents
In src/Features/Raytracing/Shape.cpp around lines 452-453, do not merely log
when vertexCount != vertices.size(); instead add a defensive guard: in debug
builds assert the mismatch (e.g., assert(vertexCount == vertices.size())) to
catch the logic error early, and in release builds fail fast by returning an
error/false or aborting the buffer creation so no buffers are created or used
with mismatched sizes; ensure any allocated resources are cleaned up and the
caller observes the failure so downstream code cannot access out-of-bounds
vertex data.
| float f = deltaUV1.x * deltaUV2.y - deltaUV2.x * deltaUV1.y; | ||
|
|
||
| if (f == 0.0f) | ||
| f = 1.0f; | ||
|
|
||
| f = 1.0f / f; | ||
|
|
||
| // Compute tangent / bitangent | ||
| half3 tangent = half3(f * (deltaUV2.y * edge1 - deltaUV1.y * edge2)); | ||
| half3 bitangent = half3(f * (-deltaUV2.x * edge1 + deltaUV1.x * edge2)); |
There was a problem hiding this comment.
Degenerate UV handling produces arbitrary tangent vectors.
When UV deltas are degenerate (f == 0), setting f = 1.0f produces tangent/bitangent values from arbitrary edge vectors rather than meaningful tangent-space vectors. This corrupts normal mapping and lighting for affected triangles.
🔎 Suggested improvement
float f = deltaUV1.x * deltaUV2.y - deltaUV2.x * deltaUV1.y;
-if (f == 0.0f)
- f = 1.0f;
-
-f = 1.0f / f;
+if (fabs(f) < 1e-8f) {
+ // Skip degenerate triangle contribution to tangent space
+ continue;
+}
+f = 1.0f / f;
// Compute tangent / bitangent
half3 tangent = half3(f * (deltaUV2.y * edge1 - deltaUV1.y * edge2));The per-vertex fallback at lines 612-618 will handle vertices that end up with zero-length tangents after accumulation.
🤖 Prompt for AI Agents
In src/Features/Raytracing/Shape.cpp around lines 580-589, the code sets f =
1.0f when the UV determinant is zero, which yields arbitrary tangent/bitangent
from edge vectors; instead detect degenerate UVs (use a small epsilon or check f
== 0.0f) and skip computing tangent/bitangent for that triangle by setting them
to zero (or leaving them as zero) so the per-vertex fallback later can handle
zero-length tangents; otherwise compute f = 1.0f / f and the tangent/bitangent
as currently implemented.
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (3)
features/Raytracing/Shaders/Raytracing/Includes/Surface.hlsli (1)
132-132: Critical: Incorrect handedness calculation - usingtangentWSinstead ofbitangentWS.The handedness should compare the computed cross product against the interpolated bitangent, not the tangent:
- float handedness = (dot(cross(normalWS, tangentWS), tangentWS) < 0.0f) ? -1.0f : 1.0f; + float handedness = (dot(cross(normalWS, tangentWS), bitangentWS) < 0.0f) ? -1.0f : 1.0f;The cross product of normal and tangent produces a vector that should be compared to the actual bitangent to determine if the tangent space is left or right-handed.
features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli (1)
372-372: Critical: Undefined variableNdotVused instead ofbrdfContext.NdotV.Line 372 uses
NdotVwhich is not defined in this scope. This should bebrdfContext.NdotVto match the pattern used elsewhere in the file.🔎 Proposed fix
const float Dfuzz = BRDF::D_Charlie(surface.Roughness, NdotH); - const float Gfuzz = BRDF::Vis_Neubelt(NdotV, NdotL); + const float Gfuzz = BRDF::Vis_Neubelt(brdfContext.NdotV, NdotL); float3 Ffuzz = surface.FuzzColor * Dfuzz * Gfuzz * NdotL;src/Features/Raytracing/Buffer.h (1)
28-31: Critical:SetNamecalls wrong object.
SetNamecallsdevice->SetName(name)instead ofresource->SetName(name). This names the device rather than the resource.🔎 Proposed fix
void SetName(LPCWSTR name) { - DX::ThrowIfFailed(device->SetName(name)); + DX::ThrowIfFailed(resource->SetName(name)); }
🧹 Nitpick comments (7)
src/Features/Raytracing.h (6)
305-308: Simplify return type fromconst booltobool.Returning
const boolprovides no benefit over returningboolsince the value is copied. Theconstqualifier on a return-by-value primitive is redundant.Suggested fix
- const bool Active() + bool Active() const { return loaded && settings.Enabled; };
471-473: Member names shadow their enum type names.
DiffuseBRDF DiffuseBRDF,LightEvalMode LightEvalMode, andLightingMode LightingModecan cause confusion when reading or maintaining code. Consider using lowercase or prefixed member names.Example fix
- DiffuseBRDF DiffuseBRDF = DiffuseBRDF::Burley; - LightEvalMode LightEvalMode = LightEvalMode::BRDF; - LightingMode LightingMode = LightingMode::PBR; + DiffuseBRDF diffuseBRDF = DiffuseBRDF::Burley; + LightEvalMode lightEvalMode = LightEvalMode::BRDF; + LightingMode lightingMode = LightingMode::PBR;Update the JSON macro accordingly.
630-631: Remove commented-out code.Per coding guidelines, avoid leaving dead code. If this logic may be needed later, track it in version control or a TODO issue instead.
1047-1055: Remove or guard expensive debug iteration before release.This O(models × shapes) nested loop runs on every texture destruction to verify texture references. The comment acknowledges it "isn't fast." This should be removed for release builds or guarded with a debug-only flag to avoid performance degradation.
Suggested fix
+#ifdef _DEBUG // I imagine this isn't fast but I'll keep this in until I'm sure everything has been fixed for (auto& [key, model] : rt.models) { for (auto& shape : model.shapes) { auto& material = shape->material; if (index == material.BaseTexture->GetIndex()) logger::error("[RT]\t\t NiSourceTexture::Destructor - Found in: {}", key); } } +#endif
1133-1140: Passthrough hook with no custom logic.
BSTriShape_OnVisible::thunkonly calls the original function with no additional behavior. If this is placeholder for future work, add a TODO comment; otherwise, remove the unnecessary hook.
862-863: Uninitialized timing members.
mainTimeandshadowsTimeare uninitialized and could contain garbage values if read before being set.Fix
- float mainTime; - float shadowsTime; + float mainTime = 0.0f; + float shadowsTime = 0.0f;features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (1)
109-109: Document the magic constant0.99998for depth adjustment.The depth value is scaled by
0.99998without explanation. Add a comment explaining why this small adjustment is necessary (e.g., to avoid precision issues at far plane, bias for specific comparison, etc.).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/Common.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/RT/SHaRCHelper.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/RT/Sharc.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/Surface.hlslisrc/Features/Raytracing.cppsrc/Features/Raytracing.hsrc/Features/Raytracing/Buffer.hsrc/Features/Raytracing/Pipelines/SHaRCPipeline.cppsrc/Features/Raytracing/Pipelines/SHaRCPipeline.h
🚧 Files skipped from review as they are similar to previous changes (4)
- features/Raytracing/Shaders/Raytracing/Includes/RT/Sharc.hlsli
- src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
- features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli
- features/Raytracing/Shaders/Raytracing/Includes/RT/SHaRCHelper.hlsli
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/Surface.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlslisrc/Features/Raytracing/Pipelines/SHaRCPipeline.hsrc/Features/Raytracing.hsrc/Features/Raytracing/Buffer.h
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/Surface.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/Surface.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlslisrc/Features/Raytracing/Pipelines/SHaRCPipeline.hsrc/Features/Raytracing.hsrc/Features/Raytracing/Buffer.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Raytracing/Pipelines/SHaRCPipeline.hsrc/Features/Raytracing.hsrc/Features/Raytracing/Buffer.h
🧠 Learnings (15)
📓 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: 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-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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/Surface.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.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 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/Surface.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlslisrc/Features/Raytracing.hsrc/Features/Raytracing/Buffer.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl
📚 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:
features/Raytracing/Shaders/Raytracing/Includes/Surface.hlslifeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.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:
src/Features/Raytracing.h
📚 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:
src/Features/Raytracing.h
📚 Learning: 2025-06-09T22:27:55.011Z
Learnt from: soda3000
Repo: doodlum/skyrim-community-shaders PR: 1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Applied to files:
src/Features/Raytracing.h
📚 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:
src/Features/Raytracing.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:
src/Features/Raytracing.h
📚 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:
src/Features/Raytracing.h
📚 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:
src/Features/Raytracing.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/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/Features/Raytracing.h
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/Features/Raytracing.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). (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)
src/Features/Raytracing.h (3)
169-186:SVGFHeapDefappears to be a copy ofShadowsHeapDefwith identical slot definitions.The
SVGFHeapDef::Slotenum has the exact same values asShadowsHeapDef::Slot(ShadowMask, Depth, TLAS, NumDescriptors, None). If SVGF denoising requires different descriptor slots, this will cause incorrect resource bindings. If intentional, consider removing the duplicate and reusingShadowsHeapDef.
1207-1235:shareTextureflag manipulation is not thread-safe.Both
BGSTextureSet_SetTextureandBSShaderTextureSet_SetTexturemodify the sharedrt.shareTextureflag around the original function call. If texture loading ever occurs on multiple threads, this pattern will cause race conditions.Consider using thread-local storage or passing the flag through the call chain if multi-threaded texture loading is possible.
1508-1512: Good SE/AE compatibility handling.Proper use of
REL::Module::IsAE()to handle different vtable offsets between Special Edition and Anniversary Edition.features/Raytracing/Shaders/Raytracing/Includes/Surface.hlsli (1)
207-207: Be aware that F0 calculation differs between constructors.The simplified constructor uses
PBR::F0(albedo, metallic)which leverages the default dielectric F0 value (0.04), while the full material constructor usesPBR::F0(material.SpecularLevel.xxx, surface.Albedo, surface.Metallic)with a custom specular level. This is an intentional design based on the available parameters, but surfaces created via the simplified path will use a fixed default F0 rather than a material-specific value. Ensure this distinction is understood when choosing which constructor to use.
| OutputTexture[idx] = float4(direct + radiance, 0.0f); | ||
| #else | ||
| OutputTexture[idx] = float4(Color::GammaToTrueLinear(mainColor.rgb) + radiance, 1.0f); | ||
| #endif | ||
|
|
||
| // Needs linear and PT doesn't have linear :( | ||
| //float2 envBRDF = max(0.0f, BRDF::EnvBRDFApproxLazarov(linearRoughness, sourceBRDFContext.NdotV)); | ||
| //SpecularAlbedo[idx] = float4(envBRDF.x * sourceSurface.F0 + envBRDF.y, 0.0f); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, get context around lines 348-355 in RayGeneration.hlsl
head -n 360 features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl | tail -n 30Repository: doodlum/skyrim-community-shaders
Length of output: 1095
🏁 Script executed:
# Check the PATH_TRACING and non-PATH_TRACING paths to understand color space handling
rg -nB20 "OutputTexture\[idx\] = float4\(direct" features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 649
🏁 Script executed:
# Check how direct and radiance are computed
rg -nB5 "float3 direct|float3 radiance" features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl | head -n 50Repository: doodlum/skyrim-community-shaders
Length of output: 915
🏁 Script executed:
# Check how OutputTexture is consumed downstream
rg -nC2 "OutputTexture" features/Raytracing/Shaders/ --type=hlsl | grep -v RayGeneration.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 104
🏁 Script executed:
# Check the Color::GammaToTrueLinear and Color::IrradianceToLinear functions
rg -n "GammaToTrueLinear|IrradianceToLinear" features/ --type=hlsl | head -n 20Repository: doodlum/skyrim-community-shaders
Length of output: 104
🏁 Script executed:
# Check EvaluateDirectRadiance implementation
rg -n "EvaluateDirectRadiance" features/Raytracing/Shaders/Raytracing/GI/ -A5Repository: doodlum/skyrim-community-shaders
Length of output: 1391
🏁 Script executed:
# Check how radiance is computed - look for SampleSky and other radiance sampling
rg -n "sampleRadiance|SampleSky" features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl -B2 -A2Repository: doodlum/skyrim-community-shaders
Length of output: 1760
🏁 Script executed:
# Check mainColor in non-PATH_TRACING path
rg -n "mainColor" features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl -B3 -A1Repository: doodlum/skyrim-community-shaders
Length of output: 632
🏁 Script executed:
# Look at the broader context to understand if this is a real issue or fixed comment
sed -n '70,110p' features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 1570
🏁 Script executed:
# Check how OutputTexture is used downstream in the rendering pipeline
find features/Raytracing/Shaders -name "*.hlsl" -o -name "*.hlsli" | xargs grep -l "OutputTexture" | head -10Repository: doodlum/skyrim-community-shaders
Length of output: 197
🏁 Script executed:
# Look for SampleSky implementation and documentation
rg -n "SampleSky" features/Raytracing/Shaders/ -B2 -A5 | head -40Repository: doodlum/skyrim-community-shaders
Length of output: 1614
🏁 Script executed:
# Look for EvaluateDirectRadiance implementation
rg -n "EvaluateDirectRadiance" features/Raytracing/Shaders/ --type-not-match=hlsl -A10Repository: doodlum/skyrim-community-shaders
Length of output: 520
🏁 Script executed:
# Check if there are any comments about color spaces in these radiance functions
rg -n "linear|color space|gamma|irradiance" features/Raytracing/Shaders/Raytracing/ -i | grep -i "sample\|evaluate\|direct"Repository: doodlum/skyrim-community-shaders
Length of output: 475
🏁 Script executed:
# Search for SampleSky function definition
fd -e hlsl -e hlsli | xargs grep -l "SampleSky" | xargs grep -B2 "SampleSky"Repository: doodlum/skyrim-community-shaders
Length of output: 3037
🏁 Script executed:
# Look for includes in RayGeneration.hlsl that might contain these functions
head -50 features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl | grep -E "#include|#ifndef|#define"Repository: doodlum/skyrim-community-shaders
Length of output: 646
🏁 Script executed:
# Search for EvaluateDirectRadiance implementation
fd -e hlsl -e hlsli | xargs grep -B5 "float3.*EvaluateDirectRadiance"Repository: doodlum/skyrim-community-shaders
Length of output: 4128
🏁 Script executed:
# Check PT version for comparison
head -400 features/Raytracing/Shaders/Raytracing/PT/RayGeneration.hlsl | tail -100Repository: doodlum/skyrim-community-shaders
Length of output: 1369
🏁 Script executed:
# Check what functions are in the included files, particularly looking for SampleSky definition
rg -n "SampleSky" features/Raytracing/Shaders/Raytracing/Includes/ -B2 -A8Repository: doodlum/skyrim-community-shaders
Length of output: 164
🏁 Script executed:
# Look for Shading.hlsli which likely contains EvaluateDirectRadiance
cat features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 172
🏁 Script executed:
# Check if there are color space comments in Shading.hlsli
rg -n "linear|gamma|color space" features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli -i -B1 -A1Repository: doodlum/skyrim-community-shaders
Length of output: 180
🏁 Script executed:
# Find the actual structure of the Raytracing shaders
find features/Raytracing -type f -name "*.hlsl" -o -name "*.hlsli" | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 1133
🏁 Script executed:
# List all files in Raytracing directory
ls -la features/Raytracing/Shaders/Raytracing/Repository: doodlum/skyrim-community-shaders
Length of output: 949
🏁 Script executed:
# Find all includes
find features -name "Shading.hlsli"Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Search for Shading.hlsli in all of features
find features -name "Shading.hlsli" 2>/dev/nullRepository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Search for where includes are actually located
ls -la features/Raytracing/Shaders/Repository: doodlum/skyrim-community-shaders
Length of output: 243
🏁 Script executed:
# Check the actual include paths by examining one shader file carefully
head -30 features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 927
🏁 Script executed:
# Look for Common folder
find features -name "Common" -type dRepository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Search for SampleSky across all hlsl/hlsli files
rg "float3.*SampleSky|SampleSky.*=" features/ -B3 -A3Repository: doodlum/skyrim-community-shaders
Length of output: 1342
🏁 Script executed:
# List all files in features/Raytracing to understand structure
find features/Raytracing -type f | head -30Repository: doodlum/skyrim-community-shaders
Length of output: 1133
🏁 Script executed:
# Try to find all hlsli files
find features/Raytracing -name "*.hlsli"Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Search for Shading.hlsli with escaped quotes
find . -path "*Raytracing*" -name "*Shading*"Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Look at file structure differently
ls -R features/Raytracing/Shaders/Raytracing/ | head -60Repository: doodlum/skyrim-community-shaders
Length of output: 691
🏁 Script executed:
# Check if there's a different shader directory structure
find features/Raytracing -type dRepository: doodlum/skyrim-community-shaders
Length of output: 394
🏁 Script executed:
# Check if includes might be in a different location
find . -path "*includes*" -o -path "*Includes*" 2>/dev/null | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Since we can't find the include files, let's check the git structure to see if there's something we're missing
git ls-files features/Raytracing/ | head -30Repository: doodlum/skyrim-community-shaders
Length of output: 1957
🏁 Script executed:
# Let's search for where SampleSky is defined more broadly
rg "SampleSky\s*\(" . -B5 -A5 2>/dev/null | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 2129
🏁 Script executed:
# Now let's read the Shading.hlsli file
cat features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsliRepository: doodlum/skyrim-community-shaders
Length of output: 172
🏁 Script executed:
# Also check for SampleSky definition in Common.hlsli
grep -n "SampleSky" features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli -B3 -A8Repository: doodlum/skyrim-community-shaders
Length of output: 169
🏁 Script executed:
# Check CommonRT.hlsli
grep -n "SampleSky" features/Raytracing/Shaders/Raytracing/Includes/RT/CommonRT.hlsli -B3 -A8Repository: doodlum/skyrim-community-shaders
Length of output: 174
🏁 Script executed:
# Check RT.hlsli
grep -n "SampleSky" features/Raytracing/Shaders/Raytracing/Includes/RT.hlsli -B3 -A8Repository: doodlum/skyrim-community-shaders
Length of output: 165
🏁 Script executed:
# Check what git thinks exists vs what's actually there
git ls-files features/Raytracing/Shaders/Raytracing/Includes/ | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 1380
🏁 Script executed:
# Let's check if the files actually exist
ls -la features/Raytracing/Shaders/Raytracing/Includes/ 2>&1 | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 171
🏁 Script executed:
# Try a direct path
test -f features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli && echo "exists" || echo "not found"Repository: doodlum/skyrim-community-shaders
Length of output: 85
🏁 Script executed:
# Let's look at what git status says
git status features/Raytracing/Shaders/Raytracing/Includes/ 2>&1Repository: doodlum/skyrim-community-shaders
Length of output: 318
🏁 Script executed:
# Check for submodules
git submodule statusRepository: doodlum/skyrim-community-shaders
Length of output: 269
🏁 Script executed:
# Since we're in sparse checkout, let's try to expand the checkout to get the include files
git sparse-checkout set features/Raytracing/Shaders/Raytracing/Includes/Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# After expanding, try to read the files
cat features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli 2>/dev/null || echo "Still not accessible"Repository: doodlum/skyrim-community-shaders
Length of output: 96
🏁 Script executed:
# Let's check if we can at least see what Common includes
git show HEAD:features/Raytracing/Shaders/Raytracing/Includes/Common.hlsli | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 2547
🏁 Script executed:
# Check the Shading file from git
git show HEAD:features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli | head -150Repository: doodlum/skyrim-community-shaders
Length of output: 5434
🏁 Script executed:
# Get more of the Shading.hlsli file to find SampleSky and EvaluateDirectRadiance
git show HEAD:features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli | tail -n +150 | head -200Repository: doodlum/skyrim-community-shaders
Length of output: 6592
🏁 Script executed:
# Search for SampleSky in the git history
git show HEAD:features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli | grep -n "SampleSky" -A5 -B2Repository: doodlum/skyrim-community-shaders
Length of output: 341
🏁 Script executed:
# Search for EvaluateDirectRadiance
git show HEAD:features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli | grep -n "EvaluateDirectRadiance" -A10 -B2Repository: doodlum/skyrim-community-shaders
Length of output: 520
🏁 Script executed:
# Let's check the entire file length and get the rest
git show HEAD:features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli | wc -lRepository: doodlum/skyrim-community-shaders
Length of output: 79
🏁 Script executed:
# Get the full SampleSky implementation to confirm it's TrueLinear
git show HEAD:features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli | sed -n '387,400p'Repository: doodlum/skyrim-community-shaders
Length of output: 494
🏁 Script executed:
# Now let's check if there are any comments about downstream OutputTexture usage or expectations
git show HEAD:features/Raytracing/Shaders/Raytracing/GI/ClosestHit.hlsl | grep -i "output\|linear\|gamma" -B2 -A2 | head -40Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Compare PT RayGeneration to see what it outputs
git show HEAD:features/Raytracing/Shaders/Raytracing/PT/RayGeneration.hlsl | grep -n "OutputTexture"Repository: doodlum/skyrim-community-shaders
Length of output: 155
Apply consistent color space conversion to PATH_TRACING output.
The PATH_TRACING path outputs direct + radiance directly to OutputTexture (line 348), both of which are in linear space (SampleSky returns TrueLinear and EvaluateDirectRadiance returns linear values). However, the PT version converts linear to gamma via Color::TrueLinearToGamma() before writing to OutputTexture (PT/RayGeneration.hlsl line 39). Apply the same conversion to maintain consistent output color space across both implementations.
🤖 Prompt for AI Agents
In features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl around lines
348-355, the PATH_TRACING branch writes linear-space direct+radiance to
OutputTexture without converting to gamma, causing inconsistent color space
versus the PT path; update the PATH_TRACING output to pass (direct + radiance)
through Color::TrueLinearToGamma() before writing and use the same alpha as the
PT path so both implementations produce consistent gamma-space output.
| const std::vector<IPipeline*>& GetPipelines() | ||
| { | ||
| static std::vector<IPipeline*> pipelines = { | ||
| sharcPipeline.get() | ||
| }; | ||
|
|
||
| return pipelines; | ||
| }; |
There was a problem hiding this comment.
Static local in GetPipelines() may return stale or null pointers.
The static vector is initialized once at first call. If sharcPipeline is null at that time (e.g., SHARC disabled or not yet initialized), the vector will permanently contain a null pointer. Additionally, if pipelines are recreated, the static vector won't update.
Consider dynamic construction or lazy validation
const std::vector<IPipeline*>& GetPipelines()
{
- static std::vector<IPipeline*> pipelines = {
- sharcPipeline.get()
- };
+ static std::vector<IPipeline*> pipelines;
+ pipelines.clear();
+#ifdef SHARC
+ if (sharcPipeline)
+ pipelines.push_back(sharcPipeline.get());
+#endif
return pipelines;
};Alternatively, return by value if the vector is small.
🤖 Prompt for AI Agents
In src/Features/Raytracing.h around lines 310-317, the function currently builds
a static vector containing sharcPipeline.get() which can be null at first init
or become stale if pipelines are recreated; change GetPipelines() to construct
and return a fresh std::vector<IPipeline*> on each call (or return by value) and
only push sharcPipeline.get() if sharcPipeline is non-null (and similarly
validate any other pipeline pointers), so the returned list never contains null
or outdated pointers.
| bool Update(RE::NiNode* pNiNode, [[maybe_unused]] const eastl::pair<eastl::string, Model&>& modelPair) | ||
| { | ||
| // Instance was not changed by the game, so there is no need to update it | ||
| // This doesn't work at all for actors | ||
| /*if (pNiNode->lastUpdatedFrameCounter < globals::state->frameCount && hasUpdated) | ||
| return true;*/ | ||
|
|
||
| // Instance has already been updated this frame | ||
| if (!frameChecker.IsNewFrame()) | ||
| return true; | ||
|
|
||
| XMStoreFloat3x4(&transform, GetXMFromNiTransform(pNiNode->world)); | ||
|
|
||
| auto& [path, model] = modelPair; | ||
|
|
||
| if ((model.GetFlags() & Flags::Dynamic) || (model.GetFlags() & Flags::Skinned)) { | ||
| for (auto& shape : model.shapes) { | ||
| Flags updateFlags = Flags::None; | ||
|
|
||
| // Updates Dynamic Vertex position (and Bitangent.x) buffer | ||
| // TODO: Test performance and stability of using a upload heap buffer and keeping it mapped to dynamicData | ||
| if ((shape->flags & Flags::Dynamic) && shape->geometry) { | ||
| //auto* pDynamicTriShape = netimmerse_cast<RE::BSDynamicTriShape*>(shape->geometry); | ||
|
|
||
| auto* pDynamicTriShape = skyrim_cast<RE::BSDynamicTriShape*>(shape->geometry); | ||
|
|
||
| if (pDynamicTriShape) { | ||
| const auto& dynTriShapeRuntime = pDynamicTriShape->GetDynamicTrishapeRuntimeData(); | ||
|
|
||
| // We'll test if dynamic data has changed before updating and uploading | ||
| // It does mean we have to memcpy twice, but I suppose the GPU bandwith we save makes up for it | ||
| if (dynTriShapeRuntime.dynamicData && std::memcmp(shape->dynamicPosition.data(), dynTriShapeRuntime.dynamicData, dynTriShapeRuntime.dataSize) != 0) { | ||
| std::memcpy(shape->dynamicPosition.data(), dynTriShapeRuntime.dynamicData, dynTriShapeRuntime.dataSize); | ||
|
|
||
| shape->dynamicPositionBuffer->Update(dynTriShapeRuntime.dynamicData, dynTriShapeRuntime.dataSize); | ||
|
|
||
| // We'll barrier and upload ourselfs in batch | ||
| //shape.dynamicPositionBuffer->Upload(commandList); | ||
| updateFlags |= Flags::Dynamic; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // TODO: Handle skinned meshes | ||
| if ((shape->flags & Flags::Skinned) && shape->geometry) { | ||
| // Restore pre-skinning vertices | ||
| //shape.vertexBuffer->Upload(commandList); | ||
|
|
||
| updateFlags |= Flags::Skinned; | ||
| } | ||
|
|
||
| if ((updateFlags & Flags::Dynamic) || (updateFlags & Flags::Skinned)) { | ||
| auto& rt = globals::features::raytracing; | ||
|
|
||
| rt.modelUpdate.emplace_back(path); | ||
| rt.vertexUpdate.emplace_back(shape->allocation->GetIndex(), updateFlags & Flags::Dynamic ? shape->dynamicPositionBuffer.get() : nullptr, shape->vertexBuffer.get(), shape->vertexCount, updateFlags); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Thread safety concern in Instance::Update.
The method appends to rt.modelUpdate and rt.vertexUpdate vectors (lines 680-681) without synchronization. If UpdateInstances() is called from multiple threads or while another thread is iterating these vectors, this will cause data races.
Ensure this method is only called from a single thread, or protect access to these vectors with geometryMutex or bufferMutex.
🤖 Prompt for AI Agents
In src/Features/Raytracing.h around lines 626 to 687, Update() appends to
globals::features::raytracing.modelUpdate and .vertexUpdate without
synchronization, causing possible data races; protect those emplace_back calls
with the existing mutex (geometryMutex or bufferMutex) by taking a lock (e.g.,
std::lock_guard or std::scoped_lock) around the region that mutates
rt.modelUpdate and rt.vertexUpdate, or assert/ensure Update() is only ever
called from the single update thread; keep the lock scope as small as possible
to cover the two emplace_back operations.
| static bool Register() | ||
| { | ||
| static TESLoadGameEventHandler singleton; | ||
|
|
||
| auto scriptEventSourceHolder = RE::ScriptEventSourceHolder::GetSingleton(); | ||
| scriptEventSourceHolder->GetEventSource<RE::TESLoadGameEvent>()->AddEventSink(&singleton); | ||
|
|
||
| logger::info("Registered {}", typeid(singleton).name()); | ||
|
|
||
| return true; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing null check for ScriptEventSourceHolder::GetSingleton().
Unlike MenuOpenCloseEventHandler::Register() which checks globals::game::ui for null, TESLoadGameEventHandler::Register() directly dereferences the singleton result without null checking. If called too early or in an invalid state, this will cause a null pointer dereference.
Suggested fix
static bool Register()
{
static TESLoadGameEventHandler singleton;
auto scriptEventSourceHolder = RE::ScriptEventSourceHolder::GetSingleton();
+ if (!scriptEventSourceHolder) {
+ logger::error("ScriptEventSourceHolder not found");
+ return false;
+ }
scriptEventSourceHolder->GetEventSource<RE::TESLoadGameEvent>()->AddEventSink(&singleton);
logger::info("Registered {}", typeid(singleton).name());
return true;
}| static bool Register() | ||
| { | ||
| static TESObjectLoadedEventHandler singleton; | ||
|
|
||
| auto scriptEventSourceHolder = RE::ScriptEventSourceHolder::GetSingleton(); | ||
| scriptEventSourceHolder->GetEventSource<RE::TESObjectLoadedEvent>()->AddEventSink(&singleton); | ||
|
|
||
| logger::info("Registered {}", typeid(singleton).name()); | ||
|
|
||
| return true; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Same missing null check in TESObjectLoadedEventHandler::Register().
Apply the same null check pattern as suggested for TESLoadGameEventHandler.
🤖 Prompt for AI Agents
In src/Features/Raytracing.h around lines 1582 to 1593,
TESObjectLoadedEventHandler::Register() lacks null checks; replicate the
TESLoadGameEventHandler pattern by validating
RE::ScriptEventSourceHolder::GetSingleton() for null, then retrieving the
TESObjectLoadedEvent event source into a local pointer and validating it before
calling AddEventSink. If either is null, log an error with context and return
false; only call AddEventSink and log the success message when both pointers are
valid and then return true.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (2)
348-348: Apply gamma conversion to PATH_TRACING output.This issue was flagged in a previous review and remains unresolved. Both
direct(fromEvaluateDirectRadiance) andradiance(accumulated fromSampleSkyand other sources) are in linear space. The PT/RayGeneration.hlsl version converts linear to gamma viaColor::TrueLinearToGamma()before writing to OutputTexture. Apply the same conversion here for consistency.🔎 Proposed fix
- OutputTexture[idx] = float4(direct + radiance, 0.0f); + OutputTexture[idx] = float4(Color::TrueLinearToGamma(direct + radiance), 0.0f);Based on learnings, proper linear color space handling is required for the rendering pipeline.
353-355: Remove or document commented-out envBRDF code.Per project guidelines, commented experimental code should be removed or documented with a TODO and issue reference. The note "PT doesn't have linear" suggests color-space concerns but provides insufficient context for future maintainers.
🔎 Recommendation
Either remove the commented lines, or replace with:
// TODO(#ISSUE): EnvBRDFApproxLazarov requires linear color space // which is unavailable in PT path. Re-enable after [condition]. // float2 envBRDF = max(0.0f, BRDF::EnvBRDFApproxLazarov(linearRoughness, sourceBRDFContext.NdotV)); // SpecularAlbedo[idx] = float4(envBRDF.x * sourceSurface.F0 + envBRDF.y, 0.0f);Based on coding guidelines, complete solutions are required without placeholder comments.
🧹 Nitpick comments (3)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (3)
109-109: Document the depth scaling factor.The magic number
0.99998applied to depth lacks explanation. Clarify whether this is for Z-fighting prevention, precision handling, or another purpose.
221-222: Consider epsilon tolerance for backface check.The exact
<= 0.0comparison may incorrectly cull rays at grazing angles due to floating-point precision. Consider using a small negative epsilon (e.g.,-1e-5) to avoid premature termination.
244-247: Fixed ray offset may cause artifacts at varying scales.The hardcoded
0.01foffset for ray origin (line 244) andTMin(line 246) could cause self-intersection at small scales or light leaks at large scales. Consider making this scene-scale relative or using a dynamic epsilon based on hit distance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/Types/FrameData.hlslisrc/Features/Raytracing.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- features/Raytracing/Shaders/Raytracing/Includes/Types/FrameData.hlsli
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl
🧠 Learnings (5)
📓 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.
📚 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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 (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 (1)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (1)
1-361: Register usage is properly centralized and no conflicts exist.All raytracing shaders—including this GI variant—use the centralized
Registers.hlslifile, which defines a single set of register bindings (constant buffers, UAVs, SRVs, and samplers) shared across the pipeline. This prevents conflicts between the GI, Path Tracing, and Shadows ray generation shaders.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlsl (1)
59-63: Remove unused variables.
uv(line 59) andhistoryColor(line 62) are computed/loaded but never used in this shader, adding unnecessary computation overhead.🔎 Proposed fix
if (DTid.x >= screen_size.x || DTid.y >= screen_size.y) return; - float2 uv = float2(DTid.xy + 0.5) * SharedData::BufferDim.zw * FrameBuffer::DynamicResolutionParams2.xy; - float3 blendedColor = 0; - float4 historyColor = HistoryTexture[DTid.xy]; float4 ssrColor = SSRColorTexture[DTid.xy]; float depthCenter = DepthTexture[DTid.xy];features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlsl (1)
24-24: Remove unused variable.
uvis computed but never used in this shader.🔎 Proposed fix
if (DTid.x >= screen_size.x || DTid.y >= screen_size.y) return; - float2 uv = float2(DTid.xy + 0.5) * SharedData::BufferDim.zw * FrameBuffer::DynamicResolutionParams2.xy; - float4 ssrColor = SSRColorTexture[DTid.xy];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslsrc/Features/Raytracing.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlsl
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.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:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlsl
🧠 Learnings (3)
📓 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: 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: 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.
📚 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:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlsl
🔇 Additional comments (4)
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlsl (1)
53-122: LGTM - main spatial filtering logic.The A-Trous spatial filter implementation is correct with proper bounds checking, depth-gated sampling, and fallback to original color when
weightSumis zero.features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlsl (1)
18-79: LGTM - variance estimation pass.The conditional neighbor accumulation for low-history pixels (≤2 frames) and variance boosting via
2/historyscaling follows standard SVGF practices. The early write at line 29 serves as an efficient fallback for pixels with sufficient history.features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlsl (2)
21-38: LGTM - history validation.The
IsValidHistoryfunction properly validates reprojected samples with UV bounds, pixel bounds, and a normal consistency check (30° threshold). The unusedroughnessoutput is acceptable as it's required by theGetNormalRoughnesssignature.
78-149: Consider using neighbor UVs inIsValidHistorycalls.In both fallback loops (lines 86 and 129),
prevUVis passed toIsValidHistoryinstead of the computed neighbor UV. While the pixel bounds check inside uses the correctneighborPixel, the UV bounds check validates the original reprojection point rather than the neighbor's position. This is likely intentional (checking if reprojection itself was valid), but if neighbors can fall outside UV bounds independently, consider computing neighbor UVs.
| float GaussianBlur(uint2 id) | ||
| { | ||
| float sum = 0.f; | ||
| float kernelSum = 0.f; | ||
| const float kernel[2][2] = | ||
| { | ||
| { 1.0 / 4.0, 1.0 / 8.0 }, | ||
| { 1.0 / 8.0, 1.0 / 16.0 } | ||
| }; | ||
|
|
||
| const int radius = 1; | ||
|
|
||
| for (int y = -radius; y <= radius; y++) | ||
| { | ||
| for (int x = -radius; x <= radius; x++) | ||
| { | ||
| const int2 p = id + int2(x, y); | ||
| const bool inside = (p.x >= 0 && p.y >= 0) && (p.x < SharedData::BufferDim.x * FrameBuffer::DynamicResolutionParams1.x && p.y < SharedData::BufferDim.y * FrameBuffer::DynamicResolutionParams1.y); | ||
|
|
||
| if (inside) | ||
| { | ||
| const float k = kernel[abs(x)][abs(y)]; | ||
| kernelSum += k; | ||
| sum += SSRColorTexture[p].w * k; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return sum / kernelSum; | ||
| } |
There was a problem hiding this comment.
Potential division by zero in GaussianBlur.
If all samples fall outside the valid region (e.g., pixel at corner with negative offsets), kernelSum remains zero, causing undefined behavior on line 45.
🔎 Proposed fix
- return sum / kernelSum;
+ return kernelSum > 0.f ? sum / kernelSum : 0.f;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| float GaussianBlur(uint2 id) | |
| { | |
| float sum = 0.f; | |
| float kernelSum = 0.f; | |
| const float kernel[2][2] = | |
| { | |
| { 1.0 / 4.0, 1.0 / 8.0 }, | |
| { 1.0 / 8.0, 1.0 / 16.0 } | |
| }; | |
| const int radius = 1; | |
| for (int y = -radius; y <= radius; y++) | |
| { | |
| for (int x = -radius; x <= radius; x++) | |
| { | |
| const int2 p = id + int2(x, y); | |
| const bool inside = (p.x >= 0 && p.y >= 0) && (p.x < SharedData::BufferDim.x * FrameBuffer::DynamicResolutionParams1.x && p.y < SharedData::BufferDim.y * FrameBuffer::DynamicResolutionParams1.y); | |
| if (inside) | |
| { | |
| const float k = kernel[abs(x)][abs(y)]; | |
| kernelSum += k; | |
| sum += SSRColorTexture[p].w * k; | |
| } | |
| } | |
| } | |
| return sum / kernelSum; | |
| } | |
| float GaussianBlur(uint2 id) | |
| { | |
| float sum = 0.f; | |
| float kernelSum = 0.f; | |
| const float kernel[2][2] = | |
| { | |
| { 1.0 / 4.0, 1.0 / 8.0 }, | |
| { 1.0 / 8.0, 1.0 / 16.0 } | |
| }; | |
| const int radius = 1; | |
| for (int y = -radius; y <= radius; y++) | |
| { | |
| for (int x = -radius; x <= radius; x++) | |
| { | |
| const int2 p = id + int2(x, y); | |
| const bool inside = (p.x >= 0 && p.y >= 0) && (p.x < SharedData::BufferDim.x * FrameBuffer::DynamicResolutionParams1.x && p.y < SharedData::BufferDim.y * FrameBuffer::DynamicResolutionParams1.y); | |
| if (inside) | |
| { | |
| const float k = kernel[abs(x)][abs(y)]; | |
| kernelSum += k; | |
| sum += SSRColorTexture[p].w * k; | |
| } | |
| } | |
| } | |
| return kernelSum > 0.f ? sum / kernelSum : 0.f; | |
| } |
🤖 Prompt for AI Agents
In features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlsl around
lines 17 to 46, the GaussianBlur function can divide by zero when kernelSum
remains 0 if every sample is outside the valid region; fix by guarding the final
division: after accumulating kernelSum and sum, check if kernelSum is <= 0 (or
very small) and return a safe default (e.g., 0.0 or the center sample value)
instead of performing sum / kernelSum, so the shader never executes an undefined
divide-by-zero.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp (2)
3-36: Consider adding parameter validation for robustness.The method doesn't validate that
deviceis non-null before use. While the caller may guarantee this, adding a null check or assertion would make the code more defensive and easier to debug.🔎 Optional defensive check
void SHaRCPipeline::CreateRootSignature(ID3D12Device5* device) { + assert(device != nullptr); + heap = eastl::make_unique<DX12::DescriptorHeap<SHaRCHeap>>(
80-80: Misleading parameter name:frameBufferis used as a constant buffer.The parameter
frameBufferis bound as a constant buffer view (line 90), not as a typical frame buffer texture. Consider renaming toconstantBufferorparamBufferto clarify its intended usage and avoid confusion.🔎 Suggested parameter rename
-void SHaRCPipeline::Resolve(ID3D12GraphicsCommandList4* commandList, ID3D12Resource* frameBuffer) +void SHaRCPipeline::Resolve(ID3D12GraphicsCommandList4* commandList, ID3D12Resource* paramBuffer) { commandList->SetPipelineState(pipelineState.get()); commandList->SetComputeRootSignature(rootSignature.get()); auto* pHeap = heap->Heap(); commandList->SetDescriptorHeaps(1, &pHeap); commandList->SetComputeRootDescriptorTable(0, heap->TableGPUHandle(SHaRCHeap::Table::UAV)); - commandList->SetComputeRootConstantBufferView(1, frameBuffer->GetGPUVirtualAddress()); + commandList->SetComputeRootConstantBufferView(1, paramBuffer->GetGPUVirtualAddress());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
**/*
⚙️ 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:
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
🧠 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: 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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
📚 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:
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
📚 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:
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
📚 Learning: 2025-06-09T22:27:55.011Z
Learnt from: soda3000
Repo: doodlum/skyrim-community-shaders PR: 1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Applied to files:
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
📚 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:
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp
⏰ 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 (6)
src/Features/Raytracing/Pipelines/SHaRCPipeline.cpp (6)
38-49: Shader compilation looks correct.The compute shader is compiled with shader model 6.5, which is appropriate for DirectX 12 ray-tracing features. Error handling is properly implemented.
66-78: UAV creation correctly handles resolve and RT resource views.The method appropriately creates separate UAV sets for the resolve compute pass (using internal heap descriptors) and the ray-tracing pass (using external descriptors). The omission of the lock buffer from the resolve UAVs matches the descriptor table layout in
CreateRootSignature.
92-103: UAV barrier synchronization is correctly implemented.The method properly uses UAV barriers before and after the compute dispatch to ensure memory coherency. The barrier array includes only the three buffers used in the resolve pass (hash entries, accumulation, resolved), which correctly excludes the lock buffer.
51-64: No issues found. MAX_CAPACITY is defined as a static constexpr member in the header (line 34), and both SharcAccumulationData and SharcPackedData are properly defined in the included SharcTypes.h header file. All referenced constants and types are correctly declared and accessible.
100-101: This review comment references a file that does not exist in this repository. The filesrc/Features/Raytracing/Pipelines/SHaRCPipeline.cppis not present. This repository contains only HLSL shader files with no C++ pipeline code. The SharcResolveCS.hlsl shader exists atfeatures/Raytracing/Shaders/Raytracing/SharcResolveCS.hlsland uses[numthreads(LINEAR_BLOCK_SIZE, 1, 1)]withLINEAR_BLOCK_SIZEdefined as 256, but there is no C++ dispatch code to review.Likely an incorrect or invalid review comment.
90-90: Verify resource type and validation for frameBuffer parameter in Resolve method.The code passes
frameBufferas a genericID3D12Resource*toSetComputeRootConstantBufferView(), which requires a properly flagged constant buffer resource. The root signature declares parameter 1 as a constant buffer view, but there is no runtime validation or type safety to ensure the passed resource meets this requirement. Verify that all callers provide a valid constant buffer resource.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (1)
345-348: Color space and alpha channel inconsistency between paths.The PATH_TRACING and non-PATH_TRACING outputs differ in two ways:
- Color space: PATH_TRACING outputs linear values directly (line 345), while the non-PATH_TRACING path converts
mainColorfrom gamma to linear (line 347). A past review comment suggested both should output in gamma space for consistency with the PT implementation.- Alpha channel: PATH_TRACING uses
0.0fwhile non-PATH_TRACING uses1.0f, which may affect downstream blending.Verify that these differences are intentional and that downstream consumers handle both paths correctly.
src/Features/Raytracing/Buffer.h (1)
28-31: Critical:SetNamecalls wrong object.
SetNamecallsdevice->SetName(name)instead ofresource->SetName(name). This names the device rather than the resource.🔎 Proposed fix
void SetName(LPCWSTR name) { - DX::ThrowIfFailed(device->SetName(name)); + DX::ThrowIfFailed(resource->SetName(name)); }
🧹 Nitpick comments (3)
src/Features/Raytracing/Buffer.h (3)
289-289: Remove commented-out code.The commented line should be removed to maintain code cleanliness.
🔎 Proposed fix
this->TransitionBarrier(commandList, D3D12_RESOURCE_STATE_COPY_DEST); commandList->CopyResource(this->resource.get(), uploadResource.get()); - //commandList->CopyBufferRegion(this->resource.get(), 0, uploadResource.get(), 0, sizeof(T) * this->count); this->TransitionBarrier(commandList, D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE);Based on coding guidelines, which specify no TODO/FIXME placeholders in implementation files.
43-52: Consider documenting thread-safety expectations.The
TransitionBarriermethod reads and updatesstatewithout synchronization. IfResourceinstances are accessed from multiple threads concurrently, this creates a race condition. If the intended usage is single-threaded access per resource (aligned with D3D12 command list thread-safety model), consider adding documentation to clarify this expectation.
401-413: Consider bounds validation in UpdateAt.
UpdateAtcalculatesbegin = index * sizeof(T)without validating thatindex < count. While this is a low-level abstraction where caller responsibility is typical, out-of-bounds writes could corrupt memory. Consider adding assertions in debug builds for defensive programming.🔎 Potential addition
void UpdateAt(T const* srcData, size_t index = 0, uint uploadIndex = 0) { + assert(index < this->count && "UpdateAt: index out of bounds"); size_t begin = index * sizeof(T);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.h
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.h
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Raytracing/Buffer.h
🧠 Learnings (11)
📓 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: 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-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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.h
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.h
📚 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.h
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.h
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.h
📚 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing/Buffer.h
🔇 Additional comments (8)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (8)
1-19: LGTM: Include structure is well-organized.The include hierarchy is appropriate for a raytracing shader with SHaRC integration, BRDF evaluation, and surface handling.
20-50: LGTM: Initialization and SHaRC update pass logic.The random seed initialization and SHaRC update pass pixel selection pattern (5×5 blocks with rotating indices) are correctly implemented with appropriate boundary checks.
51-100: LGTM: PATH_TRACING primary ray setup.The camera ray reconstruction, primary ray trace, and surface/BRDF context initialization are correctly implemented. The miss case appropriately returns sky irradiance and exits early.
101-153: LGTM: G-buffer reconstruction logic.The G-buffer sampling, world-space position reconstruction, and surface initialization are correctly implemented. The gamma-to-linear conversions for albedo and mainColor are appropriate for a linear rendering pipeline.
155-160: LGTM: SHaRC debug visualization.The debug path correctly outputs hash grid visualization and exits early when enabled.
162-182: LGTM: Loop variable initialization.The variable declarations for the path tracing loop are appropriate.
183-289: LGTM: Bounce loop logic.The multi-bounce path tracing loop is correctly implemented with:
- Proper BRDF sampling (diffuse-only or full PBR modes)
- Backface rejection to prevent invalid rays
- Russian roulette termination with configurable minimum bounces
- Ray origin bias to avoid self-intersection
- SHaRC cache lookup with validity checks
- Direct radiance evaluation and emissive accumulation
Also applies to: 299-340
350-354: LGTM: Specular output computation.The environment BRDF approximation using Hirvonen's method and the specular hit distance output from the first specular bounce are correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (2)
290-298: Potential division by zero in footprint calculation.This issue was previously flagged: when
surface.Roughnessequals or approaches1.0, line 296 produces division by zero or NaN, corrupting theisValidHitcheck and SHaRC cache lookup.
344-348: Apply consistent color space conversion to PATH_TRACING output.This issue was previously flagged: the PATH_TRACING path outputs linear-space
direct + radiancedirectly (line 345), while the PT implementation converts to gamma viaColor::TrueLinearToGamma(). Bothdirectandradianceare in linear space from SampleSky and EvaluateDirectRadiance.
🧹 Nitpick comments (4)
features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli (2)
211-211: Remove commented-out code.The commented attenuation formula can be removed as it's been replaced by
InverseSquareAtten.🔎 Proposed fix
- // float atten = VanillaSquaredAtten(dist, light.Range); float atten = InverseSquareAtten(dist * GAME_UNIT_TO_M, light.Range * GAME_UNIT_TO_M);
117-117: Extract duplicate Efuzz calculation to a helper function.The complex Efuzz energy calculation appears identically in both
EvalFuzzBSDF(line 117) andSampleFuzzBSDF(line 312). Consider extracting this to a helper function to follow the DRY principle and improve maintainability.🔎 Proposed refactor
Add a helper function:
#if defined(FULL_MATERIAL) float ComputeFuzzEnergy(float roughness, float NdotV) { return (0.526422 / ((-0.227114 + roughness) * (-0.968835 + roughness) * ((5.38869 - 20.2835 * NdotV) * roughness) - (-1.18761 - ((2.58744 - NdotV) * NdotV)))) + 0.0615456; } #endifThen replace both occurrences:
- float Efuzz = (0.526422 / ((-0.227114 + surface.Roughness) * (-0.968835 + surface.Roughness) * ((5.38869 - 20.2835 * brdfContext.NdotV) * surface.Roughness) - (-1.18761 - ((2.58744 - brdfContext.NdotV) * brdfContext.NdotV)))) + 0.0615456; + float Efuzz = ComputeFuzzEnergy(surface.Roughness, brdfContext.NdotV);Also applies to: 312-312
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (2)
1-1: Consider more descriptive PR title.Per coding guidelines, the PR title "feat: raytracing" could be more specific. Consider:
feat(raytracing): add global illumination and path tracing supportfeat(renderer): implement dx12 raytracing with gi and shadowsThis follows conventional commits format
type(scope): descriptionand better describes the scope of changes.Based on coding guidelines, conventional commit titles should include scope and clear description.
61-65: Consider documenting TMin value differences.Primary rays use
TMin = 0.1f(line 64) while secondary bounce rays useTMin = 0.01f(line 243). If this difference is intentional (e.g., camera rays need larger offset, surface rays need tighter precision), a brief comment would clarify the design choice for future maintainers.Also applies to: 241-244
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli
🧠 Learnings (14)
📓 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.
📚 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.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 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl
📚 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl
📚 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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 (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 (3)
features/Raytracing/Shaders/Raytracing/Includes/RT/Shading.hlsli (2)
387-400: LGTM! Proper linear color space handling.The sky sampling correctly converts from gamma to linear space, which aligns with the project's linear lighting requirements for proper color blending and accumulation.
Based on learnings, proper linear color space handling is critical for raytracing operations.
1-410: Verify HLSL register/buffer usage to avoid conflicts.This file introduces new raytracing resources (Scene, Lights, SkyHemisphere, Frame, etc.) through included headers. Per coding guidelines, verify that register assignments don't conflict with existing features.
As per coding guidelines, HLSL register conflicts should be verified. Consider running hlslkit buffer scanning or manually reviewing
features/Raytracing/Shaders/Raytracing/Includes/Registers.hlslito ensure no overlaps with other feature registers.features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (1)
1-19: No action needed. TheRaytracing/Includes/Registers.hlslifile referenced in the include does not exist in the repository, and the code follows the established codebase convention where hardcoded register slot numbers are used directly in shader code (as seen throughout other Raytracing shaders likeShadowsRT.hlsl,SkinningCS.hlsl, etc.).Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (8)
src/Features/Raytracing/Utils.h (2)
97-134: Remove redundant break statements.The
breakstatements afterreturnare unreachable and can be removed for cleaner code.🔎 Proposed cleanup
static inline bool IsShareableFormat(DXGI_FORMAT format) { switch (format) { case DXGI_FORMAT_BC4_UNORM: return false; - break; case DXGI_FORMAT_BC4_SNORM: return false; - break; case DXGI_FORMAT_BC7_UNORM: return false; - break; case DXGI_FORMAT_BC7_UNORM_SRGB: return false; - break; default: return true; - break; } } static inline DXGI_FORMAT GetCompatibleFormat(DXGI_FORMAT format, bool recompress) { switch (format) { case DXGI_FORMAT_BC4_UNORM: return recompress ? DXGI_FORMAT_BC1_UNORM : DXGI_FORMAT_R8_UNORM; - break; case DXGI_FORMAT_BC7_UNORM: return recompress ? DXGI_FORMAT_BC3_UNORM : DXGI_FORMAT_R8G8B8A8_UNORM; - break; case DXGI_FORMAT_BC7_UNORM_SRGB: return recompress ? DXGI_FORMAT_BC3_UNORM_SRGB : DXGI_FORMAT_R8G8B8A8_UNORM_SRGB; - break; default: return format; - break; } }
176-181: Consider pass-by-const-reference for efficiency.Taking the string by value creates an unnecessary copy. Consider passing by const reference and explicitly returning a new string.
🔎 Proposed optimization
-static inline std::string ToLower(std::string s) +static inline std::string ToLower(const std::string& s) { - std::transform(s.begin(), s.end(), s.begin(), + std::string result = s; + std::transform(result.begin(), result.end(), result.begin(), [](unsigned char c) { return std::tolower(c); }); - return s; + return result; }src/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.h (1)
1-12: Include order:PCH.hshould be first.For precompiled headers to work correctly,
PCH.hshould be the first include. This applies to all the SVGF header files.🔎 Proposed fix
#pragma once +#include "PCH.h" + #include "Features/Raytracing/Buffer.h" #include "Features/Raytracing/Heap.h" #include "Features/Raytracing/HeapManager.h" #include "Features/Raytracing/Pipeline.h" #include "Features/Raytracing/Utils.h" -#include "PCH.h" #include <d3d12.h> #include "Features/Raytracing/Types.h"src/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.h (1)
43-52: Consider using a parameter struct forRegisterResources.The method has 10 parameters, which is difficult to maintain and easy to misorder. A struct would improve readability.
struct SVGFTemporalResources { DX12::Texture2D* temporalTexture; DX12::Texture2D* momentsTexture; DX12::Texture2D* historyTexture; ID3D12Resource* motionVectorResource; ID3D12Resource* normalRoughnessResource; ID3D12Resource* colorResource; ID3D12Resource* depthResource; DX12::Texture2D* historyMomentsTexture; DX12::Texture2D* historyNormalsTexture; }; void RegisterResources(ID3D12Device5* device, const SVGFTemporalResources& resources);src/Features/Raytracing/Pipeline.h (4)
11-18: Consider adding documentation comments.The interface design is clean with proper virtual destructor and pure virtual methods. However, adding brief documentation would help explain the purpose of each lifecycle method (CreateRootSignature, CompileShaders, SetupResources) and their expected calling order.
📝 Example documentation
+/// @brief Base interface for all pipeline types (raytracing, compute, etc.) +/// Defines the common initialization lifecycle for pipelines. struct IPipeline { virtual ~IPipeline() = default; + /// @brief Create the root signature defining parameter layout virtual void CreateRootSignature(ID3D12Device5* device) = 0; + /// @brief Compile all shaders required by this pipeline virtual void CompileShaders(ID3D12Device5* device) = 0; + /// @brief Allocate and setup GPU resources (buffers, heaps, etc.) virtual void SetupResources(ID3D12Device5* device) = 0; };
27-30: Consider consistent container usage.
MasterPipelineusesstd::vectorwhile the codebase elsewhere uses EASTL containers (eastl::unique_ptron line 24). For consistency, consider usingeastl::vectorunless there's a specific reason to use the standard library version here.🔎 Consistent container usage
struct MasterPipeline : IPipeline { - std::vector<eastl::unique_ptr<IPipeline>> subPipelines; + eastl::vector<eastl::unique_ptr<IPipeline>> subPipelines; };
32-36: Redundant nullptr initialization.Similar to the
Pipelinebase class, the explicitnullptrinitialization is redundant aswinrt::com_ptris already default-initialized to nullptr.🔎 Simplified initialization
template <IsHeap HeapType> struct RaytracingPipeline : Pipeline<HeapType> { - winrt::com_ptr<ID3D12StateObject> stateObject = nullptr; + winrt::com_ptr<ID3D12StateObject> stateObject; };
38-42: Redundant nullptr initialization.Similar to other pipeline classes, the explicit
nullptrinitialization is redundant aswinrt::com_ptris already default-initialized to nullptr.🔎 Simplified initialization
template <IsHeap HeapType> struct ComputePipeline : Pipeline<HeapType> { - winrt::com_ptr<ID3D12PipelineState> pipelineState = nullptr; + winrt::com_ptr<ID3D12PipelineState> pipelineState; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Features/Raytracing/Pipeline.hsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFVariance.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFVariance.hsrc/Features/Raytracing/Utils.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/Raytracing/Pipelines/SVGFPipeline.hsrc/Features/Raytracing/Pipeline.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFVariance.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFVariance.hsrc/Features/Raytracing/Utils.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Raytracing/Pipelines/SVGFPipeline.hsrc/Features/Raytracing/Pipeline.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFVariance.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFVariance.hsrc/Features/Raytracing/Utils.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:
src/Features/Raytracing/Pipelines/SVGFPipeline.hsrc/Features/Raytracing/Pipeline.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFVariance.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.hsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline/SVGFVariance.hsrc/Features/Raytracing/Utils.h
🧠 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.
📚 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:
src/Features/Raytracing/Pipelines/SVGFPipeline/SVGFVariance.cpp
📚 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:
src/Features/Raytracing/Utils.h
📚 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:
src/Features/Raytracing/Utils.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:
src/Features/Raytracing/Utils.h
📚 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:
src/Features/Raytracing/Utils.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:
src/Features/Raytracing/Utils.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). (3)
- 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)
🔇 Additional comments (12)
src/Features/Raytracing/Utils.h (8)
18-26: LGTM!The unpacking logic is correct and handles the bit shifts and normalization properly.
33-36: LGTM!The unpacking and remapping logic correctly inverts the PackByte4 operation.
44-53: LGTM!The function safely handles null pointers and provides appropriate fallback behavior.
55-90: LGTM!The transformation from NiTransform to XMMATRIX correctly handles rotation, scale, and translation components with proper matrix indexing.
92-95: LGTM!Simple and correct conversion utility.
136-156: LGTM!The texture sharing logic appropriately handles standard and PBR texture sets with correct fallback behavior.
158-174: LGTM!The template function correctly uses magic_enum for flag enumeration and formatting.
214-222: LGTM!Both overloads correctly implement divide-with-round-up for integer and floating-point divisors.
src/Features/Raytracing/Pipelines/SVGFPipeline/SVGFSpatial.h (1)
13-40: LGTM!The heap definition and pipeline structure follow the established pattern used across the SVGF sub-pipelines.
src/Features/Raytracing/Pipelines/SVGFPipeline/SVGFTemporal.h (1)
13-36: LGTM!The heap definition is comprehensive with clear slot assignments for UAV/SRV bindings.
src/Features/Raytracing/Pipeline.h (2)
1-10: LGTM - includes and dependencies are appropriate.The use of
#pragma onceis acceptable and widely supported. The included headers provide the necessary D3D12 and raytracing infrastructure.
20-25: This review comment references a file and code that do not exist in the repository. The filesrc/Features/Raytracing/Pipeline.hand theIsHeapconcept are not present in the codebase.Likely an incorrect or invalid review comment.
| static inline uint PackUByte4(float4 unpacked) | ||
| { | ||
| auto x = (uint)(unpacked.x * 255.0f) & 0xFF; | ||
| auto y = (uint)(unpacked.y * 255.0f) & 0xFF; | ||
| auto z = (uint)(unpacked.z * 255.0f) & 0xFF; | ||
| auto w = (uint)(unpacked.w * 255.0f) & 0xFF; | ||
|
|
||
| return (w << 24) | (z << 16) | (y << 8) | x; | ||
| } |
There was a problem hiding this comment.
Critical: Add input clamping to prevent incorrect packing.
Values outside [0, 1] will produce incorrect results. For example, unpacked.x = 1.5f casts to (uint)(1.5 * 255.0f) = 382, which after masking & 0xFF yields 126 instead of the expected 255.
🔎 Proposed fix with std::clamp
static inline uint PackUByte4(float4 unpacked)
{
- auto x = (uint)(unpacked.x * 255.0f) & 0xFF;
- auto y = (uint)(unpacked.y * 255.0f) & 0xFF;
- auto z = (uint)(unpacked.z * 255.0f) & 0xFF;
- auto w = (uint)(unpacked.w * 255.0f) & 0xFF;
+ auto x = static_cast<uint>(std::clamp(unpacked.x, 0.0f, 1.0f) * 255.0f);
+ auto y = static_cast<uint>(std::clamp(unpacked.y, 0.0f, 1.0f) * 255.0f);
+ auto z = static_cast<uint>(std::clamp(unpacked.z, 0.0f, 1.0f) * 255.0f);
+ auto w = static_cast<uint>(std::clamp(unpacked.w, 0.0f, 1.0f) * 255.0f);
return (w << 24) | (z << 16) | (y << 8) | x;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static inline uint PackUByte4(float4 unpacked) | |
| { | |
| auto x = (uint)(unpacked.x * 255.0f) & 0xFF; | |
| auto y = (uint)(unpacked.y * 255.0f) & 0xFF; | |
| auto z = (uint)(unpacked.z * 255.0f) & 0xFF; | |
| auto w = (uint)(unpacked.w * 255.0f) & 0xFF; | |
| return (w << 24) | (z << 16) | (y << 8) | x; | |
| } | |
| static inline uint PackUByte4(float4 unpacked) | |
| { | |
| auto x = static_cast<uint>(std::clamp(unpacked.x, 0.0f, 1.0f) * 255.0f); | |
| auto y = static_cast<uint>(std::clamp(unpacked.y, 0.0f, 1.0f) * 255.0f); | |
| auto z = static_cast<uint>(std::clamp(unpacked.z, 0.0f, 1.0f) * 255.0f); | |
| auto w = static_cast<uint>(std::clamp(unpacked.w, 0.0f, 1.0f) * 255.0f); | |
| return (w << 24) | (z << 16) | (y << 8) | x; | |
| } |
🤖 Prompt for AI Agents
In src/Features/Raytracing/Utils.h around lines 8 to 16, the PackUByte4 function
currently scales and casts float components without clamping which lets
out-of-range values wrap when masked; clamp each component to [0.0f, 1.0f]
before multiplying by 255.0f and casting to uint (e.g. use
std::clamp(unpacked.x, 0.0f, 1.0f) for each channel), add the necessary include
(<algorithm>) if using std::clamp, then proceed with scaling, cast and packing
so values >1 become 255 and <0 become 0.
| static inline uint PackByte4(float4 unpacked) | ||
| { | ||
| return PackUByte4(unpacked * 0.5f + float4(0.5f, 0.5f, 0.5f, 0.5f)); | ||
| } |
There was a problem hiding this comment.
Add input clamping for signed packing.
Values outside [-1, 1] will produce incorrect results after the mapping and subsequent PackUByte4 call. Consider clamping inputs to the valid range.
🔎 Proposed fix
static inline uint PackByte4(float4 unpacked)
{
- return PackUByte4(unpacked * 0.5f + float4(0.5f, 0.5f, 0.5f, 0.5f));
+ float4 clamped = float4(
+ std::clamp(unpacked.x, -1.0f, 1.0f),
+ std::clamp(unpacked.y, -1.0f, 1.0f),
+ std::clamp(unpacked.z, -1.0f, 1.0f),
+ std::clamp(unpacked.w, -1.0f, 1.0f)
+ );
+ return PackUByte4(clamped * 0.5f + float4(0.5f, 0.5f, 0.5f, 0.5f));
}🤖 Prompt for AI Agents
In src/Features/Raytracing/Utils.h around lines 28 to 31, the PackByte4 function
maps signed inputs to unsigned bytes without clamping so values outside [-1,1]
yield incorrect results; modify the function to first clamp the incoming float4
to the range [-1.0f, 1.0f], then perform the existing mapping (value * 0.5f +
0.5f) and call PackUByte4 on the clamped/mapped result so out-of-range inputs
are handled safely.
| static inline float3 Normalize(float3 vector) | ||
| { | ||
| vector.Normalize(); | ||
| return vector; | ||
| } |
There was a problem hiding this comment.
Critical: Guard against zero-length vectors.
Normalizing a zero-length vector results in NaN or Inf, which will propagate through raytracing calculations and corrupt results.
🔎 Proposed fix with epsilon check
static inline float3 Normalize(float3 vector)
{
+ constexpr float epsilon = 1e-8f;
+ float lengthSq = vector.SqrLength();
+ if (lengthSq < epsilon * epsilon) {
+ return float3(0.0f, 0.0f, 0.0f); // or float3(0, 0, 1) for a safe default
+ }
vector.Normalize();
return vector;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static inline float3 Normalize(float3 vector) | |
| { | |
| vector.Normalize(); | |
| return vector; | |
| } | |
| static inline float3 Normalize(float3 vector) | |
| { | |
| constexpr float epsilon = 1e-8f; | |
| float lengthSq = vector.SqrLength(); | |
| if (lengthSq < epsilon * epsilon) { | |
| return float3(0.0f, 0.0f, 0.0f); // or float3(0, 0, 1) for a safe default | |
| } | |
| vector.Normalize(); | |
| return vector; | |
| } |
🤖 Prompt for AI Agents
In src/Features/Raytracing/Utils.h around lines 38-42, the Normalize(float3)
function unconditionally calls vector.Normalize(), which will produce NaN/Inf
for zero-length vectors; change it to compute the squared length (or length) and
compare against a small epsilon (e.g. 1e-8f), only call Normalize() when
length^2 > epsilon, otherwise return a safe zero vector (or the original vector
set to zero) to avoid propagating NaNs; add a local const float EPS = 1e-8f if
not already defined.
| //if (pathLower.ends_with("_d.dds")) | ||
| // return true; |
There was a problem hiding this comment.
Remove commented-out code.
Per coding guidelines, commented-out code should be removed. If this logic is needed later, it can be retrieved from version control.
🔎 Proposed cleanup
auto pathLower = ToLower(path);
- //if (pathLower.ends_with("_d.dds"))
- // return true;
-
if (pathLower.ends_with("_n.dds"))
return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //if (pathLower.ends_with("_d.dds")) | |
| // return true; | |
| auto pathLower = ToLower(path); | |
| if (pathLower.ends_with("_n.dds")) | |
| return false; |
🤖 Prompt for AI Agents
In src/Features/Raytracing/Utils.h around lines 190-191, remove the two
commented-out lines ("//if (pathLower.ends_with("_d.dds"))" and "//\treturn
true;") as per coding guidelines; delete this dead code so the file contains no
commented-out logic (rely on version control if the logic is needed later).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Features/Raytracing.h (1)
997-1004: Consider removing commented-out code.Multiple blocks of commented code exist throughout the file. If this code is not needed, remove it to improve readability. If it's for future reference, consider documenting the intent or tracking via version control.
Also applies to: 1266-1282, 1303-1305, 1511-1512, 1521-1521, 1525-1526, 1530-1534
src/Features/Raytracing/Pipelines/SVGFPipeline.h (2)
18-27: Explicitnullptrinitialization is redundant.The
unique_ptrtemplate defaults tonullptr, making the explicit= nullptrinitialization redundant. While this doesn't cause issues, removing it aligns with modern C++ practices.🔎 Optional simplification
- eastl::unique_ptr<SVGFTemporal> temporalPipeline = nullptr; - eastl::unique_ptr<SVGFVariance> variancePipeline = nullptr; - eastl::unique_ptr<SVGFSpatial> spatialPipeline = nullptr; + eastl::unique_ptr<SVGFTemporal> temporalPipeline; + eastl::unique_ptr<SVGFVariance> variancePipeline; + eastl::unique_ptr<SVGFSpatial> spatialPipeline; - eastl::unique_ptr<DX12::Texture2D> temporalTexture = nullptr; - eastl::unique_ptr<DX12::Texture2D> momentsTexture = nullptr; - eastl::unique_ptr<DX12::Texture2D> varianceTexture = nullptr; - eastl::unique_ptr<DX12::Texture2D> historyMomentsTexture = nullptr; - eastl::unique_ptr<DX12::Texture2D> historyNormalsTexture = nullptr; - eastl::unique_ptr<DX12::Texture2D> historyTexture = nullptr; + eastl::unique_ptr<DX12::Texture2D> temporalTexture; + eastl::unique_ptr<DX12::Texture2D> momentsTexture; + eastl::unique_ptr<DX12::Texture2D> varianceTexture; + eastl::unique_ptr<DX12::Texture2D> historyMomentsTexture; + eastl::unique_ptr<DX12::Texture2D> historyNormalsTexture; + eastl::unique_ptr<DX12::Texture2D> historyTexture;
16-35: Add documentation for the pipeline lifecycle and member usage.The struct lacks documentation explaining:
- The purpose of the SVGF (Spatiotemporal Variance Guided Filtering) pipeline
- The initialization lifecycle (CreateRootSignature → CompileShaders → SetupResources → SetupTextureResources)
- The role of each sub-pipeline and texture member
- Parameter descriptions for public methods
Consider adding class-level and method-level documentation comments for better maintainability.
📝 Example documentation structure
/** * @brief SVGF (Spatiotemporal Variance Guided Filtering) denoising pipeline. * * Manages a three-stage denoising process: Temporal → Variance → Spatial. * * Initialization lifecycle: * 1. CreateRootSignature() - Initialize D3D12 root signatures * 2. CompileShaders() - Compile SVGF shaders * 3. SetupResources() - Allocate internal texture resources * 4. SetupTextureResources() - Register external texture dependencies * * @note All sub-pipeline and texture members are initialized during setup methods. */ struct SVGFPipeline : IPipeline { // Sub-pipeline stages eastl::unique_ptr<SVGFTemporal> temporalPipeline; eastl::unique_ptr<SVGFVariance> variancePipeline; eastl::unique_ptr<SVGFSpatial> spatialPipeline; // Working and history textures for denoising passes eastl::unique_ptr<DX12::Texture2D> temporalTexture; eastl::unique_ptr<DX12::Texture2D> momentsTexture; eastl::unique_ptr<DX12::Texture2D> varianceTexture; eastl::unique_ptr<DX12::Texture2D> historyMomentsTexture; eastl::unique_ptr<DX12::Texture2D> historyNormalsTexture; eastl::unique_ptr<DX12::Texture2D> historyTexture; void CreateRootSignature(ID3D12Device5* device) override; void CompileShaders(ID3D12Device5* device) override; void SetupResources(ID3D12Device5* device) override; /** * @brief Execute the three-stage SVGF denoising process. * @param commandList D3D12 command list for GPU commands */ void Denoise(ID3D12GraphicsCommandList4* commandList); // ... etc. };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Features/Raytracing.cppsrc/Features/Raytracing.hsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/Raytracing/Pipelines/SVGFPipeline.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/Raytracing/Pipelines/SVGFPipeline.hsrc/Features/Raytracing.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Raytracing/Pipelines/SVGFPipeline.hsrc/Features/Raytracing.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:
src/Features/Raytracing/Pipelines/SVGFPipeline.hsrc/Features/Raytracing.h
🧠 Learnings (13)
📓 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: 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 src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
src/Features/Raytracing.h
📚 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:
src/Features/Raytracing.h
📚 Learning: 2025-06-09T22:27:55.011Z
Learnt from: soda3000
Repo: doodlum/skyrim-community-shaders PR: 1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Applied to files:
src/Features/Raytracing.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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
src/Features/Raytracing.h
📚 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:
src/Features/Raytracing.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:
src/Features/Raytracing.h
📚 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:
src/Features/Raytracing.h
📚 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:
src/Features/Raytracing.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/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/Features/Raytracing.h
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/Features/Raytracing.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:
src/Features/Raytracing.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:
src/Features/Raytracing.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). (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)
| AO | ||
| }; | ||
|
|
||
| // TODO: Rename to ReflectanceModel? |
There was a problem hiding this comment.
Remove TODO comments per coding guidelines.
The coding guidelines explicitly require complete solutions without TODO/FIXME placeholders. Either implement the suggested changes or remove the comments.
As per coding guidelines, files matching **/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py} should not include TODO/FIXME placeholders.
Also applies to: 580-581, 590-590, 698-698, 796-799
🤖 Prompt for AI Agents
In src/Features/Raytracing.h around lines 387 (and also addresses similar TODOs
at 580-581, 590, 698, and 796-799), remove the TODO/FIXME placeholders per
coding guidelines by either implementing the suggested rename or eliminating the
comment; specifically, rename the identifier and any documentation/usages from
"ReflectanceModel?" to the intended final name (e.g., ReflectanceModel or
another chosen term) across the header and all references, update comments to
reflect the applied change, and commit the cleaned-up file so no TODO/FIXME
comments remain in these line ranges.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (26)
src/Features/Raytracing/Utils.h (5)
8-16: Critical: Add input clamping to prevent incorrect packing.Values outside [0, 1] will produce incorrect results due to overflow/underflow during the cast and mask operations.
28-31: Add input clamping for signed packing.Values outside [-1, 1] will produce incorrect results after the mapping to unsigned range.
38-42: Critical: Guard against zero-length vectors.Normalizing a zero-length vector results in NaN or Inf, corrupting raytracing calculations.
190-191: Remove commented-out code.Per coding guidelines, commented-out code should be removed.
235-249: Add error handling for CreateShaderResourceView.Per coding guidelines, plugin code should include robust error handling with graceful degradation.
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlsl (1)
7-36: Division by zero risk inGaussianBluralready flagged.This issue was previously identified -
kernelSumcan be zero if all samples fall outside valid bounds.features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlsl (2)
49-49: Inconsistent moment scaling with 0.5 factor.This was previously flagged - the 0.5 scaling factor on
curMomentcreates inconsistency withprevMomentcalculation and distorts variance computation per SVGF standards.
147-151:prevMomentrecomputed instead of using stored history.This was previously flagged - line 147-148 recomputes
prevMomentfromprevColor.rgbrather than using the already-loadedprevMomentsfromHistoryMomentsTexture, discarding accumulated moment history.src/Features/Raytracing.h (13)
3-3: Unconditional#define DLSS_RRin header.This was previously flagged - unconditional defines in headers affect all translation units. Consider moving to build system or using include guards.
46-46:NTDDI_VERSIONredefinition risk.Previously flagged - defining
NTDDI_VERSIONmid-header can conflict with platform headers.
64-64:using namespacedirective pollutes global namespace.Previously flagged -
using namespace magic_enum::bitwise_operators;in header affects all includers.
570-570: UninitializedHANDLE fenceEvent.Previously flagged - should be initialized to
nullptrorINVALID_HANDLE_VALUE.
1113-1116: Race condition:sharedTextures.erasewithout mutex.Previously flagged -
ID3D11Texture2D_Release::thunkerases fromsharedTextureswithout acquiringsharedTextureMutex.
1387-1388:CreateModelcalled with wrong number of arguments.Previously flagged - both
Clone3DBaseandClone3DcallCreateModelwith 2 arguments but it expects 3.Also applies to: 1413-1414
1667-1668: Missing null check forScriptEventSourceHolder::GetSingleton().Previously flagged -
TESLoadGameEventHandler::Register()directly dereferences the singleton without null checking.
1685-1686: Same missing null check inTESObjectLoadedEventHandler::Register().Previously flagged - apply the same null check pattern.
311-321:GetPipelinesstill uses static local with potential stale pointers.The static array is now wrapped with a null check for
sharcPipeline, but the array itself is still static and initialized once. IfsharcPipelineis recreated, the array will contain a stale pointer.This was previously flagged with a suggestion to clear and rebuild dynamically.
973-993: Exceptions in parallel algorithm causestd::terminate.Previously flagged -
DX::ThrowIfFailedinsidestd::execution::parloop will callstd::terminate()on failure.
385-385: TODO/FIXME comments violate coding guidelines.Previously flagged - per coding guidelines, complete working solutions should be provided instead of placeholders.
Also applies to: 579-580, 589-590, 697-698, 795-799
191-201: Placeholder metadata should be updated before release.Previously flagged - contains placeholder text ("Cheese.", "999999" mod ID).
706-709: Thread safety concern inInstance::Update.Previously flagged - appending to
rt.modelUpdateandrt.vertexUpdatevectors without synchronization while potentially called from multiple threads.features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (3)
75-89: InconsistentSpecularHitDistfor sky miss case.Line 87 writes
0.0ffor sky misses in PATH_TRACING, while line 126 writesRAY_TMAXfor the equivalent case in non-PATH_TRACING. This inconsistency could affect downstream denoisers. Consider usingRAY_TMAXfor both paths.
293-298: Division by zero whensurface.Roughnessequals 1.0.Line 296 computes
sqrt(0.5f * surface.Roughness / (1.0f - surface.Roughness)). When roughness is 1.0, this produces division by zero, resulting in inf/NaN that corrupts the validity check.🔎 Suggested fix
if (isValidHit) { - float footprint = payload.hitDistance * sqrt(0.5f * surface.Roughness / (1.0f - surface.Roughness)); + float footprint = payload.hitDistance * sqrt(0.5f * surface.Roughness / max(1.0f - surface.Roughness, 1e-4f)); isValidHit &= footprint > voxelSize; }
344-352: Verify color space consistency forPATH_TRACINGoutput.Line 345 outputs
direct + radiancein linear space, while the non-PATH_TRACING path at line 350 adds linear radiance to gamma-convertedmainColor. If downstream processing expects consistent color space, confirm whether PATH_TRACING output should also applyColor::TrueLinearToGamma()as noted in PT/RayGeneration.hlsl.#!/bin/bash # Check how PT/RayGeneration.hlsl handles output color space git show HEAD:features/Raytracing/Shaders/Raytracing/PT/RayGeneration.hlsl | grep -n "OutputTexture\|TrueLinearToGamma\|GammaToTrueLinear" -A1 -B1features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl (2)
51-52: Unused variableaoextracted but never used.Similar to the
mainentry point, theaovalue is unpacked but not used in any output or calculation.🔎 Simplify or document unused extraction
- float metallic, ao; - UnpackMAO(GNMAO[id].z, metallic, ao); + float metallic, unused_ao; + UnpackMAO(GNMAO[id].z, metallic, unused_ao);
53-53: CRITICAL: Missing color space conversion in main2 entry point.Line 53 multiplies
Albedo[id] * (1.0f - metallic)without converting from gamma to linear space. This is inconsistent with themainentry point at line 39, which correctly appliesColor::GammaToTrueLinear(albedo.rgb)before the same multiplication.The raytracing pipeline operates in true linear space, so both entry points must apply the gamma-to-linear conversion to ensure correct lighting calculations. The current implementation will produce incorrect diffuse values when
main2is used.🔎 Apply the same fix as line 39
- Diffuse[id] = Albedo[id] * (1.0f - metallic); + const float4 albedo = Albedo[id]; + Diffuse[id] = float4(Color::GammaToTrueLinear(albedo.rgb) * (1.0f - metallic), albedo.a);Based on learnings: The raytracing pipeline requires true linear color space for correct lighting computations.
🧹 Nitpick comments (4)
src/Features/Raytracing/Utils.h (1)
97-134: Remove unnecessary break statements after returns.In both
IsShareableFormatandGetCompatibleFormat, thebreakstatements afterreturnare unreachable and can be removed for cleaner code.🔎 Proposed cleanup
static inline bool IsShareableFormat(DXGI_FORMAT format) { switch (format) { case DXGI_FORMAT_BC4_UNORM: return false; - break; case DXGI_FORMAT_BC4_SNORM: return false; - break; case DXGI_FORMAT_BC7_UNORM: return false; - break; case DXGI_FORMAT_BC7_UNORM_SRGB: return false; - break; default: return true; - break; } } static inline DXGI_FORMAT GetCompatibleFormat(DXGI_FORMAT format, bool recompress) { switch (format) { case DXGI_FORMAT_BC4_UNORM: return recompress ? DXGI_FORMAT_BC1_UNORM : DXGI_FORMAT_R8_UNORM; - break; case DXGI_FORMAT_BC7_UNORM: return recompress ? DXGI_FORMAT_BC3_UNORM : DXGI_FORMAT_R8G8B8A8_UNORM; - break; case DXGI_FORMAT_BC7_UNORM_SRGB: return recompress ? DXGI_FORMAT_BC3_UNORM_SRGB : DXGI_FORMAT_R8G8B8A8_UNORM_SRGB; - break; default: return format; - break; } }features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlsl (1)
14-14: Unused variableuv.The computed UV coordinate is never used in the shader.
🔎 Remove unused variable
- float2 uv = float2(DTid.xy + 0.5) * ResolutionRcp;features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlsli (1)
64-69: Dead code:prevScreenis computed but immediately overwritten.Line 65 computes
prevScreenfrom the reprojected clip coordinates, but it is immediately overwritten on line 69 using velocity-based calculation. The matrix reprojection on lines 60-65 appears to be dead code.Either remove the unused matrix reprojection or use a blend of both approaches if that was the intent.
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl (1)
35-36: Unused variableaoextracted but never used.The
ao(ambient occlusion) value is unpacked from GNMAO but is not written to any output or used in calculations. If ambient occlusion is not needed for this conversion pass, consider removing the extraction or document why it's extracted but unused.🔎 Simplify UnpackMAO call
If only metallic is needed:
- float metallic, ao; - UnpackMAO(GNMAO.SampleLevel(Sampler, uv, 0).z, metallic, ao); + float metallic, unused_ao; + UnpackMAO(GNMAO.SampleLevel(Sampler, uv, 0).z, metallic, unused_ao);Or if UnpackMAO signature allows, extract only metallic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslfeatures/Raytracing/Shaders/Raytracing/CopyDepthCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlslifeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SVGF.hlslifeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing.cppsrc/Features/Raytracing.hsrc/Features/Raytracing/Pipeline.hsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing/Pipelines/SVGFPipeline.hsrc/Features/Raytracing/Utils.h
🚧 Files skipped from review as they are similar to previous changes (4)
- features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SVGF.hlsli
- features/Raytracing/Shaders/Raytracing/CopyDepthCS.hlsl
- src/Features/Raytracing/Pipeline.h
- src/Features/Raytracing/Pipelines/SVGFPipeline.h
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlslifeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlslsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing.hfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing/Utils.h
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlslifeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlslfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.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:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlslifeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlslsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing.hfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing/Utils.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing.hsrc/Features/Raytracing/Utils.h
🧠 Learnings (22)
📓 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: 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-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:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlslifeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlslsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing/Utils.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} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/VarianceCS.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlslifeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlslsrc/Features/Raytracing/Pipelines/SVGFPipeline.cppsrc/Features/Raytracing.hfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing/Utils.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:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlslifeatures/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing.hfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing/Utils.h
📚 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:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlslisrc/Features/Raytracing.hsrc/Features/Raytracing/Utils.h
📚 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:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/Common.hlslisrc/Features/Raytracing.hsrc/Features/Raytracing/Utils.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing.hsrc/Features/Raytracing/Utils.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing/Utils.h
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlslfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing.hfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslsrc/Features/Raytracing/Utils.h
📚 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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslsrc/Features/Raytracing.hsrc/Features/Raytracing/Utils.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.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:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslfeatures/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl
📚 Learning: 2025-05-30T11:44:15.542Z
Learnt from: davo0411
Repo: doodlum/skyrim-community-shaders PR: 1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
Applied to files:
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.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:
src/Features/Raytracing.h
📚 Learning: 2025-06-09T22:27:55.011Z
Learnt from: soda3000
Repo: doodlum/skyrim-community-shaders PR: 1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Applied to files:
src/Features/Raytracing.h
📚 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:
src/Features/Raytracing.hsrc/Features/Raytracing/Utils.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/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/Features/Raytracing.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 src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Include robust error handling and resource management with graceful degradation in the plugin code
Applied to files:
src/Features/Raytracing/Utils.h
🔇 Additional comments (8)
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/SpatialCS.hlsl (1)
51-51: Output is black (zero) for pixels with invalid depth.When
depthCenter <= 0,blendedColorremains at its initial value of0, resulting in a black pixel output. If this is intentional for invalid depth regions, consider adding a comment. Otherwise, you may want to fallback to the originalssrColor.rgb.Also applies to: 110-112
features/Raytracing/Shaders/Raytracing/Denoiser/SVGF/TemporalCS.hlsl (1)
1-11: LGTM on resource bindings.The texture and UAV register assignments are well-organized with clear naming. The sampler binding at s0 is appropriate for the bilinear sampling needs.
src/Features/Raytracing.h (1)
205-205: VR support explicitly disabled.
SupportsVR()returnsfalse. Per coding guidelines, ensure SE/AE/VR runtime compatibility. If VR is intentionally unsupported for this feature, this is fine, but consider adding a comment explaining why.features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl (5)
1-18: LGTM!Include structure is well-organized with clear dependency ordering from types through SHaRC integration, common utilities, and finally PBR/MonteCarlo sampling.
20-49: LGTM!The SHaRC update pass correctly implements a 5x5 block pattern with temporal spreading via hash-based start indices. The bounds check at line 42-43 properly guards against out-of-bounds dispatch indices.
101-153: LGTM!G-buffer reconstruction correctly handles color space conversion (gamma to linear for albedo), proper coordinate transformations, and the depth offset (0.99998) is a standard technique for z-precision.
162-181: LGTM!Variable declarations are clean with proper initialization for ray tracing loop state and conditional SHaRC variables.
354-358: LGTM!The specular albedo calculation using Hirvonen's environment BRDF approximation follows the standard split-sum pattern for real-time rendering.
| [numthreads(8, 8, 1)] | ||
| void main2(uint2 id : SV_DispatchThreadID) | ||
| { | ||
| const unorm half3 normalGlossiness = NormalGlossiness[id].xyz; | ||
| const snorm half3 normalWS = normalize(ViewToWorldVector(GBuffer::DecodeNormal(normalGlossiness.xy), FrameBuffer::CameraViewInverse[0])); | ||
| NormalRoughness[id] = half4(normalWS, 1.0f - normalGlossiness.z); | ||
|
|
||
| float metallic, ao; | ||
| UnpackMAO(GNMAO[id].z, metallic, ao); | ||
| Diffuse[id] = Albedo[id] * (1.0f - metallic); | ||
|
|
||
| MotionVectorsOut[id] = MotionVectors[id]; | ||
| } |
There was a problem hiding this comment.
Missing bounds check in main2 could cause out-of-bounds writes.
The main2 entry point lacks the bounds check present in main (lines 26-27). If the compute dispatch size exceeds the texture dimensions, this will result in out-of-bounds writes to the output textures.
🔎 Add bounds check consistent with main
[numthreads(8, 8, 1)]
void main2(uint2 id : SV_DispatchThreadID)
{
+ if (any(id >= RenderRes))
+ return;
+
const unorm half3 normalGlossiness = NormalGlossiness[id].xyz;🤖 Prompt for AI Agents
In features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl around lines 44
to 56, the main2 entry point is missing the bounds check present in main (around
lines 26-27), which can allow out-of-bounds texture writes when dispatch >
texture size; add the same early-return bounds check at the top of main2
(compare SV_DispatchThreadID.x/y against the target texture/dispatch dimensions
used in main and return if outside) so threads outside the valid pixel rectangle
do nothing, matching the existing main implementation.
🧩 Analysis chain
🏁 Script executed:
find . -name "ConvertTexturesCS.hlsl" -type fRepository: doodlum/skyrim-community-shaders
Length of output: 139
🏁 Script executed:
# Search for references to main2 as a shader entry point in C++ files
rg -n "main2" --type cpp --type h -C 3Repository: doodlum/skyrim-community-shaders
Length of output: 224
🏁 Script executed:
# Search for references to ConvertTexturesCS in C++ files
rg -n "ConvertTexturesCS" --type cpp --type h -C 3Repository: doodlum/skyrim-community-shaders
Length of output: 224
🏁 Script executed:
cat -n ./features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 2467
🏁 Script executed:
# Search for main2 without type filters
rg -n "main2" -C 3Repository: doodlum/skyrim-community-shaders
Length of output: 809
🏁 Script executed:
# Search for ConvertTexturesCS without type filters
rg -n "ConvertTexturesCS" -C 3Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Search more broadly for any reference to main2 entry point
rg -i "main2|entry.*point" -C 2Repository: doodlum/skyrim-community-shaders
Length of output: 520
🏁 Script executed:
# Check for any shader compilation or dispatch patterns
rg -n "Dispatch|compute.*shader|entry" --type toml --type json --type yaml -C 2Repository: doodlum/skyrim-community-shaders
Length of output: 224
Remove the unused main2 entry point.
The file contains two compute shader entry points: main (lines 24-42, which is correct) and main2 (lines 45-56, which is incorrect and unused). The main2 entry point:
- Uses direct indexing instead of filtered sampling
- Lacks the bounds check present in
main - Missing color space conversion (
Color::GammaToTrueLinear) thatmaincorrectly applies on line 39
Since main2 is not referenced anywhere in the codebase, it should be removed entirely rather than maintained as dead code or fixed.
🤖 Prompt for AI Agents
In features/Raytracing/Shaders/Raytracing/ConvertTexturesCS.hlsl around lines 44
to 56, the unused compute shader entry point main2 should be removed: delete the
entire main2 function (the [numthreads...] declaration and its body) so only the
correct main entry point remains; ensure no remaining references to main2 exist
and that the file retains the bounds checks, filtered sampling, and
Color::GammaToTrueLinear usage present in the original main.
| #if defined(SHARC) && defined(SHARC_DEBUG) | ||
| HashGridParameters gridParameters = GetSharcGridParameters(); | ||
|
|
||
| OutputTexture[idx] = float4(HashGridDebugColoredHash(positionWS, geometryNormalWS, gridParameters), 1); | ||
| return; | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file size and context
wc -l features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 140
🏁 Script executed:
# Examine the code structure around lines 100-165 to see the full context
sed -n '100,165p' features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 2630
🏁 Script executed:
# Search for PATH_TRACING definition and SHARC_DEBUG to understand conditional compilation
rg -n "PATH_TRACING|SHARC_DEBUG" features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 181
🏁 Script executed:
# Look for where positionWS and geometryNormalWS are declared
rg -n "positionWS|geometryNormalWS" features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 508
🏁 Script executed:
# Get lines 51-160 to see the complete #if/#else/#endif structure
sed -n '51,160p' features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 4911
🏁 Script executed:
# Check if there are any includes that might define these variables in PATH_TRACING branch
head -50 features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl | rg -n "include|PATH_TRACING|SHARC"Repository: doodlum/skyrim-community-shaders
Length of output: 733
🏁 Script executed:
# Look for all #endif markers around line 150-160 to understand the structure
sed -n '145,165p' features/Raytracing/Shaders/Raytracing/GI/RayGeneration.hlsl | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 965
positionWS and geometryNormalWS undefined when PATH_TRACING is defined.
The SHARC_DEBUG block (lines 155-160) uses positionWS and geometryNormalWS which are only declared in the non-PATH_TRACING branch (lines 107 and 142). Since this debug block exists outside the PATH_TRACING conditional, defining both PATH_TRACING and SHARC_DEBUG will cause a compile error due to undefined variables.
Either guard the SHARC_DEBUG block with !defined(PATH_TRACING) or ensure these variables are available in both code paths.
| auto resetViews = [&]() { | ||
| srvs.fill(nullptr); | ||
| uavs.fill(nullptr); | ||
|
|
||
| context->CSSetShaderResources(0, (uint)srvs.size(), srvs.data()); | ||
| context->CSSetUnorderedAccessViews(0, (uint)srvs.size(), uavs.data(), nullptr); | ||
| }; |
There was a problem hiding this comment.
Incorrect array size passed to CSSetUnorderedAccessViews.
Line 98 passes srvs.size() (7) instead of uavs.size() (2) for the UAV count, which could cause issues or unnecessary work.
🔎 Proposed fix
auto resetViews = [&]() {
srvs.fill(nullptr);
uavs.fill(nullptr);
context->CSSetShaderResources(0, (uint)srvs.size(), srvs.data());
- context->CSSetUnorderedAccessViews(0, (uint)srvs.size(), uavs.data(), nullptr);
+ context->CSSetUnorderedAccessViews(0, (uint)uavs.size(), uavs.data(), nullptr);
};🤖 Prompt for AI Agents
In src/Features/Raytracing/Pipelines/SVGFPipeline.cpp around lines 93 to 99, the
lambda resetViews incorrectly passes srvs.size() as the UAV count to
CSSetUnorderedAccessViews; change the third argument to use uavs.size() (cast to
uint) and pass uavs.data() so the call becomes CSSetUnorderedAccessViews(0,
(uint)uavs.size(), uavs.data(), nullptr); keep CSSetShaderResources as-is with
srvs.size().
| } | ||
|
|
||
| context->CopyResource(historyNormalsTexture->resource.get(), normalRoughness->resource11); | ||
| //context->CopyResource(texOutput->resource.get(), texSSRColor->resource.get()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
As per coding guidelines, files should not contain TODO/FIXME placeholders or commented-out code. Either remove this line or implement the intended logic.
🤖 Prompt for AI Agents
In src/Features/Raytracing/Pipelines/SVGFPipeline.cpp around line 169, there is
a commented-out line "context->CopyResource(texOutput->resource.get(),
texSSRColor->resource.get());" which violates the guideline against
commented-out code; either remove this commented line entirely or, if the copy
was intended, restore and implement the CopyResource call correctly (ensuring
proper resource pointers and any required state transitions or error checks) so
the code compiles and behaves as expected.
add more ll control and correct isl
correct hair color
|
The feature has evolved on a different branch, so this PR is being closed. A new PR reflecting its updated state will be opened. |
2 participants
Adds Ray-tracing support in the form of a feature that manages game resources translated to DirectX12 compatible formats, its meant to be a housing for raytraced effects.
At the current time of this Draft PR creation it has the following effects:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.