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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Solari fixes #21747

wants to merge 6 commits into from
+11 −7

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

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

@tychedelia tychedelia Awaiting requested review from tychedelia

1 more reviewer

@DGriffin91 DGriffin91 DGriffin91 left review comments

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-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

Rendering
Status: No status

Milestone

0.18

Development

Successfully merging this pull request may close these issues.

4 participants

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