Skip to content

refactor(skylighting): simplify skylightingSH initialization#2139

Merged
alandtse merged 1 commit into
devfrom
smallcleanup
Apr 17, 2026
Merged

refactor(skylighting): simplify skylightingSH initialization#2139
alandtse merged 1 commit into
devfrom
smallcleanup

Conversation

@jiayev
Copy link
Copy Markdown
Collaborator

@jiayev jiayev commented Apr 17, 2026

This pull request refactors how the skylightingSH variable is initialized and used in the RunGrass.hlsl shader. The main improvement is to only declare and assign skylightingSH when skylighting is actually sampled, simplifying the code and reducing unnecessary variable declarations.

Shader logic simplification:

  • Removed the static initialization of skylightingSH (unitSH_grass1 and unitSH_grass2) and now declares and assigns skylightingSH only when skylighting is sampled, making the code cleaner and reducing unnecessary assignments. (package/Shaders/RunGrass.hlsl, [1] [2]

Summary by CodeRabbit

  • Bug Fixes

    • Improved grass skylighting calculation to ensure proper lighting in interior and exterior areas.
  • Refactor

    • Optimized shader code structure for grass skylighting computation with reduced memory overhead.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 17, 2026

📝 Walkthrough

Walkthrough

The grass pixel shader's skylighting spherical-harmonics workflow is simplified by eliminating pre-initialized compile-time basis constants and directly initializing the skylightingSH variable from Skylighting::sample() calls within scoped branches.

Changes

Cohort / File(s) Summary
Skylighting SH Initialization Refactor
package/Shaders/RunGrass.hlsl
Removed static const SH basis declarations and changed initialization pattern: skylightingSH is now directly assigned from Skylighting::sample() within conditional branches instead of being pre-initialized to a basis value and conditionally reassigned.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • doodlum
  • davo0411
  • alandtse

Poem

🐰 A shader so clever, now lean and so tight,
No wasteful basis constants bloating the light,
Direct from the sample, the SH takes its place,
With scopes keeping order, we optimize grace! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title 'refactor(skylighting): simplify skylightingSH initialization' accurately describes the main change—refactoring the skylightingSH variable initialization by removing static basis initialization and simplifying the assignment pattern.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch smallcleanup

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

No actionable suggestions for changed features.

@github-actions
Copy link
Copy Markdown

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

@alandtse alandtse changed the title refactor(skylighting): small cleanup refactor(skylighting): simplify skylightingSH initialization Apr 17, 2026
@alandtse alandtse merged commit 03e2014 into dev Apr 17, 2026
21 checks passed
@alandtse alandtse deleted the smallcleanup branch April 17, 2026 05:44
YtzyFvra pushed a commit to YtzyFvra/skyrim-community-shaders that referenced this pull request Apr 19, 2026
ParticleTroned pushed a commit to ParticleTroned/skyrim-community-shaders that referenced this pull request May 2, 2026
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.

3 participants