Support for Multisampled Anti-Aliasing by EriKWDev · Pull Request #213 · kvark/blade

EriKWDev · 2024-12-04T13:50:41Z

closes #198

Changes

This PR introduces a sample_count: u32 to TextureDesc as well as a multisample_state: MultisampleState to the RenderPipelineDesc. Also added alpha_to_coverage while at it.

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub struct MultisampleState {
    pub sample_count: u32,
    pub sample_mask: u64,
    pub alpha_to_coverage: bool,
}

impl Default for MultisampleState {
    fn default() -> Self {
        Self {
            sample_count: 1,
            sample_mask: !0,
            alpha_to_coverage: false,
        }
    }
}

Together with the existing FinishOp::ResolveTo(TextureView), a multisampled renderpass can now be described and executed

Implementations

Vulkan

The rendering attachment needs to be told to resolve

if let crate::FinishOp::ResolveTo(resolve_view) = rt.finish_op {
    vk_info = vk_info
        .resolve_image_view(resolve_view.raw)
        .resolve_image_layout(vk::ImageLayout::GENERAL)
        .resolve_mode(vk::ResolveModeFlags::AVERAGE);
}

The store_op is currently always set to DONT_CARE as in many cases for msaa rendering the resolved texture is the only one required. Would be nice to be able to specify this behaviour in blade as well though:

vk_info.store_op = match rt.finish_op {
    crate::FinishOp::ResolveTo(..) => {
        /*
            TODO: DONT_CARE is most optimal in many cases where the msaa texture itself is never read afterwards but only the resolved,
                  but how can the user specify this in blade?
                  https://docs.vulkan.org/samples/latest/samples/performance/msaa/README.html#_best_practice_summary
        */

        // vk::AttachmentStoreOp::STORE
        vk::AttachmentStoreOp::DONT_CARE
    }
};

For the texture creation, a SampleCountFlags is constructed for the vk::ImageCreateInfo:

/// in vulkan/resource.rs:268
samples: vk::SampleCountFlags::from_raw(desc.sample_count),

and for the pipeline, I looked at how wgpu does it and came to the following code

let vk_sample_mask = [
    desc.multisample_state.sample_mask as u32,
    (desc.multisample_state.sample_mask >> 32) as u32,
];

let vk_multisample = vk::PipelineMultisampleStateCreateInfo::default()
    .rasterization_samples(vk::SampleCountFlags::from_raw(
        desc.multisample_state.sample_count,
    ))
    .alpha_to_coverage_enable(desc.multisample_state.alpha_to_coverage)
    .sample_mask(&vk_sample_mask);

Metal

The metal backend already had support for the `ResolveTo` finishop so no changes needed for the renderpipeline. The creation of the renderpipeline required description though:

descriptor.set_raster_sample_count(desc.multisample_state.sample_count);
descriptor.set_alpha_to_coverage_enabled(desc.multisample_state.alpha_to_coverage);

For the texture, only specifying the sample_count was required

/// in metal/resource.rs:205
descriptor.set_sample_count(desc.sample_count as u64);

OpenGL ES

This was the hardest one as I am very unfamiliar with msaa in opengl, but in summary the texture needs to be created with renderbuffer_storage_multisample if it is a rendertarget and with a glow::TEXTURE_2D_MULTISAMPLE target type if it is a normal texture.

The sample count is then set with tex_storage_2d_multisample, but there is now no-longer a way to specify mip_count so don't know if that is even possible.

/*
    TODO(ErikWDev): How to set mip count and sample count? Not possible in gles?
*/
gl.tex_storage_2d_multisample(
    target,
    desc.sample_count as i32,
    format_desc.internal,
    desc.size.width as i32,
    desc.size.height as i32,
    true,
);

For the rendering, I use blit_framebuffer to blit the msaa texture onto the resolve target which required turning the renderbuffers into FBO:s. Currently, the FBO:s are created and deleted ad-hoc but could be saved as done in wgpu-hal. See #198

