chore(terrain shadows): modernize#1678
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis change fixes terrain shadow issues in Markarth by correcting heightmap context selection and introducing conditional shadow rendering modes. Updates include version bumping, adding sharper shadow computation via DEFERRED conditional compilation, traversing world hierarchy for proper heightmap lookup, and defining DEFERRED in shader compilation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
doodlum
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
features/Terrain Shadows - Heightmaps/textures/HeightMaps/Blackreach.Terrain.HeightMap.-7.-8.7.12.-284.861.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/DLC01FalmerValley.HeightMap.-16.-13.10.13.-32768.32768.-530.2212.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/DLC01FalmerValley.Terrain.HeightMap.-16.-13.10.13.-530.2212.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/DLC01SoulCairn.Terrain.HeightMap.-52.-43.30.44.-188.245.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/DLC1HunterHQWorld.Terrain.HeightMap.-31.-8.3.5.-662.308.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/DLC2ApocryphaWorld.Terrain.HeightMap.-6.-1.7.9.-250.-206.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/DLC2SolstheimWorld.HeightMap.-64.-64.127.127.-32768.32768.-1024.6342.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/DLC2SolstheimWorld.Terrain.HeightMap.-64.-64.127.127.-1024.6342.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/DeepwoodRedoubtWorld.Terrain.HeightMap.-39.15.-27.24.-618.472.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/JaphetsFollyWorld.Terrain.HeightMap.-7.-6.6.6.-210.163.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/MarkarthWorld.Terrain.HeightMap.-48.-3.-31.5.-3375.2609.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/SkuldafnWorld.Terrain.HeightMap.40.-21.62.-1.-285.1876.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/Sovngarde.Terrain.HeightMap.-23.-32.34.40.-1331.4715.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/Tamriel.HeightMap.-57.-43.61.50.-32768.32768.-4629.4924.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/Tamriel.Terrain.HeightMap.-57.-43.61.50.-4629.4924.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/ccBGSSSE067DeadlandsWorld.Terrain.HeightMap.-6.-4.4.7.-140.353.ddsis excluded by!**/*.ddsfeatures/Terrain Shadows - Heightmaps/textures/HeightMaps/ccKRTSSE001QNWorld.Terrain.HeightMap.-3.-4.2.2.-972.265.ddsis excluded by!**/*.dds
📒 Files selected for processing (4)
features/Terrain Shadows/Shaders/Features/TerrainShadows.inifeatures/Terrain Shadows/Shaders/TerrainShadows/ShadowUpdate.cs.hlslfeatures/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlslisrc/Features/TerrainShadows.cpp
🧰 Additional context used
📓 Path-based instructions (4)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
features/Terrain Shadows/Shaders/Features/TerrainShadows.inifeatures/Terrain Shadows/Shaders/TerrainShadows/ShadowUpdate.cs.hlslsrc/Features/TerrainShadows.cppfeatures/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlsli
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
features/Terrain Shadows/Shaders/TerrainShadows/ShadowUpdate.cs.hlslsrc/Features/TerrainShadows.cppfeatures/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlsli
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Terrain Shadows/Shaders/TerrainShadows/ShadowUpdate.cs.hlslfeatures/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlsli
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/TerrainShadows.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
features/Terrain Shadows/Shaders/TerrainShadows/ShadowUpdate.cs.hlslfeatures/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlsli
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
features/Terrain Shadows/Shaders/TerrainShadows/ShadowUpdate.cs.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
features/Terrain Shadows/Shaders/TerrainShadows/ShadowUpdate.cs.hlsl
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
features/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlsli
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
features/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlsli
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
features/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlsli
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
features/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlsli
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Run Shader Unit Tests
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
features/Terrain Shadows/Shaders/Features/TerrainShadows.ini (1)
2-2: LGTM! Version bump aligns with the feature updates.The version increment from 1-0-0 to 1-0-1 appropriately reflects the thread group size increase and shadow quality improvements in this PR.
features/Terrain Shadows/Shaders/TerrainShadows/TerrainShadows.hlsli (1)
25-31: LGTM! Clean conditional for shadow quality modes.The DEFERRED conditional provides a sensible way to switch between sharp and blurry shadow transitions. The 10x multiplier in the sharp mode creates a tighter penumbra while the original formula maintains softer scattering simulation.
features/Terrain Shadows/Shaders/TerrainShadows/ShadowUpdate.cs.hlsl (1)
62-63: LGTM! Thread group size increase is well-justified.The 8x increase in thread group size (128→1024) improves parallelism for the shadow height reduction. Shared memory usage remains conservative at 8KB (1024 × sizeof(float2)), well under the typical 32KB limit.
src/Features/TerrainShadows.cpp (1)
200-204: Good addition: Parent world traversal for land data.The traversal logic correctly ascends through parent worlds when land data is inherited via
ParentUseFlag::kUseLandData, ensuring heightmaps are loaded from the appropriate source worldspace. The static TES singleton retrieval is a reasonable optimization.
davo0411
requested changes
Jan 4, 2026
Collaborator
davo0411
left a comment
davo0411
left a comment
There was a problem hiding this comment.
check ai comments, otherwise looks good. if ai is jibberish i am happy to merge
Collaborator
|
Please check the PR you asked me to do. #1676 |
alandtse
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Features/TerrainShadows.cpp (1)
200-212: Parent world traversal logic correctly addresses issue #1581.The loop at lines 203-204 properly traverses to the parent world when
kUseLandDatais set, ensuring terrain shadows use the correct heightmap in child worldspaces like Markarth.One minor observation: making
tesstatic (line 200) persists the singleton pointer across calls. WhileRE::TES::GetSingleton()is typically stable, if you want to be defensive against edge cases during game state transitions, consider removingstatic:🔎 Optional: Remove static for defensive coding
- static auto tes = RE::TES::GetSingleton(); + auto tes = RE::TES::GetSingleton(); + if (!tes) + return;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Shaders/RunGrass.hlslsrc/Features/TerrainShadows.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
package/Shaders/RunGrass.hlslsrc/Features/TerrainShadows.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/RunGrass.hlslsrc/Features/TerrainShadows.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/TerrainShadows.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
package/Shaders/RunGrass.hlslsrc/Features/TerrainShadows.cpp
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
package/Shaders/RunGrass.hlslsrc/Features/TerrainShadows.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/RunGrass.hlsl
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/Features/TerrainShadows.cpp
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
src/Features/TerrainShadows.cpp
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
src/Features/TerrainShadows.cpp
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
src/Features/TerrainShadows.cpp
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
src/Features/TerrainShadows.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Run Shader Unit Tests
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (2)
package/Shaders/RunGrass.hlsl (1)
10-11: LGTM! Enables sharper terrain shadows for grass via the deferred path.The
DEFERREDdefine is correctly placed before theTerrainShadows.hlsliinclude (line 438), enabling the sharper shadow fraction computation branch. This aligns with the PR objective of producing sharper shadows for opaque objects.src/Features/TerrainShadows.cpp (1)
315-317: LGTM! The log2 calculation is now correctly derived fromupdateLength.The previous issue with the hardcoded
128uhas been fixed.std::bit_width(1024) - 1 = 10correctly computes log₂(1024).The reminder comment on line 315 is helpful for keeping
NTHREADSin the shader synchronized withupdateLength.
|
✅ A pre-release build is available for this PR: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/TerrainShadows.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/TerrainShadows.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/TerrainShadows.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/TerrainShadows.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
src/Features/TerrainShadows.cpp
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/Features/TerrainShadows.cpp
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
src/Features/TerrainShadows.cpp
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
src/Features/TerrainShadows.cpp
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
src/Features/TerrainShadows.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Run Shader Unit Tests
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
alandtse
approved these changes
Jan 7, 2026
davo0411
approved these changes
Jan 11, 2026
davo0411
added a commit
to davo0411/skyrim-community-shaders
that referenced
this pull request
Jan 11, 2026
* build: bump eastl to 3.27.01 (community-shaders#1673) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(grass collision): ignore hkpListShape (community-shaders#1661) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore(upscaling): update SDKs (community-shaders#1684) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore(terrain shadows): modernize (community-shaders#1678) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat: add user wiki link to readme (community-shaders#1689) * feat(LLF): add debug overlay in llf visualiser (community-shaders#1666) * feat: linear lighting (community-shaders#1359) Co-authored-by: jiayev <jiayev@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat: weather and imagespace editor (community-shaders#1630) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(ui): load previously selected theme on startup (community-shaders#1664) --------- Co-authored-by: Alan Tse <alandtse@users.noreply.github.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: doodlum <15017472+doodlum@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: jiayev <l936249247@hotmail.com> Co-authored-by: jiayev <jiayev@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Apr 20, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Updates
✏️ Tip: You can customize this high-level summary in your review settings.