Skip to content
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

Add multisampled antialiasing #377

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Add multisampled antialiasing #377

merged 2 commits into from
Oct 12, 2023

Conversation

raphlinus
Copy link
Contributor

This is ported from the multi branch.

Configuration of antialiasing mode is currently set statically, but could become more dynamic. In addition, the mask LUT is computed and uploaded every frame, rather than being persistent.

This is ported from the multi branch.

Configuration of antialiasing mode is currently set statically, but could become more dynamic. In addition, the mask LUT is computed and uploaded every frame, rather than being persistent.
shader/fine.wgsl Outdated
Comment on lines 148 to 153
#ifdef have_uniform
let total = workgroupUniformLoad(&sh_count[slice_size - 1u]);
#else
workgroupBarrier();
let total = sh_count[slice_size - 1u];
#endif
Copy link
Collaborator

@armansito armansito Oct 11, 2023

Choose a reason for hiding this comment

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

I think we always use workgroupUniformLoad in the other shaders now, so you can do that here too. Then again I'm not sure if the second barrier that it injects is useful in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need it to pass uniformity analysis. In any case, this was a holdover from the original multi branch being on an older wgpu that didn't have it, so good catch.

shader/fine.wgsl Show resolved Hide resolved
src/mask.rs Outdated
Copy link
Collaborator

@armansito armansito Oct 11, 2023

Choose a reason for hiding this comment

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

Would you mind moving this into crates/encoding? That way this utility can be available to any renderer that desires it. It doesn't pertain to the scene encoding per se but it is related to managing GPU buffers which is currently a feature of the encoding crate.

I'm also happy to move it over in a follow up if you don't have time. I'm going to start working on enabling this in the crates once the PR lands.

Comment on lines +183 to +184
let dy_dxdy = dy / (dx + dy);
let a = dx / (dx + dy);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may want to define and reuse idxdy = 1.0 / (dx + dy) to avoid computing it twice. An optimizing shader compiler could reduce the divisor in both lines into one computation but I don't know if that's guaranteed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a TODO here. When #378 is fixed, we'll want to apply it here as well.

An optimizing compiler could do it, but one that satisfied IEEE arithmetic could not.

shader/fine.wgsl Outdated Show resolved Hide resolved
@armansito
Copy link
Collaborator

I'm seeing artifacts on cartioid with both msaa8 and msaa16.

I don't know if I want to necessarily block this PR on this as it can be investigated in a follow up. Unless you want to fix that in this PR, consider setting the default static AA config to AaConfig::Area until msaa is stabilized.

Screenshot 2023-10-11 at 4 10 38 PM

@raphlinus
Copy link
Contributor Author

I think the artifacts you're seeing are because of only 4 bits for winding number accumulation, and that test case is a good one for slamming lots of winding number increments into a small area. I'm not 100% sure though. An obvious way to validate that would be to try an 8 bit accumulator.

In any case, agree with keeping area as the default and considering this a followup.

Minor cleanup of the multisampling logic, and switch default to Area.
@raphlinus raphlinus merged commit 4edf786 into main Oct 12, 2023
4 checks passed
@raphlinus raphlinus deleted the multi2 branch October 12, 2023 01:57
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.

2 participants