Skip to content

chore(grass lighting): complex grass improvements#1456

Merged
doodlum merged 2 commits into
devfrom
complex-grass-improvements
Sep 9, 2025
Merged

chore(grass lighting): complex grass improvements#1456
doodlum merged 2 commits into
devfrom
complex-grass-improvements

Conversation

@doodlum
Copy link
Copy Markdown
Collaborator

@doodlum doodlum commented Sep 9, 2025

Summary by CodeRabbit

  • Improvements

    • Refined grass lighting for more natural highlights and smoother falloff.
    • Reduced wrap intensity to prevent over-brightening and improve contrast.
    • Enhanced shadow influence on subsurface scattering for greater depth and realism.
    • More consistent directional and point light behavior across grass surfaces.
  • Refactor

    • Simplified lighting calculations for consistent results across light types.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Sep 9, 2025

Walkthrough

Replaces a boolean complexity check with a texture-sampled length test, halves wrapAmount, simplifies wrapped directional and point-light wrap computations (applied also in LIGHT_LIMIT_FIX), and increases shadow influence on subsurface scattering in RunGrass.hlsl.

Changes

Cohort / File(s) Summary of Changes
Lighting and shading adjustments
package/Shaders/RunGrass.hlsl
- Replace x != y complexity check with a sample from TexBaseSampler at int3(0, int(y)-1, 0) and set complex when length(sample.xyz) is between 0.9 and 1.1.
- Scale wrapAmount by 0.5: wrapAmount = saturate(input.VertexNormal.w * 10.0) * 0.5.
- Simplify wrapped directional and point light: saturate(angle + wrapAmount) / (1.0 + wrapAmount); apply same formula in LIGHT_LIMIT_FIX path (removed previous per-light lightViewWrap / lightNoL logic).
- Increase subsurface scattering shadow influence: multiply SSS term by lerp(1.0, dirDetailShadow, 0.5).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Frame
  participant PixelShader as RunGrass.hlsl
  participant TexBase as TexBaseSampler
  participant Output as Color

  Frame->>PixelShader: invoke per-pixel shading
  PixelShader->>TexBase: Load(int3(0, int(y)-1, 0))
  TexBase-->>PixelShader: texel.xyz
  Note right of PixelShader: complex = (0.9 < length(texel.xyz) < 1.1)
  PixelShader->>PixelShader: wrapAmount = saturate(normal.w*10.0) * 0.5
  PixelShader->>PixelShader: wrappedDir = saturate(dirAngle + wrapAmount)/(1+wrapAmount)
  PixelShader->>PixelShader: wrappedPoint = saturate(lightAngle + wrapAmount)/(1+wrapAmount)
  PixelShader->>PixelShader: sss *= lerp(1.0, dirDetailShadow, 0.5)
  PixelShader->>Output: output final shaded color
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • jiayev

Pre-merge checks (1 passed, 1 warning, 1 inconclusive)

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Description Check ⚠️ Warning There is no description provided that outlines the intent or details of this changeset, so reviewers lack context about the specific updates to the RunGrass.hlsl shader, such as the complexity test overhaul, wrap amount adjustment, and subsurface shading tweaks. Please add a descriptive PR summary that briefly explains the motivation and key changes, for example noting the replacement of the complexity test with a texture‐based approach, the wrap amount scaling adjustment, and the subsurface shading improvements.
Title Check ❓ Inconclusive The current title specifies the scope as grass lighting and notes improvements, but the phrasing “complex grass improvements” is ambiguous and does not clearly convey the primary changes made, making it too vague for a teammate scanning history to immediately understand the main update. Consider renaming the PR to clearly highlight the key changes, for example “Refine grass lighting complexity and wrap shading in RunGrass.hlsl,” so that the title succinctly captures the primary modifications.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Poem

I nibble light in blades of green,
A sampled whisper tells what's seen.
Wrap halved, angles softened neat,
Shadows tuck SSS more sweet.
Tiny texels hum—grass gleams between. 🐇🌿


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e69c7bc and 9e8ec4f.

📒 Files selected for processing (1)
  • package/Shaders/RunGrass.hlsl (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • 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: Build plugin and addons
  • GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
  • GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch complex-grass-improvements

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 9, 2025

Using provided base ref: 40a6bc3
Using base ref: 40a6bc3
Base commit date: 2025-09-09T14:18:56+01:00 (Tuesday, September 09, 2025 02:18 PM)
No actionable suggestions for changed features.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
package/Shaders/RunGrass.hlsl (2)

595-599: Wrap intensity halved; verify visual balance, and drop redundant saturate

  • Halving wrap changes overall grass brightness and rimming; please sanity-check a few time-of-day scenes.
  • wrappedDirLight is already saturated; the extra saturate() on Line 599 is redundant.
-	lightsDiffuseColor += dirLightColor * saturate(wrappedDirLight) * dirDetailShadow;
+	lightsDiffuseColor += dirLightColor * wrappedDirLight * dirDetailShadow;

617-617: SSS shadow coupling may flicker; consider a smoother mask

Screen-space shadow masks are noisy; squaring the factor reduces shimmer on thin blades with backlighting.

-	float3 sss = dirBacklighting * dirLightColor * saturate(-dirLightAngle) * lerp(1.0, dirDetailShadow, 0.5);
+	float3 sss = dirBacklighting * dirLightColor * saturate(-dirLightAngle)
+	           * lerp(1.0, saturate(dirDetailShadow * dirDetailShadow), 0.5);
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40a6bc3 and e69c7bc.

📒 Files selected for processing (1)
  • package/Shaders/RunGrass.hlsl (4 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 (1)
📓 Common learnings
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build plugin and addons
  • GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
  • GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (1)
package/Shaders/RunGrass.hlsl (1)

678-679: Point-light wrap formula change: confirm parity with directional and energy feel

The simplified wrapped Lambert for clustered lights matches the directional path; good for consistency. Please spot-check near point/spot lights for over-bright grazing angles given the shared wrapAmount from VertexNormal.w (authoring-dependent).

Comment thread package/Shaders/RunGrass.hlsl Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 9, 2025

✅ A pre-release build is available for this PR:
Download

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants