fix(grass): use correct light direction#1602
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Shader directional light data source package/Shaders/RunGrass.hlsl |
Replaced the first argument of GrassLighting::GetLightSpecularInput in the pixel shader from DirLightDirection to SharedData::DirLightDirection.xyz; other parameters unchanged. |
Estimated code review effort
🎯 1 (Trivial) | ⏱️ ~2 minutes
Suggested reviewers
- doodlum
- Pentalimbed
Poem
🐰 In blades of grass the sunlight plays,
A shared direction lights the days.
One small swap, the shimmer’s right,
Specular hops into the light. 🌿✨
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'fix(grass): use correct light direction' directly matches the main change: switching from DirLightDirection to SharedData::DirLightDirection.xyz in the grass shader's specular lighting calculation. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 @coderabbitai help to get the list of available commands and usage tips.
|
its already world space |
f727bd8 to
9fede19
Compare
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)
package/Shaders/RunGrass.hlsl(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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.hlsl
🧠 Learnings (2)
📓 Common learnings
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/RunGrass.hlsl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
|
|
||
| if (complex) | ||
| lightsSpecularColor += GrassLighting::GetLightSpecularInput(DirLightDirection, viewDirection, normal, dirLightColor, SharedData::grassLightingSettings.Glossiness); | ||
| lightsSpecularColor += GrassLighting::GetLightSpecularInput(SharedData::DirLightDirection.xyz, viewDirection, normal, dirLightColor, SharedData::grassLightingSettings.Glossiness); |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Change aligns with existing usage patterns.
The change to use SharedData::DirLightDirection.xyz is consistent with line 584, which already uses SharedData::DirLightDirection.xyz for the directional light angle calculation in the same shader path.
However, please verify whether line 617 (in the TRUE_PBR path) should also be updated for consistency:
Line 617 passes DirLightDirection (from the constant buffer) to PBR::GetDirectLightInput. If SharedData::DirLightDirection is the canonical source, that call may need the same update.
🏁 Script executed:
#!/bin/bash
# Description: Check usage patterns of DirLightDirection vs SharedData::DirLightDirection in shader files
# Search for all DirLightDirection usages in HLSL files to identify inconsistent patterns
rg -n -C3 --type=hlsl 'DirLightDirection' -g '!RunGrass.hlsl'
# Also check in RunGrass.hlsl specifically
rg -n 'DirLightDirection' package/Shaders/RunGrass.hlslLength of output: 715
Update line 617 for consistency with GRASS_LIGHTING data sources.
The change at line 645 correctly aligns with line 584's usage of SharedData::DirLightDirection.xyz within the GRASS_LIGHTING path. However, line 617 in the TRUE_PBR sub-path still uses the constant buffer DirLightDirection directly.
For consistency within the GRASS_LIGHTING block, line 617 should also be updated to use SharedData::DirLightDirection:
PBR::GetDirectLightInput(dirDiffuseColor, coatDirDiffuseColor, dirTransmissionColor, dirSpecularColor, normal, normal, viewDirection, viewDirection, SharedData::DirLightDirection, SharedData::DirLightDirection, lightProperties, pbrSurfaceProperties, tbn, input.TexCoord.xy);🤖 Prompt for AI Agents
In package/Shaders/RunGrass.hlsl around line 617, the TRUE_PBR path still passes
the standalone constant buffer DirLightDirection to PBR::GetDirectLightInput
while the GRASS_LIGHTING path uses SharedData::DirLightDirection; update the
PBR::GetDirectLightInput call to use SharedData::DirLightDirection for both
directional-light parameters (replacing DirLightDirection with
SharedData::DirLightDirection) so it matches the GRASS_LIGHTING data source and
remains consistent with the change at line 645.
|
✅ A pre-release build is available for this PR: |
* chore(ui): update discord banner (community-shaders#1493) * fix: use proper filename settingsuser.json (community-shaders#1491) * chore(upscaling): increase fsr sharpness * chore: rename d3d12interop to d3d12SwapChainActive (community-shaders#1494) * feat(llf): remove particle lights (community-shaders#1495) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat(llf): move llf to core (community-shaders#1496) * fix: remove water clamp (community-shaders#1497) * fix(upscaling): more upscaling fixes (community-shaders#1498) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix: fix some internal errors when debugging (community-shaders#1500) * fix(ui): fix save settings conflicts & welcome screen (community-shaders#1501) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(ui): add constraints for discord banner size (community-shaders#1463) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: doodlum <15017472+doodlum@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(VR): fix exiting menu using controllers (community-shaders#1502) * build: fix warnings (community-shaders#1505) * feat(UI): allow tooltips for disabled elements (community-shaders#1503) * feat(upscaling): add downscale percentages (community-shaders#1506) * perf(ssgi): optimize (community-shaders#1499) 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> * feat(ui): font size and perf overlay improvements (community-shaders#1511) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore: remove unused hooks (community-shaders#1510) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix: adjust IsInterior to consider kNoSky or kFixedDimensions flags (community-shaders#1512) * fix(hair): correct hair indirect normal, marschner by default (community-shaders#1515) Co-authored-by: doodlum <15017472+doodlum@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: mostly revert ISHDR to 1.3.6 (community-shaders#1516) * chore(upscaling): simplify interop and upscale methods (community-shaders#1514) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(hair): typo in code (community-shaders#1517) * feat(ibl): lerp sky ibl using skylighting (community-shaders#1519) * fix(sss): burley artifacts with effect blend (community-shaders#1518) * fix(upscaling): fix screenshots when upscaling enabled (community-shaders#1520) * fix(upscaling): fix mipbias sometimes being wrong (community-shaders#1521) * fix: fix compile error if snow shader on (community-shaders#1522) * chore(upscaling): revert fsr to typical settings (community-shaders#1523) * fix: fix minor ui issues (community-shaders#1524) * chore(grass collision): simpler grass collision (community-shaders#1525) * fix: update skylighting and version * fix(pbr): fix inconsistencies (community-shaders#1526) 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> Co-authored-by: jiayev <l936249247@hotmail.com> * feat(upscaling): sharpening slider (community-shaders#1527) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore: bump versions * fix(ibl): add ibl to reflection normalization (community-shaders#1528) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(hair): remove pbr lighting mult for hair (community-shaders#1531) * chore(upscaling): add back upscale multiplier (community-shaders#1532) * fix(upscaling): fix minor upscaling issues (community-shaders#1536) * chore: gamma space normalisation (community-shaders#1535) 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> * feat(grass collision): implement with texture and history (community-shaders#1539) 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> * chore(grass collision): less aggressive (community-shaders#1546) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(skylighting): fix cell id casting (community-shaders#1544) * chore(emat): auto detect terrain parallax (community-shaders#1545) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore: update versions * feat(VR): enable upscaling (community-shaders#1507) * fix(terrain shadows): fix brightened lods (community-shaders#1547) * chore(upscaling): reduce ghosting near camera (community-shaders#1548) * fix: fix grass not animating (community-shaders#1549) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(grass collision): fix non-standard timescales (community-shaders#1550) * build: deploy only updated files (community-shaders#1556) * feat: add Clear Shader Cache to Advanced (community-shaders#1555) * chore(featureissues): default collapse testing menu (community-shaders#1554) * fix(VR): use only supported shaders from cache (community-shaders#1553) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * build: use gersemi cmake formatter (community-shaders#1557) * fix(terrain): vanilla diffuse in pbr terrain cell too bright due to wrong color space (community-shaders#1558) * docs: add new feature development template guide (community-shaders#1529) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * docs(UI): remove duplicate GPL license statement (community-shaders#1561) * feat: add renderdoc for debugging (community-shaders#1560) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@gmail.com> * fix(ui): welcome popup size issues (community-shaders#1573) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore(grass collision): minor tweaks (community-shaders#1568) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(terrain helper): fix conflicting bit (community-shaders#1566) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * feat(UI): separate theme settings, UI refactor, font support (community-shaders#1571) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore: bump versions * build: fix zipping aio (community-shaders#1579) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(grass collision): clamp maximum depth of grass (community-shaders#1578) * feat(UI): enhance shader blocking (community-shaders#1564) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@gmail.com> * fix: remove duplicate buffer setup (community-shaders#1586) * feat: update shader compile elapsed time every second (community-shaders#1587) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * build: add cmake install commands (community-shaders#1372) * feat(perf-overlay): add size controls (community-shaders#1591) * fix(perf-overlay): fix infinite draw calls table height (community-shaders#1590) * refactor(perf-overlay): remove collapsible headers (community-shaders#1572) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(perf-overlay): removed ImGuiTableFlags_ScrollX/Y for scroll bar issues (community-shaders#1594) * build: fix shader copying to relative paths (community-shaders#1603) * fix(ibl): apply ibl to cubemap normalisation for non deferred (community-shaders#1604) * fix(grass): use correct light direction (community-shaders#1602) * fix(welcome-popup): adjust font size & window spacing (community-shaders#1592) * feat(lod): add gamma sliders (community-shaders#1588) * build: correct CodeRabbit schema syntax (community-shaders#1608) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com> * build: add compile-time validation of GPU buffers (community-shaders#1427) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com> * ci: run shader validation on CMake and CI config changes (community-shaders#1606) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com> * feat: procedural sun * limb darkening * another darkening * build(deps): remove orphaned Intel XeSS dependency (community-shaders#1611) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix: accumulate sunlight color in pixel shader output * fix(ui): enter key now behaves properly when first time popup is open (community-shaders#1615) * feat(ui): add tabs to advanced settings & PBR search (community-shaders#1599) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * build: add HLSL intellisense (community-shaders#1614) * refactor(UI): move light limit visualization into debug (community-shaders#1619) * refactor(ui): add settings for shader block hotkeys (community-shaders#1624) Co-authored-by: Bruce <44987693+brucenguyen@users.noreply.github.com> * fix(ui): anchor reset settings button position (community-shaders#1621) Co-authored-by: Giovanni Correia <Gistix@users.noreply.github.com> * fix(hair): use indirect normal for deferred marschner hair (community-shaders#1626) * build: fix Package-AIO-Manual for fresh pulls (community-shaders#1625) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(snow): use world space vectors (community-shaders#1618) * feat(UI): add gaussian blur shader core files (community-shaders#1595) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat(ui): add test conditions button (community-shaders#1637) * fix(ui): blocked shader info overflow in Shader Debug tab (community-shaders#1632) * fix(upscaling): replace NIS with RCAS for DLSS (community-shaders#1620) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(dynamic cubemaps): add a check for timeskip (community-shaders#1639) * refactor: restructure lighting (community-shaders#1633) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat(ui): add themes & fonts (community-shaders#1596) 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> * feat(water): add flowmap parallax (community-shaders#1636) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix cloud shadow setting saving --------- Co-authored-by: zxcvbn <66063766+zndxcvbn@users.noreply.github.com> Co-authored-by: davo0411 <davidkehoe0411@outlook.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: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@users.noreply.github.com> Co-authored-by: soda <130315225+soda3000@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: ThePagi <32794457+ThePagi@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@gmail.com> Co-authored-by: Yupeng Zhang <ArcEarth@outlook.com> Co-authored-by: kuplion <kuplion@hotmail.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: Giovanni Correia <Gistix@users.noreply.github.com> Co-authored-by: Bruce <44987693+brucenguyen@users.noreply.github.com> Co-authored-by: Midona <106106405+midona-rhel@users.noreply.github.com>
This pull request makes a minor update to the
RunGrass.hlslshader to ensure the correct directional light vector is used in specular lighting calculations. The change clarifies which value is passed to theGrassLighting::GetLightSpecularInputfunction, improving code correctness and maintainability.GrassLighting::GetLightSpecularInputto useSharedData::DirLightDirection.xyzinstead ofDirLightDirection, ensuring the correct directional light vector is provided for specular lighting.Summary by CodeRabbit