feat(grass lighting): grass sphere normal feature flag and fixes#1483
Conversation
WalkthroughIntroduces a new GrassSphereNormal bit in shader permutation/state, conditionally alters grass normal handling in RunGrass.hlsl, adds grass-related engine hooks to propagate lighting to the new flag, and removes prior Grass PBR permutation and hook logic from TruePBR.cpp. Minor casting/log changes and enum expansion included. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Game
participant Engine as Engine Hooks
participant GrassProp as BSGrassShaderProperty_ctor
participant GrassSetup as BSGrassShader_SetupGeometry
participant Shader as RunGrass.hlsl
Game->>Engine: Load/Init
Engine->>GrassProp: Intercept ctor
Note right of GrassProp: If linked Lighting has kEffectLighting,<br/>enable kEffectLighting on Grass property
GrassProp-->>Engine: Return property
Game->>GrassSetup: SetupGeometry(pass, renderFlags)
Note right of GrassSetup: Clear GrassSphereNormal<br/>Set GrassSphereNormal if property has kEffectLighting
GrassSetup-->>Shader: Draw call with ExtraShaderDescriptor
Shader->>Shader: If (ExtraShaderDescriptor & GrassSphereNormal) == 0
Note right of Shader: Apply backface flip and clamp/normalize z
Shader-->>Game: Shaded grass pixels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
doodlum
changed the title
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package/Shaders/RunGrass.hlsl (1)
503-510: Make flag test explicit and remove redundant clamp.HLSL prefers boolean conditions; compare to 0 to avoid warnings. Also, normal.z is clamped twice; keep just one.
Apply:
- if (!(Permutation::ExtraShaderDescriptor & Permutation::ExtraFlags::GrassSphereNormal)) { - if (!frontFace) - normal = -normal; - - normal.z = max(0.0, normal.z); - normal = normalize(float3(normal.xy, max(0, normal.z))); - } + if ((Permutation::ExtraShaderDescriptor & Permutation::ExtraFlags::GrassSphereNormal) == 0) { + if (!frontFace) + normal = -normal; + // Clamp Z once and renormalize + normal = normalize(float3(normal.xy, max(0.0, normal.z))); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package/Shaders/Common/Permutation.hlsli(1 hunks)package/Shaders/RunGrass.hlsl(2 hunks)src/Hooks.cpp(3 hunks)src/State.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
package/Shaders/Common/Permutation.hlslipackage/Shaders/RunGrass.hlslsrc/State.hsrc/Hooks.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/State.hsrc/Hooks.cpp
🧠 Learnings (1)
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/Common/Permutation.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). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (7)
package/Shaders/Common/Permutation.hlsli (1)
64-65: New permutation bit looks correct; confirm cross-language sync.Bit 1<<5 does not overlap existing ExtraFlags. Please confirm this stays in sync with src/State.h::ExtraShaderDescriptors::GrassSphereNormal (also 1<<5).
src/State.h (1)
152-154: Enum mapping aligns with HLSL; keep synchronized.The C++ bit (1<<5) matches the HLSL flag. Consider adding a brief comment “must match Permutation::ExtraFlags::GrassSphereNormal”.
package/Shaders/RunGrass.hlsl (1)
8-8: Including Permutation.hlsli is appropriate.Enables access to ExtraShaderDescriptor/flags with no register conflicts (uses b4).
src/Hooks.cpp (4)
171-182: EffectShadows bit handling is fine.Clear-then-set pattern avoids stale state; cast hygiene is good.
195-205: IsTree flag set/clear logic LGTM.Uses runtime base object check and consistent casts.
229-246: GrassSphereNormal bit propagation is correct; add cautious null checks upstream.Clear-then-set is correct. The properties[1] access mirrors existing code, but if geometry/properties are ever null, this would silently skip; acceptable, but keep an eye on VR parity.
946-951: Gate new hooks per runtime; verify relocations for VR.You added ctor callsite and vfunc hooks unconditionally. If VR vtables/offsets differ, this can break.
Consider:
- logger::info("Installing SetupGeometry hooks"); - stl::write_vfunc<0x6, EffectExtensions::BSEffectShader_SetupGeometry>(RE::VTABLE_BSEffectShader[0]); - stl::write_vfunc<0x6, LightingExtensions::BSLightingShader_SetupGeometry>(RE::VTABLE_BSLightingShader[0]); - stl::write_thunk_call<GrassExtensions::BSGrassShaderProperty_ctor>(REL::RelocationID(15214, 15383).address() + REL::Relocate(0x45B, 0x4F5)); - stl::write_vfunc<0x6, GrassExtensions::BSGrassShader_SetupGeometry>(RE::VTABLE_BSGrassShader[0]); + logger::info("Installing SetupGeometry hooks"); + stl::write_vfunc<0x6, EffectExtensions::BSEffectShader_SetupGeometry>(RE::VTABLE_BSEffectShader[0]); + stl::write_vfunc<0x6, LightingExtensions::BSLightingShader_SetupGeometry>(RE::VTABLE_BSLightingShader[0]); + if (!REL::Module::IsVR()) { // TODO: add VR relocations once validated + stl::write_thunk_call<GrassExtensions::BSGrassShaderProperty_ctor>(REL::RelocationID(15214, 15383).address() + REL::Relocate(0x45B, 0x4F5)); + stl::write_vfunc<0x6, GrassExtensions::BSGrassShader_SetupGeometry>(RE::VTABLE_BSGrassShader[0]); + }
|
✅ A pre-release build is available for this PR: |
doodlum
changed the title
1 participant
Summary by CodeRabbit
New Features
Performance
Bug Fixes