Skip to content

Solari fixes #21747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Nov 21, 2025
Merged

Solari fixes #21747

merged 8 commits into from
Nov 21, 2025

Conversation

@JMS55
Copy link
Contributor

@JMS55 JMS55 commented Nov 5, 2025 •
edited
Loading

  • Fix overflow/nans in BRDF evaluation
  • Reduce jacobian rejection threshold to prevent light leaks (more aggressive cutoff now)

@JMS55 JMS55 added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen C-Refinement Improves output quality, without fixing a clear bug or adding new functionality. labels Nov 5, 2025
@JMS55 JMS55 requested review from SparkyPotato and atlv24 November 5, 2025 02:55
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Nov 5, 2025
@alice-i-cecile alice-i-cecile added this to the 0.18 milestone Nov 7, 2025
@JMS55 JMS55 requested a review from SparkyPotato November 7, 2025 15:10
JMS55
JMS55 commented Nov 7, 2025
View reviewed changes
crates/bevy_solari/src/realtime/gbuffer_utils.wgsl
let props = unpack4x8unorm(gpixel.b);
let reflectance = vec3(props.r);
let metallic = props.g;
let metallic = saturate(props.g); // TODO: Not sure why saturate is needed here to prevent NaNs
Copy link
Contributor Author

@JMS55 JMS55 Nov 7, 2025

Choose a reason for hiding this comment

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

Might be something wrong with our gbuffer encoding? @DGriffin91

Copy link
Contributor

@DGriffin91 DGriffin91 Nov 7, 2025

Choose a reason for hiding this comment

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

Seems like any bit pattern of gpixel.b should be valid in respect to metallic. It seems sus if unpack4x8unorm is returning something that's not 0.0..=1.0

Copy link
Contributor Author

@JMS55 JMS55 Nov 8, 2025

Choose a reason for hiding this comment

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

Yeah idk

janhohenheim
janhohenheim approved these changes Nov 20, 2025
View reviewed changes
tychedelia
tychedelia approved these changes Nov 20, 2025
View reviewed changes
Copy link
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Tested and runs on my laptop. Subjectively seems good with the exception that when I turn off both light sources it takes an absurdly long time (5ish seconds) for the light to fade entirely, which seems wrong.

@tychedelia tychedelia added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 20, 2025
@JMS55
Copy link
Contributor Author

JMS55 commented Nov 21, 2025 •
edited
Loading

Tested and runs on my laptop. Subjectively seems good with the exception that when I turn off both light sources it takes an absurdly long time (5ish seconds) for the light to fade entirely, which seems wrong.

That's a known issue with our radiance caching, not related to this PR.

It's very very hard to get something that's both stable and reacts quickly to lighting changes.

#21810 should improve things though.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 21, 2025
Merged via the queue into bevyengine:main with commit a32ffae Nov 21, 2025
36 checks passed
beicause pushed a commit to beicause/bevy that referenced this pull request Nov 26, 2025
@JMS55 @beicause
Solari fixes (bevyengine#21747)
7bd15eb 
* Fix overflow/nans in BRDF evaluation
* Reduce jacobian rejection threshold to prevent light leaks (more
aggressive cutoff now)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atlv24 atlv24 Awaiting requested review from atlv24

@SparkyPotato SparkyPotato Awaiting requested review from SparkyPotato

@IceSentry IceSentry Awaiting requested review from IceSentry

3 more reviewers

@DGriffin91 DGriffin91 DGriffin91 left review comments

@janhohenheim janhohenheim janhohenheim approved these changes

@tychedelia tychedelia tychedelia approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Refinement Improves output quality, without fixing a clear bug or adding new functionality. S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

Rendering
Status: Done

Milestone

0.18

Development

Successfully merging this pull request may close these issues.

6 participants

@JMS55 @janhohenheim @tychedelia @DGriffin91 @SparkyPotato @alice-i-cecile