perf(Terrain Variation): feature rework, miplevel & sample optimisation#1224
Conversation
WalkthroughThis update refactors terrain variation and blending logic in shaders and related code. It introduces modular helpers for terrain height blending, simplifies and optimizes stochastic sampling, and streamlines settings structures and UI by removing distance-based and advanced options. Shader function signatures are updated for consistency, and shadow contrast enhancements are added for terrain variation. Changes
Sequence Diagram(s)sequenceDiagram
participant Shader as ExtendedMaterials.hlsli
participant TerrainVar as TerrainVariation.hlsli
Shader->>Shader: Call GetTerrainHeight(...)
alt TERRAIN_VARIATION defined
Shader->>TerrainVar: Use stochastic sampling for heights
TerrainVar-->>Shader: Return blended heights
else
Shader->>Shader: Use standard sampling for heights
end
Shader->>Shader: ProcessTerrainHeightWeights(...)
Shader->>Shader: Blend and normalize weights
Shader->>Shader: Compute parallax and shadow as needed
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (5)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli(8 hunks)features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)package/Shaders/Common/SharedData.hlsli(1 hunks)src/Features/TerrainVariation.cpp(3 hunks)src/Features/TerrainVariation.h(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
package/Shaders/Common/SharedData.hlsli (4)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
src/Features/TerrainVariation.cpp (1)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
src/Features/TerrainVariation.h (4)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#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.
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (2)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: davo0411
PR: doodlum/skyrim-community-shaders#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.
🧬 Code Graph Analysis (2)
src/Features/TerrainVariation.cpp (3)
src/Features/TerrainShadows.h (1)
settings(100-100)src/Features/WetnessEffects.cpp (2)
RestoreDefaultSettings(766-775)RestoreDefaultSettings(766-766)src/Features/ExtendedTranslucency.cpp (2)
RestoreDefaultSettings(130-133)RestoreDefaultSettings(130-130)
src/Features/TerrainVariation.h (1)
src/Feature.h (2)
o_json(81-81)o_json(82-82)
🔇 Additional comments (9)
src/Features/TerrainVariation.cpp (1)
82-90: LGTM!The implementation of
RestoreDefaultSettings()andDrawFailLoadMessage()follows the established patterns in the codebase.src/Features/TerrainVariation.h (1)
38-43: LGTM! Proper struct alignment and HLSL compatibility.The
Settingsstruct correctly usesuintfor boolean flags and is properly aligned to 16 bytes, following the project's HLSL compatibility requirements.features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (3)
71-86: Good optimization for VR terrain variation.The reduced mip level penalty (0.5 vs 1.0) for VR when terrain variation is active helps prevent over-blurring while maintaining performance. The additional adjustments for non-VR terrain variation (mip + 2.0) appropriately compensate for the sharper appearance of stochastic sampling.
95-127: Well-structured height weight processing.The
ProcessTerrainHeightWeightsfunction properly modularizes the height-based blending logic with:
- Correct weight normalization using reciprocal
- Appropriate power functions for height influence
- Clear separation of concerns
This improves code maintainability and reusability.
567-574: Effective shadow contrast enhancement for terrain variation.The shadow intensity boost (pow 1.15) appropriately compensates for the visual characteristics of stochastic sampling, maintaining shadow quality while the terrain appears sharper.
Also applies to: 593-600
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
11-24: Excellent constant organization and documentation.The constants are well-organized with clear comments explaining their purpose. The pre-computed matrices and weights avoid runtime calculations, improving performance.
140-147: Smart optimization with early exit for high mip levels.The early exit when
mipFactor >= 0.8avoids unnecessary texture samples at distant LODs where the difference would be imperceptible, providing a good performance optimization.
182-186: Verify the runtime check doesn't cause performance issues.The runtime branch checking
enableTilingFixcould potentially impact performance if this function is called frequently. Consider whether this check should be moved to a higher level to avoid per-sample branching.Could you verify if this runtime check might cause performance issues? If the function is called in a tight loop, it might be better to have separate shader permutations or move the check to a higher level.
154-176: Sophisticated height-based blending implementation.The implementation correctly:
- Uses luminance for height detection when alpha is not available
- Properly normalizes weights to prevent color shifts
- Smoothly transitions between quality levels based on mip level
This provides high-quality terrain blending while maintaining performance.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
…1/skyrim-community-shaders into terrain-variation-rework
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (2)
129-196: Terrain height boost might be too aggressive.The 30% height boost when terrain variation tiling fix is enabled (lines 192-195) seems quite significant and could potentially cause visual artifacts or inconsistencies.
Consider making this boost configurable or reducing it to a more conservative value like 10-15%:
- [branch] if (SharedData::terrainVariationSettings.enableTilingFix) { - total *= 1.3; - } + [branch] if (SharedData::terrainVariationSettings.enableTilingFix) { + total *= 1.15; // More conservative 15% boost + }
196-196: Fix formatting issue with closing brace.There's a tab character before the closing brace that should be removed for consistency.
- return total; } + return total; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (2)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms (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)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (4)
8-10: LGTM: Simplified conditional inclusion.The removal of the
EXTENDED_MATERIALScondition simplifies the logic while maintaining the same functional behavior.
44-87: Well-implemented mip level adjustments for VR and terrain variation.The enhanced mip level calculation logic appropriately handles different scenarios:
- Reduced texture dimensions for non-parallax/non-PBR and VR cases
- Balanced VR mip penalties when terrain variation is active
- Increased mip levels for terrain variation to match vanilla appearance
The conditional logic correctly prevents over-blurring in VR while maintaining visual quality.
404-420: Consistent removal of distance parameter from GetTerrainHeight calls.The removal of the
distanceparameter from allGetTerrainHeightcalls aligns with the PR objectives to remove distance-based blending logic. The function signature updates are applied consistently across all call sites.
544-626: Enhanced shadow contrast implementation looks reasonable.The conditional shadow contrast enhancement (lines 581-587 and 611-617) applies a subtle power curve adjustment (0.8) to improve visual quality when terrain variation tiling fix is enabled. The implementation correctly maintains the original behavior when the feature is disabled.
The logic is duplicated between TRUE_PBR and non-TRUE_PBR branches, which is appropriate given the different scaling requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (1)
107-176: Excellent sampling functions with smart performance optimizations.The functions implement sophisticated blending with several performance optimizations:
- Early exits at high mip levels (>= 0.8 factor) reduce unnecessary sampling
- Smooth mip level transitions prevent visual artifacts
- Luminance-based height calculations respect terrain detail effectively
- The blend between high-quality and single samples maintains visual quality
Consider extracting the magic numbers (3.5, 2.5, 6.0, 0.8) into named constants for better maintainability:
+static const float MIP_BLEND_START = 3.5; +static const float MIP_BLEND_RANGE = 2.5; +static const float EARLY_EXIT_THRESHOLD = 0.8;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli(8 hunks)features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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: davo0411
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
🔇 Additional comments (5)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (5)
11-24: Well-structured constants with good documentation.The constant definitions are well-organized and documented. The pre-computed SKEW_MATRIX and hash multipliers are good performance optimizations.
Consider documenting the derivation of WORLD_SCALE = 332.54 if it's based on specific Skyrim world measurements or texture scaling requirements.
36-47: Optimized hash functions with good performance characteristics.The hash functions are well-implemented with pre-computed constants that avoid runtime calculations. The specialized
hashLODfunction is a good optimization for LOD terrain sampling.
49-68: Solid utility functions with efficient implementations.The
NormalizeWeightsfunction correctly usesrcp()for performance, andComputeBarycentricVertsimplements proper barycentric coordinate calculation for triangular grids. The conditional logic correctly handles both barycentric coordinate cases.The barycentric computation is mathematically sound and follows standard practices for stochastic sampling on triangular grids.
70-103: Well-designed offset computation functions with appropriate optimizations.The functions correctly implement different strategies for regular terrain versus LOD terrain:
ComputeStochasticOffsetsuses full barycentric coordinates for high-quality samplingComputeStochasticOffsetsLODis optimized with only 2 samples for distant terrainThe LOD_SCALE calculation comment mentions a specific computation (~255437). Consider verifying this calculation is correct based on the referenced formula.
178-213: Well-optimized performance variant with smart runtime checks.The function provides an excellent performance-focused alternative to the main stochastic effect:
- Runtime check for
enableTilingFixprevents unnecessary work when disabled- Proper use of
[branch]attribute for conditional execution- Fallback to regular sampling maintains compatibility
- Simplified barycentric blending without height influence reduces computational cost
The early exit logic and mip level handling are consistent with the main function, ensuring visual coherence across different performance paths.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (3)
140-147: Consider documenting the mip level threshold values.The magic numbers for mip transition (5, 2, 0.6) would benefit from named constants for better maintainability.
+// Mip level thresholds for quality transitions +static const float MIP_TRANSITION_START = 5.0; +static const float MIP_TRANSITION_RANGE = 2.0; +static const float MIP_EARLY_EXIT_THRESHOLD = 0.6; + // Calculate smooth transition factor - starts blending at mip 4.5, fully single sample at mip 7.0 -float mipFactor = saturate((mipLevel - 5) / 2); +float mipFactor = saturate((mipLevel - MIP_TRANSITION_START) / MIP_TRANSITION_RANGE); // Early exit for very high mip levels - single sample is sufficient -if (mipFactor >= 0.6) +if (mipFactor >= MIP_EARLY_EXIT_THRESHOLD)
183-187: Branch optimization may not be beneficial here.The
[branch]attribute on a simple boolean check may not provide performance benefits and could hurt performance on some GPU architectures due to divergent execution.// Check if terrain variation is actually enabled at runtime -[branch] if (!SharedData::terrainVariationSettings.enableTilingFix) +if (!SharedData::terrainVariationSettings.enableTilingFix) { // Fall back to regular sampling when terrain variation is disabled return tex.SampleLevel(samp, uv, mipLevel + SharedData::MipBias); }
199-202: Consider removing redundant branch attribute.Similar to the previous case, the
[branch]attribute on a simple numeric comparison may not be beneficial.// Early exit for very high mip levels - single sample is sufficient -[branch] if (mipFactor >= 0.6) +if (mipFactor >= 0.6) { return sample1; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms (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/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (3)
11-24: Constants section is well-structured with good documentation.The static constants are appropriately defined for compile-time optimization. The documentation clearly explains the purpose and constraints of each value.
83-103: LOD computation logic looks solid with good performance optimizations.The precomputed LOD_SCALE constant and simplified two-sample approach are good optimizations for distant terrain rendering.
107-128: LOD sampling function is well-optimized for performance.The two-sample approach with pseudo-rotation and appropriate mip bias handling provides good quality-to-performance ratio for distant terrain.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (2)
43-47: Critical bug: hashLOD function has incorrect return type.The function signature declares
float2return type but actually returns afloatvalue.
49-53: Consider adding division by zero protection.The weight normalization uses
rcp()without checking if the sum is zero, which could cause issues with degenerate cases.
🧹 Nitpick comments (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (1)
85-86: Consider adding a comment explaining the LOD_SCALE calculation.The comment mentions "~255437" but the actual value is exactly 255437.0. Consider clarifying the derivation for future maintenance.
-// Precomputed scaling: (WORLD_SCALE / 0.010416667) * 8.0 = ~255437 +// Precomputed scaling: (WORLD_SCALE / 0.010416667) * 8.0 = 255437.0 exactly static const float LOD_SCALE = 255437.0;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli(8 hunks)features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (5)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
11-24: Excellent use of static constants for performance optimization.The pre-computed constants approach eliminates runtime calculations and improves performance. The luminance weights correctly use Rec. 709 standard values.
55-68: Well-designed modular barycentric coordinate calculation.The function correctly implements triangle-based stochastic sampling with proper barycentric coordinate handling for both triangle orientations.
183-187: Excellent runtime optimization with branch condition.The early exit when terrain variation is disabled provides good performance when the feature is not needed.
158-167: Robust height calculation with alpha fallback.The luminance-based height calculation with alpha channel fallback provides good terrain detail preservation. The use of standard Rec. 709 luminance weights is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
43-47: Critical bug: hashLOD function has incorrect return type.The function signature declares
float2return type but actually returns afloatvalue. This causes a type mismatch since the calling code expects afloat2.inline float2 hashLOD(float2 p) { p = frac(p * 0.318); - return frac(p.x + p.y * 17.0); + return frac(p * float2(17.0, 31.0) + p.yx); }
49-53: Consider adding division by zero protection.The weight normalization uses
rcp()without checking if the sum is zero, which could cause issues with degenerate cases.inline float3 NormalizeWeights(float3 weights) { - float rcpWeightSum = rcp(weights.x + weights.y + weights.z); + float weightSum = weights.x + weights.y + weights.z; + float rcpWeightSum = rcp(max(weightSum, 1e-6)); return weights * rcpWeightSum; }
140-147: Replace magic numbers with named constants for better maintainability.The mip level transition thresholds are hardcoded and repeated in multiple functions, making them difficult to maintain and tune.
+// Mip level transition constants +static const float MIP_TRANSITION_START = 5.0; +static const float MIP_TRANSITION_RANGE = 2.0; +static const float EARLY_EXIT_THRESHOLD = 0.6; + // Calculate smooth transition factor - starts blending at mip 4.5, fully single sample at mip 7.0 -float mipFactor = saturate((mipLevel - 5) / 2); +float mipFactor = saturate((mipLevel - MIP_TRANSITION_START) / MIP_TRANSITION_RANGE); // Early exit for very high mip levels - single sample is sufficient -if (mipFactor >= 0.6) +if (mipFactor >= EARLY_EXIT_THRESHOLD)
195-202: Apply the same magic number constants here for consistency.The same mip level transition logic should use the same constants as the main function to ensure consistency and maintainability.
// Calculate smooth transition factor - starts blending at mip 4.5, fully single sample at mip 7.0 -float mipFactor = saturate((mipLevel - 5) / 2); +float mipFactor = saturate((mipLevel - MIP_TRANSITION_START) / MIP_TRANSITION_RANGE); // Early exit for very high mip levels - single sample is sufficient -[branch] if (mipFactor >= 0.6) +[branch] if (mipFactor >= EARLY_EXIT_THRESHOLD)
🧹 Nitpick comments (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (1)
183-187: Runtime branch check may impact performance.The branch check for terrain variation being enabled will be evaluated for every pixel/sample. Consider moving this check to a higher level if possible, or ensure the branch is coherent across shader invocations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli(8 hunks)features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (5)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms (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 (8)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (8)
11-24: Well-structured constants section improves performance and maintainability.The introduction of precomputed constants (SKEW_MATRIX, WORLD_SCALE, LUMINANCE_WEIGHTS, etc.) eliminates runtime calculations and centralizes configuration values. The height blend constants are well-documented with their performance rationale.
36-41: Hash function implementation looks correct.The hash2D2D function properly uses the new constants and implements a standard 2D hash function pattern.
55-68: Barycentric coordinate calculation is mathematically sound.The ComputeBarycentricVerts function correctly implements the skew transformation and barycentric coordinate calculation for triangular tiling. The conditional logic properly handles the two triangle cases.
70-81: Modular offset computation improves code organization.The ComputeStochasticOffsets function properly separates the offset calculation logic and reuses the barycentric computation. The hash-based offset generation is appropriate for stochastic sampling.
83-103: LOD-optimized sampling approach is well-designed.The ComputeStochasticOffsetsLOD function uses a simplified 2-sample approach for performance and includes precomputed scaling constants. The reduced sample count is appropriate for distant terrain where detail differences are less noticeable.
108-128: LOD sampling function demonstrates good performance optimizations.The StochasticSampleLOD function uses efficient pseudo-rotation and simple scaled offsets. The 2-sample blend with weighted preference is appropriate for LOD terrain.
154-177: Height-based blending logic is sophisticated and well-implemented.The luminance-based height calculation with alpha fallback provides good visual quality. The weight calculation combines barycentric coordinates with height influence effectively, and the smooth mip transition maintains performance at distance.
208-215: Performance-optimized variant provides good fallback option.The StochasticEffectNoHeight function appropriately removes height calculations for better performance while maintaining the stochastic sampling benefits. The simple barycentric blending is suitable for this optimization level.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (2)
47-51: Critical bug: hashLOD function has incorrect return type.The function signature declares
float2return type but actually returns afloatvalue.inline float2 hashLOD(float2 p) { p = frac(p * 0.318); - return frac(p.x + p.y * 17.0); + return frac(p * float2(17.0, 31.0) + p.yx); }
53-57: Consider adding division by zero protection.The weight normalization uses
rcp()without checking if the sum is zero, which could cause issues with degenerate cases.inline float3 NormalizeWeights(float3 weights) { - float rcpWeightSum = rcp(weights.x + weights.y + weights.z); + float weightSum = weights.x + weights.y + weights.z; + float rcpWeightSum = rcp(max(weightSum, 1e-6)); return weights * rcpWeightSum; }
🧹 Nitpick comments (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (1)
60-72: Complex barycentric calculation should be documented.The
ComputeBarycentricVertsfunction performs sophisticated coordinate transformations. Consider adding inline comments to explain the mathematical operations, particularly the conditional logic and matrix operations.inline float4x3 ComputeBarycentricVerts(float2 landscapeUV) { + // Transform UV coordinates to skewed triangular grid space float2 scaledUV = landscapeUV * (WORLD_SCALE); float2 skewUV = mul(SKEW_MATRIX, scaledUV); float2 vxID = floor(skewUV); float2 frac_uv = frac(skewUV); float barry_z = 1.0 - frac_uv.x - frac_uv.y; float3 barry = float3(frac_uv, barry_z); + // Select triangle vertices based on which half of the parallelogram we're in return (barry.z > 0) ? float4x3(float3(vxID, 0), float3(vxID + float2(0, 1), 0), float3(vxID + float2(1, 0), 0), barry.zyx) : float4x3(float3(vxID + float2(1, 1), 0), float3(vxID + float2(1, 0), 0), float3(vxID + float2(0, 1), 0), float3(-barry.z, 1.0 - barry.y, 1.0 - barry.x)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (5)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
11-28: Good use of named constants for maintainability.The introduction of static constants for magic numbers significantly improves code maintainability and eliminates the hardcoded values that were problematic in previous versions.
87-107: Verify LOD scaling calculation accuracy.The LOD_SCALE constant (255437.0) is derived from a complex calculation. Consider adding a comment with the derivation to ensure the scaling is correct and maintainable.
inline StochasticOffsets ComputeStochasticOffsetsLOD(float2 landscapeUV) { - // Precomputed scaling: (WORLD_SCALE / 0.010416667) * 8.0 = ~255437 + // Precomputed scaling: (332.54 / 0.010416667) * 8.0 = 255437.0 + // Where 0.010416667 = 1/96 (LOD cell size) and 8.0 is the scaling factor static const float LOD_SCALE = 255437.0;
184-191: Excellent runtime optimization with early exit.The branch for checking if terrain variation is enabled provides a good performance optimization by falling back to regular sampling when the feature is disabled.
158-173: Height blending logic is well-implemented.The luminance-respecting blend operator with alpha mask consideration is sophisticated and should provide better visual quality. The separation of luminance and alpha height calculations is particularly well done.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (1)
53-57: Division by zero protection still needed.This is the same issue flagged in previous reviews. The weight normalization uses
rcp()without checking if the sum is zero, which could cause issues with degenerate cases.inline float3 NormalizeWeights(float3 weights) { - float rcpWeightSum = rcp(weights.x + weights.y + weights.z); + float weightSum = weights.x + weights.y + weights.z; + float rcpWeightSum = rcp(max(weightSum, 1e-6)); return weights * rcpWeightSum; }
🧹 Nitpick comments (2)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (2)
59-72: Consider adding input validation for barycentric computation.The function performs complex mathematical operations on
landscapeUVwithout bounds checking. Consider adding validation to prevent potential overflow or underflow issues.inline float4x3 ComputeBarycentricVerts(float2 landscapeUV) { + // Clamp input to reasonable range to prevent overflow + landscapeUV = clamp(landscapeUV, -1000.0, 1000.0); float2 scaledUV = landscapeUV * (WORLD_SCALE); // ... rest of function }
112-132: Optimize LOD sampling with early mip level check.The LOD sampling function could benefit from an early exit similar to the main stochastic functions, especially since it only uses 2 samples.
inline float4 StochasticSampleLOD(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsetsLOD, float2 dx, float2 dy) { + // Early exit for very high mip levels + if (mipLevel >= 6.0) + { + return tex.SampleLevel(samp, uv, mipLevel + SharedData::MipBias); + } + // Apply mip bias to match normal sampling behavior float adjustedMipLevel = mipLevel + SharedData::MipBias;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)src/Features/TerrainVariation.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/TerrainVariation.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (5)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#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.
🔇 Additional comments (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
11-28: Excellent use of named constants for maintainability.The comprehensive set of static constants effectively addresses previous feedback about magic numbers and makes the code much more maintainable. The organization and documentation are clear.
184-219: Excellent performance optimization with runtime branch.The
StochasticEffectNoHeightfunction includes a smart runtime check to fall back to regular sampling when terrain variation is disabled. This is a good performance optimization that avoids unnecessary computation.
168-170: Good use of alpha masking for height calculation.The logic that uses alpha values as height when available (through the step function mask) is well-implemented and provides flexibility for different texture types.
47-51: hashLOD return type is correctThe expression
p.x + p.y * float2(17.0, 23.0)promotes the scalarp.xto afloat2, sofrac(...)is invoked on afloat2and returns afloat2as declared. No changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (2)
40-45: Hash function uses inconsistent parameter naming.The function parameter is named
sbut would be more descriptive aspto match thehashLODfunction and maintain consistency across hash functions.-inline float2 hash2D2D(float2 s) +inline float2 hash2D2D(float2 p) { - s = s * HASH_MULTIPLIER; - return frac(sin(s.x + s.y) * HASH_SINE_MULTIPLIER); + p = p * HASH_MULTIPLIER; + return frac(sin(p.x + p.y) * HASH_SINE_MULTIPLIER); }
152-153: Consider optimizing hash computation based on early exit potential.The comment mentions "Wasted ALU since there are still 3 hash calls computed for the pixel" which suggests the offsets are pre-computed regardless of the early exit. Consider restructuring to compute offsets only when needed.
// Main stochastic sampling function -inline float4 StochasticEffect(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsets, float2 dx, float2 dy) +inline float4 StochasticEffect(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, float2 landscapeUV, float2 dx, float2 dy) { // Apply mip bias to match normal sampling behavior float adjustedMipLevel = mipLevel + SharedData::MipBias; + // Calculate smooth transition factor first + float mipFactor = saturate((mipLevel - MIP_BLEND_START) / MIP_BLEND_RANGE); + + // Compute offsets only if we might need multiple samples + StochasticOffsets offsets = (mipFactor < MIP_EARLY_EXIT_THRESHOLD) ? + ComputeStochasticOffsets(landscapeUV) : + (StochasticOffsets)0; + // Take first sample (always needed) float4 sample1 = tex.SampleLevel(samp, uv + offsets.offset1, adjustedMipLevel);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (5)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms (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 (8)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (8)
11-28: Well-organized constant definitions with performance considerations.The constants are logically grouped and include helpful comments. The reduction of
HEIGHT_BLEND_CONTRASTfrom 16.0 to 12.0 for performance is a good optimization choice. The pre-computed matrices and scaling factors will avoid runtime calculations.
47-51: Verify hashLOD function correctness after type fix.The function now correctly returns a float2, addressing the previous critical bug. However, the calculation
p.x + p.y * float2(17.0, 23.0)producesfloat2(p.x + p.y * 17.0, p.x + p.y * 23.0), which may not provide optimal hash distribution since both components share the same base termp.x.Consider using a more independent calculation for better hash distribution:
inline float2 hashLOD(float2 p) { p = frac(p * 0.318); - return frac(p.x + p.y * float2(17.0, 23.0)); + return frac(p * float2(17.0, 23.0) + p.yx * float2(31.0, 37.0)); }
60-73: Excellent modularization of barycentric coordinate calculation.The function is well-structured with clear variable names and efficient branching logic. The use of pre-computed
SKEW_MATRIXandWORLD_SCALEconstants optimizes performance.
75-86: Clean separation of standard stochastic offset computation.The function properly delegates to the modular
ComputeBarycentricVertsfunction and maintains clear data flow through theStochasticOffsetsstruct.
88-108: Efficient LOD-specific offset computation with reduced sampling.The LOD variant uses only 2 samples instead of 3, which is a good performance optimization for distant terrain. The pre-computed
LOD_SCALEconstant and simplified weight distribution (0.65, 0.35, 0.0) are well-designed choices.
113-133: Well-optimized LOD sampling with performance considerations.The function uses efficient pseudo-rotation with simple transforms and limits to 2 samples for performance. The mip bias application is consistent with the main sampling function.
136-183: Comprehensive refactoring with improved mip level handling.The function now uses explicit mip levels with
SampleLevelinstead of gradients, which aligns with the PR objective of fixing mipmap issues. The early exit optimization and smooth transitions between sampling modes are well-implemented.
185-221: Performance-optimized variant provides good fallback path.The
StochasticEffectNoHeightfunction offers a simplified sampling path without height blending, which is valuable for performance-critical scenarios. The runtime check forenableTilingFixensures proper fallback behavior.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (1)
149-157: Consider optimizing hash calculation placement for better performance.The comment on line 156-157 acknowledges that hash calculations are still performed even when early exit occurs at high mip levels. While the current approach avoids recalculation during camera movement, consider whether the hash computations could be moved after the early exit check for better performance at distance.
inline float4 StochasticEffect(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsets, float2 dx, float2 dy) { // Apply mip bias to match normal sampling behavior float adjustedMipLevel = mipLevel + SharedData::MipBias; + + // Calculate smooth transition factor - starts at mip 5.0, early exit at mip 6.5 + float mipFactor = saturate((mipLevel - MIP_BLEND_START) / MIP_BLEND_RANGE); + + // Early exit for very high mip levels - single sample is sufficient + if (mipFactor >= MIP_EARLY_EXIT_THRESHOLD) + { + return tex.SampleLevel(samp, uv + offsets.offset1, adjustedMipLevel); + } // Take first sample (always needed) float4 sample1 = tex.SampleLevel(samp, uv + offsets.offset1, adjustedMipLevel); - - // Calculate smooth transition factor - starts at mip 5.0, early exit at mip 6.5 - float mipFactor = saturate((mipLevel - MIP_BLEND_START) / MIP_BLEND_RANGE); - - // Early exit for very high mip levels - single sample is sufficient. Miplevels hide the artifacts. - if (mipFactor >= MIP_EARLY_EXIT_THRESHOLD) - { - return sample1; - // Wasted ALU since there are still 3 hash calls computed for the pixel, but doesn't really hurt performance and also avoids recalculating when moving closer/further away. - }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (5)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms (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 (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
11-28: Excellent implementation of named constants to replace magic numbers.The introduction of well-documented static constants significantly improves code maintainability and addresses previous review feedback. The organization into logical groups (height blend, world scale, blending, hash, and mip level constants) is clear and professional.
52-56: Critical bug fix: hashLOD function now correctly returns float2.The function signature and implementation are now consistent - the return statement properly returns a
float2value usingfloat2(17.0, 23.0)instead of the previous incorrect singlefloatreturn.
58-63: Division by zero protection successfully implemented.The
max(weightSum, 1e-6)safeguard prevents division by zero errors in thercp()function call, addressing the critical safety concern from previous reviews.
190-224: Well-implemented performance-optimized variant without height blending.The
StochasticEffectNoHeightfunction provides an excellent performance alternative for scenarios where height-based blending isn't required. The early exit for disabled terrain variation and simplified barycentric blending are well-executed optimizations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (2)
12-14: Reconsider the "DO NOT CHANGE" restrictions on constants.The comments stating "DO NOT CHANGE THESE VALUES" reduce maintainability and flexibility. Consider making these tunable parameters or at least documenting why they're fixed (e.g., if they're derived from mathematical constraints or empirical optimization).
Also applies to: 24-27
156-157: Consider optimizing hash calculation placement for better performance.The comment acknowledges wasted ALU cycles from precomputing all hash offsets even when early exit occurs. Consider restructuring to compute offsets only when needed, or accept the tradeoff for code simplicity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (5)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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: alandtse
PR: doodlum/skyrim-community-shaders#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: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms (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/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (3)
164-179: Excellent height blending implementation.The luminance-based height calculation with alpha channel fallback is well-designed. The weight combination using both barycentric coordinates and height influence provides good visual results while maintaining mathematical correctness.
199-203: Excellent early exit optimization for disabled terrain variation.This early exit when
enableTilingFixis false provides significant performance benefits by avoiding all stochastic calculations when the feature is disabled. Well-designed optimization.
52-56: Review hashLOD distribution and robustnessI didn’t find any other 2D-to-2D hash variants in the repo to compare against, so it’s hard to gauge whether the simple linear combination in hashLOD will give you a sufficiently uncorrelated, uniform distribution for your mip-level offsets. Please verify its statistical properties or consider swapping in a more battle-tested 2D hash (for example, using a sine-based mix or extra prime multipliers as in hash2D2D).
• File: features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
Lines 52–56inline float2 hashLOD(float2 p) { p = frac(p * 0.318); return frac(p.x + p.y * float2(17.0, 23.0)); }• Potential concerns:
- Linear mapping may produce repeating patterns along grid axes.
- Lacks non-linear mixing (e.g. sin, additional fract/splitmix steps) found in other hash variants.
- Could lead to visible artifacts at certain lod transitions.
Action items:
- Manually test the output distribution (e.g. generate a scatter plot of hashLOD over a grid).
- If artifacts appear, replace with a more robust 2D hash (e.g. sin-based or splitmix mix as in hash2D2D).
| #pragma warning(push) | ||
| #pragma warning(disable : 4000) | ||
| inline float4 StochasticEffectNoHeight(float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsets) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider restructuring code instead of disabling warnings.
Suppressing compiler warnings can hide real issues. Instead of disabling the X4000 warning, consider restructuring the function to ensure all variables are properly initialized before use.
🤖 Prompt for AI Agents
In features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
around lines 192 to 194, the code disables warning 4000 instead of addressing
the root cause. Review the StochasticEffectNoHeight function to ensure all
variables are properly initialized before use, and restructure the code to
eliminate the warning without suppressing it. Remove the pragma that disables
the warning once the function is corrected.
There was a problem hiding this comment.
@coderabbitai Do a full inspection of StochasticEffectNoHeight and every location it is used, and fully explore if there are any uninitialised variables in any compiler case leftover.
There was a problem hiding this comment.
🧩 Analysis chain
I'll conduct a thorough analysis of the StochasticEffectNoHeight function and all its usage locations to identify any potential uninitialized variable issues.
🏁 Script executed:
#!/bin/bash
# Search for all usages of StochasticEffectNoHeight in the codebase
echo "=== Finding all usages of StochasticEffectNoHeight ==="
rg "StochasticEffectNoHeight" -A 3 -B 3
echo -e "\n=== Finding function declarations and definitions ==="
ast-grep --pattern 'StochasticEffectNoHeight($$$)'
echo -e "\n=== Checking for any other pragma warning directives in HLSL files ==="
rg "#pragma warning" --type hlsl -A 2 -B 2
echo -e "\n=== Looking for potential variable declarations in the function ==="
rg -A 50 "inline float4 StochasticEffectNoHeight" features/
Length of output: 27818
Let me examine the specific function implementation and analyze each code path:
🏁 Script executed:
#!/bin/bash
# Get the complete function implementation with better context
echo "=== Complete StochasticEffectNoHeight function implementation ==="
rg -A 35 "#pragma warning.*disable.*4000" features/
echo -e "\n=== Check for any other pragma warning usage patterns in shader files ==="
find . -name "*.hlsl" -o -name "*.hlsli" | xargs grep -l "#pragma warning" | head -5
echo -e "\n=== Analyze the constants used in the function ==="
rg "MIP_BLEND_START|MIP_BLEND_RANGE|MIP_EARLY_EXIT_THRESHOLD" features/
echo -e "\n=== Check the NormalizeWeights function implementation ==="
rg -A 10 "inline float3 NormalizeWeights" features/
Length of output: 12963
Now let me check if there are any similar warning patterns in other HLSL files:
🏁 Script executed:
#!/bin/bash
# Search for pragma warning patterns more broadly
echo "=== Search for pragma warning patterns in all shader files ==="
fd -e hlsl -e hlsli . | xargs grep -l "#pragma warning" 2>/dev/null || echo "No other pragma warning usage found"
echo -e "\n=== Verify all parameters and struct members used in the function ==="
echo "Function parameters:"
echo "- mipLevel: float (function parameter)"
echo "- tex: Texture2D (function parameter)"
echo "- samp: SamplerState (function parameter)"
echo "- uv: float2 (function parameter)"
echo "- offsets: StochasticOffsets (function parameter)"
echo -e "\nStochasticOffsets struct members:"
rg -A 10 "struct StochasticOffsets" features/
echo -e "\n=== Check SharedData dependencies ==="
rg "SharedData::" features/Terrain\ Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
Length of output: 2775
Based on my comprehensive analysis of the StochasticEffectNoHeight function and all its usage locations, I can confirm that there are no actual uninitialized variables - this is a false positive from the FXC compiler.
Detailed Analysis
Function Execution Paths
I traced through all three possible execution paths:
Path 1: Early exit (terrain variation disabled)
float adjustedMipLevel = mipLevel + SharedData::MipBias; // ✓ initialized
if (!SharedData::terrainVariationSettings.enableTilingFix)
return tex.SampleLevel(samp, uv, adjustedMipLevel); // ✓ all variables initializedPath 2: Early exit (high mip levels)
float4 sample1 = tex.SampleLevel(samp, uv + offsets.offset1, adjustedMipLevel); // ✓ initialized
float mipFactor = saturate((mipLevel - MIP_BLEND_START) / MIP_BLEND_RANGE); // ✓ initialized
[branch] if (mipFactor >= MIP_EARLY_EXIT_THRESHOLD)
return sample1; // ✓ initializedPath 3: Full blending
float4 sample2 = tex.SampleLevel(samp, uv + offsets.offset2, adjustedMipLevel); // ✓ initialized
float4 sample3 = tex.SampleLevel(samp, uv + offsets.offset3, adjustedMipLevel); // ✓ initialized
float3 weights = NormalizeWeights(saturate(offsets.weights)); // ✓ initialized
float4 blendedSample = sample1 * weights.x + sample2 * weights.y + sample3 * weights.z; // ✓ initialized
float smoothFactor = smoothstep(0.0, 1.0, mipFactor); // ✓ initialized
return lerp(blendedSample, sample1, smoothFactor); // ✓ all initializedKey Findings
- All variables are properly initialized before use in every execution path
- Function parameters are guaranteed to be passed in correctly (verified across 22 usage sites)
- Static constants are compile-time defined (
MIP_BLEND_START = 5.0,MIP_BLEND_RANGE = 1.5,MIP_EARLY_EXIT_THRESHOLD = 1.0) - Struct members are well-defined (
StochasticOffsetshasoffset1,offset2,offset3,weights) - External dependencies (
SharedData::MipBias,SharedData::terrainVariationSettings.enableTilingFix) are properly declared elsewhere
Why FXC Reports False Positives
The FXC compiler's static analysis is confused by:
- Complex control flow with multiple early return paths
- Conditional variable declarations (
sample2,sample3only declared when needed) [branch]attributes affecting control flow analysis- Conditional texture sampling operations
Conclusion
The #pragma warning(disable : 4000) is the correct approach in this case. This is a well-known limitation of FXC's static analysis for complex shader functions. The warning suppression is justified and safe since all variables are demonstrably initialized before use.
Recommendation: Keep the pragma directive as-is. Attempting to restructure this function to avoid the false positive would likely make the code less readable and potentially less efficient without providing any actual benefit.
@davo0411 The function is correctly implemented with no actual uninitialized variables. The FXC compiler warning is a false positive due to the complex control flow, and the pragma directive is the appropriate solution.
🧠 Learnings used
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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: alandtse
PR: doodlum/skyrim-community-shaders#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: davo0411
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
There was a problem hiding this comment.
🧩 Analysis chain
@davo0411 I'll conduct a thorough inspection of the StochasticEffectNoHeight function and its usage throughout the codebase to identify any potential uninitialized variable issues.
🏁 Script executed:
#!/bin/bash
# Search for all usages of StochasticEffectNoHeight function
echo "=== Searching for StochasticEffectNoHeight function calls ==="
rg "StochasticEffectNoHeight" -A 3 -B 3
echo -e "\n=== Searching for function declarations and definitions ==="
ast-grep --pattern 'StochasticEffectNoHeight($$$)'
echo -e "\n=== Checking for any includes of TerrainVariation.hlsli ==="
rg "#include.*TerrainVariation\.hlsli" -A 1 -B 1
Length of output: 23111
Now let me examine the function implementation more closely and check for any potential uninitialized variable paths:
🏁 Script executed:
#!/bin/bash
# Get the complete StochasticEffectNoHeight function implementation
echo "=== Complete StochasticEffectNoHeight function implementation ==="
rg -A 50 "inline float4 StochasticEffectNoHeight" "features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli"
echo -e "\n=== Check for any constants used in the function ==="
rg "MIP_BLEND_START|MIP_BLEND_RANGE|MIP_EARLY_EXIT_THRESHOLD" "features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli"
echo -e "\n=== Check how sharedOffset is calculated in calling contexts ==="
rg -B 5 -A 5 "sharedOffset.*=" "features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli"
Length of output: 2936
Let me search for more context about how sharedOffset is calculated and check if there are any other potential uninitialized variable scenarios:
🏁 Script executed:
#!/bin/bash
# Check how sharedOffset is defined and used in Extended Materials
echo "=== Finding sharedOffset definition and usage ==="
rg -B 10 -A 3 "sharedOffset" "features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli" | head -30
echo -e "\n=== Check for ComputeStochasticOffsets calls in Extended Materials ==="
rg -B 3 -A 3 "ComputeStochasticOffsets" "features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli"
echo -e "\n=== Check the NormalizeWeights function implementation ==="
rg -A 10 "inline float3 NormalizeWeights" "features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli"
echo -e "\n=== Check if there are any preprocessor conditionals around the function ==="
rg -B 5 -A 5 "#if.*TERRAIN_VARIATION|#ifdef.*TERRAIN_VARIATION" "features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli"
Length of output: 3120
Based on my thorough analysis of the StochasticEffectNoHeight function and all its usage locations, I can provide a comprehensive assessment:
Analysis Results: The Warning is a False Positive
After examining the function's control flow and all call sites, there are no actual uninitialized variables. Here's my detailed analysis:
Function Control Flow Analysis
The StochasticEffectNoHeight function has three possible execution paths:
-
Early exit when terrain variation disabled:
if (!SharedData::terrainVariationSettings.enableTilingFix) return tex.SampleLevel(samp, uv, adjustedMipLevel);
- All variables used are properly initialized
-
Early exit for high mip levels:
float4 sample1 = tex.SampleLevel(...); // Initialized float mipFactor = saturate(...); // Initialized if (mipFactor >= MIP_EARLY_EXIT_THRESHOLD) return sample1; // Uses initialized sample1
-
Full blending path:
float4 sample2 = tex.SampleLevel(...); // Initialized float4 sample3 = tex.SampleLevel(...); // Initialized float3 weights = NormalizeWeights(...); // Initialized float4 blendedSample = sample1 * ...; // Uses initialized variables
Usage Context Analysis
The function is called in 26 locations across:
- Extended Materials (18 calls) - for displacement height sampling
- Lighting.hlsl (6 calls) - for RMAOS texture sampling
All call sites provide properly initialized parameters, and the sharedOffset parameter is a valid StochasticOffsets struct.
Why the Compiler Warning Occurs
The X4000 warning is a false positive caused by:
- Complex control flow with multiple early returns
- Branch attributes
[branch]confusing static analysis - FXC compiler limitations in analyzing conditional paths
Recommendation
The #pragma warning(disable : 4000) is justified in this case. However, if you prefer to eliminate the pragma, here's a restructured version:
inline float4 StochasticEffectNoHeight(float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsets)
{
float adjustedMipLevel = mipLevel + SharedData::MipBias;
// Early exit for disabled terrain variation
if (!SharedData::terrainVariationSettings.enableTilingFix)
{
return tex.SampleLevel(samp, uv, adjustedMipLevel);
}
// Always take first sample
float4 sample1 = tex.SampleLevel(samp, uv + offsets.offset1, adjustedMipLevel);
// Calculate transition factor
float mipFactor = saturate((mipLevel - MIP_BLEND_START) / MIP_BLEND_RANGE);
// Initialize remaining samples (compiler will optimize away if unused)
float4 sample2 = float4(0, 0, 0, 0);
float4 sample3 = float4(0, 0, 0, 0);
float4 blendedSample = sample1;
// Only compute additional samples if needed
if (mipFactor < MIP_EARLY_EXIT_THRESHOLD)
{
sample2 = tex.SampleLevel(samp, uv + offsets.offset2, adjustedMipLevel);
sample3 = tex.SampleLevel(samp, uv + offsets.offset3, adjustedMipLevel);
float3 weights = NormalizeWeights(saturate(offsets.weights));
blendedSample = sample1 * weights.x + sample2 * weights.y + sample3 * weights.z;
}
float smoothFactor = smoothstep(0.0, 1.0, mipFactor);
return lerp(blendedSample, sample1, smoothFactor);
}Conclusion: The current implementation with #pragma warning(disable : 4000) is correct and safe. The warning is a false positive, and there are no actual uninitialized variable issues in any compiler case.
🧠 Learnings used
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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: alandtse
PR: doodlum/skyrim-community-shaders#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: davo0411
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
|
✅ A pre-release build is available for this PR: |
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes