-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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.
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
[Merged by Bors] - Include 2x/8x sample counts for Msaa #7684
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nice and simple change. I think disabling MSAA for unsupported levels is probably the right choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should default to the default value (so 4) if it's supported and not what is asked, then disable if even the default value is not supported
So
- 2x / 8x requested but not supported, 4x is supported -> set it to 4x
- 2x / 4x / 8x not supported -> set it to off
This should be the case now |
msaa.samples(), | ||
); | ||
*msaa = Msaa::Off; | ||
if sample_flags.sample_count_supported(Msaa::default().samples()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than two different log statements in and f, use the if statement to determine the fallback sample count, and then set that once and use one log statement. That will be cleaner.
Pseudo code:
let fallback_sample = if default_supported() { default } else { off };
log("Not supported, falling back to {fallback_sample}");
*msaa = fallback_sample;
Msaa::Off | ||
}; | ||
|
||
let fallback_str = if fallback == Msaa::Off { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually small nit. Remove to_owned(), and &borrow the format!().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that will work since it doesn't live long enough
let fallback_str = if fallback == Msaa::Off { | ||
"disabling MSAA".to_owned() | ||
} else { | ||
format!("MSAA {}x", fallback.samples()) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nicer to use a Debug
(or Display
) impl of the Masa
enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
# Objective Fixes #7295 Should we maybe default to 4x if 2x/8x is selected but not supported? --- ## Changelog - Added 2x and 8x sample counts for MSAA.
# Objective Fixes bevyengine#7295 Should we maybe default to 4x if 2x/8x is selected but not supported? --- ## Changelog - Added 2x and 8x sample counts for MSAA.
Objective
Fixes #7295
Should we maybe default to 4x if 2x/8x is selected but not supported?
Changelog