Skip to content

fix(ibl): blown out main menu - #2596

Merged
SkrubbySkrubInAShrub merged 7 commits into
community-shaders:devfrom
SkrubbySkrubInAShrub:main-menu-ambientsh
Aug 2, 2026
Merged

fix(ibl): blown out main menu#2596
SkrubbySkrubInAShrub merged 7 commits into
community-shaders:devfrom
SkrubbySkrubInAShrub:main-menu-ambientsh

Conversation

@SkrubbySkrubInAShrub

@SkrubbySkrubInAShrub SkrubbySkrubInAShrub commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

fixes #2509

DirectionalAmbient is written per draw from the engine's ambient cube, so it stays correct when a pass rebinds that cube (MistMenu does); the global SH is a once-per-frame snapshot and can disagree.

also removes dead code

Summary by CodeRabbit

Summary by CodeRabbit

  • Bug Fixes

    • Improved ambient lighting behavior when the main or loading menu is open.
    • Directional ambient is now applied only when its directional data is present, reducing lighting inconsistencies.
  • Refactor

    • Updated shared shader data used for ambient/menu behavior, including alignment adjustments for consistency.
  • Chores

    • Removed an unused transform-storage utility.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

State::UpdateSharedData no longer writes the directional ambient transform or menu-state flag. Matching shared-buffer fields and the transform helper are removed, while Lighting.hlsl changes conditional ambient evaluation.

Changes

Ambient shared-data cleanup

Layer / File(s) Summary
Update shared-data layouts and population
src/State.h, package/Shaders/Common/SharedData.hlsli, src/State.cpp
CPU and shader shared-data layouts remove DirectionalAmbient and InMainOrLoadingMenu, padding is adjusted, and the removed fields are no longer populated.
Remove obsolete transform utility
src/Utils/Game.h, src/Utils/Game.cpp
The unused StoreTransform3x4NoScale declaration and implementation are deleted.
Change lighting ambient evaluation
package/Shaders/Lighting.hlsl
directionalAmbientColor uses a conditional intermediate ambient value before the existing clamped ambient-color conversion.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: doodlum, dlizzio, jiayev

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes target the overexposure around the main menu/loading screens and remove the unused ambient/menu plumbing.
Out of Scope Changes check ✅ Passed No clear unrelated changes are introduced; the edits stay focused on the lighting fix and dead-code removal.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change: fixing overexposure in the main menu and loading screens.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

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

@github-actions

Copy link
Copy Markdown

No actionable suggestions for changed features.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/State.cpp`:
- Around line 1054-1070: Ensure the ambient SH coefficients are preserved when
IsMainOrLoadingMenuOpen() is true: avoid uploading zero-initialized
data.AmbientSHR/G/B in that path. Update the surrounding constant-buffer
preparation/upload flow to carry forward the previously cached coefficients or
otherwise upload without modifying those fields, while retaining the existing
DALCToSH update when the menu is closed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4636d0e0-cdd5-4058-a538-3e0694d3bc6a

📥 Commits

Reviewing files that changed from the base of the PR and between 8e8c239 and ec75cc4.

📒 Files selected for processing (1)
  • src/State.cpp

Comment thread src/State.cpp Outdated
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/State.cpp (1)

1007-1007: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Link the fix to its tracking issue.

If this bug fix corresponds to a GitHub issue, add Fixes #<issue-number> or Closes #<issue-number> to the PR description.

As per path instructions, bug-fixing PRs should include an appropriate GitHub keyword reference.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/State.cpp` at line 1007, Add a GitHub closing keyword with the relevant
issue number to the pull request description, such as “Fixes #<issue-number>” or
“Closes #<issue-number>”. No source-code change is required around
IsMainOrLoadingMenuOpen.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/State.cpp`:
- Line 1007: Add a GitHub closing keyword with the relevant issue number to the
pull request description, such as “Fixes #<issue-number>” or “Closes
#<issue-number>”. No source-code change is required around
IsMainOrLoadingMenuOpen.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 11ef72df-82c8-4031-826c-fb24b5f053c9

📥 Commits

Reviewing files that changed from the base of the PR and between ec75cc4 and 6bfd95c.

📒 Files selected for processing (5)
  • package/Shaders/Common/SharedData.hlsli
  • src/State.cpp
  • src/State.h
  • src/Utils/Game.cpp
  • src/Utils/Game.h
💤 Files with no reviewable changes (2)
  • src/Utils/Game.cpp
  • src/Utils/Game.h

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
package/Shaders/Lighting.hlsl (1)

2969-2971: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the issue-closing reference to the PR description.

Because this change fixes issue #2509, add Fixes #2509 (or `Closes `#2509) to the PR body. The existing title already satisfies Conventional Commits, so no title change is needed.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@package/Shaders/Lighting.hlsl` around lines 2969 - 2971, Update the pull
request description to include “Fixes `#2509`” or “Closes `#2509`”. Do not change
the existing Conventional Commits-compliant title.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 2969-2971: Update the pull request description to include “Fixes
`#2509`” or “Closes `#2509`”. Do not change the existing Conventional
Commits-compliant title.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d7af4aab-9acd-4f87-87f5-2014f6b3925a

📥 Commits

Reviewing files that changed from the base of the PR and between 3cfcc9f and 3243e6a.

📒 Files selected for processing (2)
  • package/Shaders/Common/SharedData.hlsli
  • package/Shaders/Lighting.hlsl
💤 Files with no reviewable changes (1)
  • package/Shaders/Common/SharedData.hlsli

@SkrubbySkrubInAShrub
SkrubbySkrubInAShrub merged commit 45b4ef2 into community-shaders:dev Aug 2, 2026
8 checks passed
@SkrubbySkrubInAShrub
SkrubbySkrubInAShrub deleted the main-menu-ambientsh branch August 2, 2026 13:37
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.

loading screens are overexposed

3 participants