-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
gpui: Enable MSAA to Path render for Anti-Aliasing #22812
base: main
Are you sure you want to change the base?
Conversation
Try a Shader change to use gather
✅ UPDATE: Is looks like not need to do this, reverted this change. BeforeAfterSame case in Sketch |
…th 4 points." This reverts commit 693258f.
There need a method to check if the system supports a specific sample count. Ref zed-industries/zed#22812 need this feature to setup sample_count.
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.
Nice work!
@@ -128,7 +129,7 @@ struct BladePipelines { | |||
} | |||
|
|||
impl BladePipelines { | |||
fn new(gpu: &gpu::Context, surface_info: gpu::SurfaceInfo) -> Self { | |||
fn new(gpu: &gpu::Context, surface_info: gpu::SurfaceInfo, sample_count: u32) -> Self { |
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.
the argument should say explicitly that it's only for the path rasterization, e.g. path_sample_count
|
||
// TODO: Determine on non-macOS platforms, until Blade supports querying sample counts. | ||
#[cfg(not(target_os = "macos"))] | ||
let sample_count = 4; |
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.
do you really need the query? pretty sure the sample count of 4
is universally supported on all platforms
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.
Oh, here is a reference:
https://developer.apple.com/documentation/metal/mtldevice/1433355-supportstexturesamplecount
Sample Count | Devices |
---|---|
1 | All devices |
2 | All iOS devices All tvOS devices Some macOS devices |
4 | All devices |
8 | Some macOS devices |
} | ||
|
||
impl BladeAtlas { | ||
pub(crate) fn new(gpu: &Arc<gpu::Context>) -> Self { | ||
pub(crate) fn new(gpu: &Arc<gpu::Context>, sample_count: u32) -> Self { |
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 is a bit wasteful. We only want MSAA atlas tiles for things that actually require MSAA.
The atlas contains texture of different formats via AtlasTextureKind
.
A general way to implement it is to turn BladeAtlasStorage
into basically a HashMap<BladeAtlasKey, AtlasTextureList<BladeAtlasTexture>>
, where BladeAtlasKey
would be a combination of both AtlasTextureKind
and the sample count.
A less general way would be to just make it explicitly path_sample_count
, so that it only affects the rasterized path.
Honestly, I think this is a better way due to its simplicity.
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.
Makes sense.
I have changed only Path kind to create msaa_texutre
let mut pass = self.command_encoder.render( | ||
"paths", | ||
let frame_view = tex_info.raw_view; | ||
let render_target = if let Some(msaa_view) = tex_info.msaa_view { | ||
gpu::RenderTargetSet { |
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 be slightly different
let color_target = if let Some(msaa_view) = tex_info.msaa_view {
gpu::RenderTarget {
view: tex_info.raw_view,
view: msaa_view,
init_op: gpu::InitOp::Clear(gpu::TextureColor::OpaqueBlack),
finish_op: gpu::FinishOp::ResolveTo(frame_view),
}
} else {
gpu::RenderTarget {
view: frame_view,
init_op: gpu::InitOp::Clear(gpu::TextureColor::OpaqueBlack),
finish_op: gpu::FinishOp::Store,
}
}
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.
Do you means should rename render_target
to color_target
? Or is other I missed?
Because the code that you given, there have a mistake (duplicate view
), so I'm not sure.
Closes #20762
Release Notes:
Enable MSAA for Anti-Aliasing to Path (
cx.paint_path
) for drawing a better vector graphics.Before
After
TODO
Ref kvark/blade#213
Ask @kvark to review.
I am not sure if there is anything I missed. I modified it according to the particle example of Blade project. But the difference is that after the first MSAA render, I did not do it a second time, I tested it and found it was not necessary.
Before
After
It looks no difference?