/// in gles/command.rs:814
Self::BlitFramebuffer { from, to } => {
    /*
        TODO(ErikWDev): Validate
    */

    let target_from = match from.inner {
        super::TextureInner::Renderbuffer { raw } => raw,
        _ => panic!("Unsupported resolve between non-renderbuffers"),
    };
    let target_to = match to.inner {
        super::TextureInner::Renderbuffer { raw } => raw,
        _ => panic!("Unsupported resolve between non-renderbuffers"),
    };

    let framebuf_from = gl.create_framebuffer().unwrap();
    let framebuf_to = gl.create_framebuffer().unwrap();

    gl.bind_framebuffer(glow::READ_FRAMEBUFFER, Some(framebuf_from));
    gl.framebuffer_renderbuffer(
        glow::READ_FRAMEBUFFER,
        glow::COLOR_ATTACHMENT0, // NOTE: Assuming color attachment
        glow::RENDERBUFFER,
        Some(target_from),
    );

    gl.bind_framebuffer(glow::DRAW_FRAMEBUFFER, Some(framebuf_to));
    gl.framebuffer_renderbuffer(
        glow::DRAW_FRAMEBUFFER,
        glow::COLOR_ATTACHMENT0, // NOTE: Assuming color attachment
        glow::RENDERBUFFER,
        Some(target_to),
    );
    assert_eq!(
        gl.check_framebuffer_status(glow::DRAW_FRAMEBUFFER),
        glow::FRAMEBUFFER_COMPLETE,
        "DRAW_FRAMEBUFFER is not complete"
    );

    gl.blit_framebuffer(
        0,
        0,
        from.target_size[0] as _,
        from.target_size[1] as _,
        0,
        0,
        to.target_size[0] as _,
        to.target_size[1] as _,
        glow::COLOR_BUFFER_BIT, // NOTE: Assuming color
        glow::NEAREST,
    );

    gl.bind_framebuffer(glow::READ_FRAMEBUFFER, None);
    gl.bind_framebuffer(glow::DRAW_FRAMEBUFFER, None);

    gl.delete_framebuffer(framebuf_from);
    gl.delete_framebuffer(framebuf_to);
}

Testing

The vulkan implementation has been validated on an AMD RX 6600 on debian, an integrated intel GPU on Fedora as well as a GTX 1050 on Windows and it all seems to work (the particle sample). After inspection in renderdoc the result is as expected.

The metal implementation has been tested on a Mac Mini M4 and works. However, particle example seems to break after the sample count is adjusted in runtime for some reason.

The Opengl ES implementation has not been tested!!!

Particle Example

I changed the particle example to now utilize msaa which requires it to keep a msaa texture with the desired sample_count available and recreated upon resizing as well as a FinishOp::ResolveTo to resolve the msaa texture to the acquired frame texture as such:

if let mut pass = self.command_encoder.render(
    "draw",
    gpu::RenderTargetSet {
        colors: &[gpu::RenderTarget {
            view: self.msaa_view,
            init_op: gpu::InitOp::Clear(gpu::TextureColor::OpaqueBlack),
            finish_op: gpu::FinishOp::ResolveTo(frame.texture_view()),
        }],
        depth_stencil: None,
    },
) {
    self.particle_system
        .draw(&mut pass, screen_desc.physical_size);
    self.gui_painter
        .paint(&mut pass, gui_primitives, screen_desc, &self.context);
}

Egui

Furthermore, the blade_egui renderer also requires information about the multisample state since it has a pipeline that now needs information about the texture it's going to render to so it's initializer now takes a sample_count as well:

pub fn new(
    info: blade_graphics::SurfaceInfo,
    context: &blade_graphics::Context,
    sample_count: u32,
) -> GuiPainter {
// ...
}

Let me know what needs changing or if testing fails somewhere. Especially the Opengl ES implementation as I didn't know how to run using it

kvark

This is amazing work, thank you!
High quality, something that any open source project would cherish and greatly appreciate.
Very solid for the first contribution, I'm excited to see this finished.

kvark · 2024-12-05T07:11:20Z

