-
Notifications
You must be signed in to change notification settings - Fork 372
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
[re_renderer] Enable 4x MSAA #276
Conversation
TODO: seems webgl ignores alpha to coverage and then everything looks just a little it worse than before (because we're hitting the edges of the squares essentially as we don't discard when we should) |
was a matter of fixing it in wgpu as well gfx-rs/wgpu#3187 |
also better document the various color conversions and formats
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.
Looks great!
It would be great to also set multisampling: 1
explicitly in crates/re_viewer/src/native.rs
, with a note to why
// TODO(andreas): Do something meaningful with values above 1 | ||
let hdr = clamp(hdr, vec3<f32>(0.0), vec3<f32>(1.0)); | ||
return Vec4(srgb_from_linear(hdr), 1.0); | ||
let input = clamp(input, vec3<f32>(0.0), vec3<f32>(1.0)); |
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.
let input = clamp(input, vec3<f32>(0.0), vec3<f32>(1.0)); | |
let input = clamp(input, Vec3(0.0), Vec3(1.0)); |
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.
doesn't work gfx-rs/naga#2105
tbh I'm not so sure about our typedefs
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.
Dare I even say:
let input = clamp(input, vec3<f32>(0.0), vec3<f32>(1.0)); | |
let input = clamp(input, ZERO, ONE); |
👀
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.
witchcraft!
var modified_radius = point_data.radius * distance_to_camera_inv * sqrt(distance_to_camera_sq - radius_sq); | ||
// We're computing a coverage mask int the fragment shader - make sure the quad doesn't cut off our antialiasing. | ||
// It's fairly subtle but if we don't do this our spheres look slightly squarish | ||
modified_radius += frame.pixel_world_size_from_camera_distance / distance_to_camera_inv; |
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.
Good comments ⭐ ⭐ ⭐
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 love Iq's work, but took me ages to understand his sphere AA shadertoy and then a bit more to understand "custom" pieces like this one :)
5251c9e
to
6299bf4
Compare
Fixes PRO-244 |
Enables 4x MSAA and alpha to coverage for point cloud.
left new with MSAA, right old no MSAA
For the latter it took me quite a while to disect what Iq is doing here, but I think I was able to grasp it and put my knowledge into comments and overly long variable names.
left new with alpha to coverage, right old no MSAA
Two wgpu PRs came out of this work regarding webgl compatibility.
The Alpha to coverage one has landed and the float MSAA one is no longer required as I disabled HDR rendering for the time being (thanks @emilk for pointing out that we don't actually need it yet). There's comments about that in the code.
Checklist
CHANGELOG.md
(if this is a big enough change to warrant it)