+
+            Self::BlitFramebuffer { from, to } => {
+                /*
+                    TODO(ErikWDev): Validate


Please follow these instructions - https://github.com/kvark/blade/tree/main/blade-graphics#opengl-es
and let me know if something doesn't work!

Sadly, I am unable to get it to work.

[2024-12-07T20:55:05Z ERROR blade_graphics::hal::platform] EGL 'eglCreatePlatformWindowSurface' code 0x300b: _eglCreateWindowSurfaceCommon thread 'main' panicked at blade-graphics/src/gles/egl.rs:431:26: called `Result::unwrap()` on an `Err` value: BadNativeWindow

I have tried forcing X11 and Wayland and both return the same issue. I am running on Debian GNU/Linux 12 (bookworm) x86_64 with kernel 6.1.0-28-amd64.

I have libgles-dev 1.6.0-1 installed as well as libegl-dev 1.6.0-1

Ok, good to know, thank you for trying!
This has probably regressed recently due to #203
I'll check on a few more devices I have...

kvark · 2024-12-05T07:12:31Z

+
+                let target_from = match from.inner {
+                    super::TextureInner::Renderbuffer { raw } => raw,
+                    _ => panic!("Unsupported resolve between non-renderbuffers"),


Is this just a thing we want to leave for a follow-up, or you are seeing actual technical difficulty here? I think, handling a texture here should be straightforward.

Nop, just unfamiliar with opengl. Found framebufferTexture2D so should be easy

Think this should work

gl.bind_framebuffer(glow::READ_FRAMEBUFFER, Some(framebuf_from)); match from.inner { super::TextureInner::Renderbuffer { raw } => { gl.framebuffer_renderbuffer( glow::READ_FRAMEBUFFER, glow::COLOR_ATTACHMENT0, // NOTE: Assuming color attachment glow::RENDERBUFFER, Some(raw), ); } super::TextureInner::Texture { raw, target } => { gl.framebuffer_texture_2d( glow::READ_FRAMEBUFFER, glow::COLOR_ATTACHMENT0, target, Some(raw), 0, ); } }

yep! if in doubt, we can also check wgpu-hal

kvark · 2024-12-05T07:24:18Z

    }

-    pub fn draw(&self, pass: &mut gpu::RenderCommandEncoder) {
+    pub fn draw(&self, pass: &mut gpu::RenderCommandEncoder, size: (u32, u32)) {


should we just get the aspect here?

Used the size and sent it directly to the shader before but changed it to be the aspect ratio xP Maybe should be the input to the function now yeah since that is what is used

kvark · 2024-12-05T07:28:40Z


 mod particle;

+// const SAMPLE_COUNT: u32 = 1;


would be useful to support SAMPLE_COUNT = 1 in the code here, which would mean - not creating the extra MSAA texture at all

Usually do it that way. Should I make the SAMPLE_COUNT parameter dynamically changable in the UI as well? Should be possible but the pipeline needs recreating as well in that case

it's a nice feature but it wouldn't block this PR from landing

EriKWDev · 2024-12-05T07:55:40Z

This is amazing work, thank you! High quality, something that any open source project would cherish and greatly appreciate. Very solid for the first contribution, I'm excited to see this finished.

Compared to having created this whole package for us to use and tinker with I don't think its much :) We will continue to evaluate blade for a period and my intention is to contribute if I make good changes.

Can probably fix some of the open winit issues too since have been having to deal with their API changes as well for our game..

EriKWDev · 2024-12-05T09:49:55Z

will re-request review when ready. Will get my hands on a mac and also try the metal as well as gles impl

EriKWDev · 2024-12-06T12:55:36Z

Don't know if related: For some reason I feel like the egui in blade is much more pixelated than I am used to. To the left is our game using wgpu and right is blade

screenshots

opened #215

…pile

…e view

EriKWDev · 2024-12-08T22:02:07Z

(force push from rebase on main)

kvark

Looks great, let's get this in!
Just one question though - how do you see this being merged? Do you want to make a pass over commits to have a reasonable (and testable) history, or I can just squash it all together.

kvark · 2024-12-09T03:42:15Z


 [target.'cfg(target_arch = "wasm32")'.dev-dependencies]
+# see https://github.com/emilk/egui/issues/4270
+egui-winit = { version="0.29", default-features=false, features=["links"] }


it's now the same dependency between wasm and normal, can we move it into a common block of dev dependencies?

kvark · 2024-12-09T03:43:28Z

                    // ns_view always have a layer and don't need to verify that it exists.
-                    let layer: *mut Object = msg_send![handle.ns_view.as_ptr(), layer];
+                    let layer: *mut Object =
+                        msg_send![handle.ns_view.as_ptr() as *mut Object, layer];


curious, does this cast do anything? I think the type is essentially erased at that point, anyway

It didn't compile for me, msg_send![] required the thing to implement the "Message" trait and it wasn't implemented by a raw pointer. The cast fixed the compilation though ofc no guarantees

This made it compile, though I could still not get it to run :/

EriKWDev · 2024-12-09T22:12:12Z

As for merging, I am totally fine with a squash.

The reason I am still hesitant is I have not gotten the OpenGL version to run as the surface creation fail, so can only guess that my code is working in that path.

I also still observe an issue on pipeline recreation on Mac for the new particle sample (when sample count changes), though will continue looking into it as I am working to get blade running on iOS

@kvark

Closes #20762 Release Notes: - N/A --- Enable MSAA for Anti-Aliasing to Path (`cx.paint_path`) for drawing a better vector graphics. ```bash cargo run -p gpui --example gradient --features macos-blade cargo run -p gpui --example gradient cargo run -p gpui --example painting --features macos-blade cargo run -p gpui --example painting ``` **Before** <img width="1089" alt="image" src="https://github.com/user-attachments/assets/0ae7240f-4ba9-4ef5-896c-e436c1282770" /> **After** <img width="944" alt="image" src="https://github.com/user-attachments/assets/71a07ae8-be54-452c-aacc-b8cec1f810c0" /> ## TODO - [x] Support Metal and Blade. - [x] Detect system support to set up sample count. - [x] Fix extra lines between Path vertices wait #22808 to merge. 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](https://github.com/kvark/blade/tree/main/examples/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.

@kvark

Closes #20762 Release Notes: - N/A --- Enable MSAA for Anti-Aliasing to Path (`cx.paint_path`) for drawing a better vector graphics. ```bash cargo run -p gpui --example gradient --features macos-blade cargo run -p gpui --example gradient cargo run -p gpui --example painting --features macos-blade cargo run -p gpui --example painting ``` **Before** <img width="1089" alt="image" src="https://github.com/user-attachments/assets/0ae7240f-4ba9-4ef5-896c-e436c1282770" /> **After** <img width="944" alt="image" src="https://github.com/user-attachments/assets/71a07ae8-be54-452c-aacc-b8cec1f810c0" /> ## TODO - [x] Support Metal and Blade. - [x] Detect system support to set up sample count. - [x] Fix extra lines between Path vertices wait #22808 to merge. 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](https://github.com/kvark/blade/tree/main/examples/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.

kvark reviewed Dec 5, 2024

View reviewed changes

EriKWDev marked this pull request as draft December 5, 2024 09:48

EriKWDev added 13 commits December 8, 2024 22:51

add sample_count to texture + msaa state to render pip desc, make com…

e2e0881

…pile

Implement msaa API + particle example use msaa

e0028d7

no sample_count in egui

34d7be6

initial support for changing the msaa dynamically

ecf5d19

support msaa 2d array textures

3a7b351

make compile on metal + add missing sample_count dependency in textur…

f9c281e

…e view

get real surface format when recreating particle system

58b57d7

use correct texture constant in gl

c93df83

fix compiling egl gles on macos

4795021

tmp

9d652a1

support blitting to/from texture, not just renderbuffers

e7d7148

ResolveTo docs

334c127

winit

a63bc0d

EriKWDev force-pushed the ErikWDev/msaa-support branch from e849398 to a63bc0d Compare December 8, 2024 22:01

EriKWDev mentioned this pull request Dec 8, 2024

Ability to allocate transient buffers/textures + hint to lazy allocate #219

Open

kvark approved these changes Dec 9, 2024

View reviewed changes

kvark marked this pull request as ready for review December 10, 2024 05:24

kvark merged commit 450bb7d into kvark:main Dec 10, 2024

This was referenced Jan 7, 2025

gpui: Draw Path with 2x to anti-aliasing. huacnlee/zed#13

Closed

gpui: Add PathBuilder based on lyon to build Path zed-industries/zed#22808

Merged

gpui: Enable MSAA to Path render for Anti-Aliasing zed-industries/zed#22812

Merged

Conversation

EriKWDev commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Implementations

Testing

Particle Example

Egui

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EriKWDev Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EriKWDev Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EriKWDev commented Dec 5, 2024

Uh oh!

EriKWDev commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EriKWDev commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EriKWDev commented Dec 8, 2024

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EriKWDev Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EriKWDev commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

EriKWDev commented Dec 4, 2024 •

edited

Loading

EriKWDev Dec 7, 2024 •

edited

Loading

EriKWDev Dec 7, 2024 •

edited

Loading

EriKWDev commented Dec 5, 2024 •

edited

Loading

EriKWDev commented Dec 6, 2024 •

edited

Loading

EriKWDev Dec 9, 2024 •

edited

Loading

EriKWDev commented Dec 9, 2024 •

edited

Loading