MSAA config: address review comments

examples/with_winit/src/lib.rs

@@ -114,7 +114,8 @@ fn run(
     let mut vsync_on = true;
 
     const AA_CONFIGS: [AaConfig; 3] = [AaConfig::Area, AaConfig::Msaa8, AaConfig::Msaa16];
-    let mut aa_config_ix = 0;
+    // We allow cycling through AA configs in either direction, so use a signed index
+    let mut aa_config_ix: i32 = 0;
 
     let mut frame_start_time = Instant::now();
     let start = Instant::now();
@@ -134,8 +135,8 @@ fn run(
     }
     let mut profile_stored = None;
     let mut prev_scene_ix = scene_ix - 1;
-    let mut prev_aa_config_ix = aa_config_ix;
     let mut profile_taken = Instant::now();
+    let mut modifiers = ModifiersState::default();
     // _event_loop is used on non-wasm platforms to create new windows
     event_loop.run(move |event, _event_loop, control_flow| match event {
         Event::WindowEvent {
@@ -150,6 +151,7 @@ fn run(
             }
             match event {
                 WindowEvent::CloseRequested => *control_flow = ControlFlow::Exit,
+                WindowEvent::ModifiersChanged(m) => modifiers = *m,
                 WindowEvent::KeyboardInput { input, .. } => {
                     if input.state == ElementState::Pressed {
                         match input.virtual_keycode {
@@ -180,7 +182,11 @@ fn run(
                                 stats.clear_min_and_max();
                             }
                             Some(VirtualKeyCode::M) => {
-                                aa_config_ix = (aa_config_ix + 1) % AA_CONFIGS.len();
+                                aa_config_ix = if modifiers.shift() {
+                                    aa_config_ix.saturating_sub(1)
+                                } else {
+                                    aa_config_ix.saturating_add(1)
+                                };
                             }
                             Some(VirtualKeyCode::P) => {
                                 if let Some(renderer) = &renderers[render_state.surface.dev_id] {
@@ -333,27 +339,15 @@ fn run(
 
             // Allow looping forever
             scene_ix = scene_ix.rem_euclid(scenes.scenes.len() as i32);
+            aa_config_ix = aa_config_ix.rem_euclid(AA_CONFIGS.len() as i32);
+
             let example_scene = &mut scenes.scenes[scene_ix as usize];
-            let mut reset_title = false;
             if prev_scene_ix != scene_ix {
                 transform = Affine::IDENTITY;
                 prev_scene_ix = scene_ix;
-                reset_title = true;
-            }
-            if prev_aa_config_ix != aa_config_ix {
-                prev_aa_config_ix = aa_config_ix;
-                reset_title = true;
-            }
-            if reset_title {
-                let aa_str = match AA_CONFIGS[aa_config_ix] {
-                    AaConfig::Area => "Analytic Area",
-                    AaConfig::Msaa16 => "16xMSAA",
-                    AaConfig::Msaa8 => "8xMSAA",
-                };
-                render_state.window.set_title(&format!(
-                    "Vello demo - {} - AA method: {}",
-                    example_scene.config.name, aa_str
-                ));
+                render_state
+                    .window
+                    .set_title(&format!("Vello demo - {}", example_scene.config.name));
             }
             let mut builder = SceneBuilder::for_fragment(&mut fragment);
             let mut scene_params = SceneParams {
@@ -371,6 +365,7 @@ fn run(
 
             // If the user specifies a base color in the CLI we use that. Otherwise we use any
             // color specified by the scene. The default is black.
+            let aa_config = AA_CONFIGS[aa_config_ix as usize];
             let render_params = vello::RenderParams {
                 base_color: args
                     .args
@@ -379,7 +374,7 @@ fn run(
                     .unwrap_or(Color::BLACK),
                 width,
                 height,
-                antialiasing_method: AA_CONFIGS[aa_config_ix],
+                antialiasing_method: aa_config,
             };
             let mut builder = SceneBuilder::for_scene(&mut scene);
             let mut transform = transform;
@@ -400,6 +395,7 @@ fn run(
                     stats.samples(),
                     complexity_shown.then_some(scene_complexity).flatten(),
                     vsync_on,
+                    aa_config,
                 );
                 if let Some(profiling_result) = renderers[render_state.surface.dev_id]
                     .as_mut()

examples/with_winit/src/stats.rs

@@ -19,7 +19,7 @@ use std::{collections::VecDeque, time::Duration};
 use vello::{
     kurbo::{Affine, Line, PathEl, Rect, Stroke},
     peniko::{Brush, Color, Fill},
-    BumpAllocators, SceneBuilder,
+    AaConfig, BumpAllocators, SceneBuilder,
 };
 use wgpu_profiler::GpuTimerScopeResult;
 
@@ -44,6 +44,7 @@ impl Snapshot {
         samples: T,
         bump: Option<BumpAllocators>,
         vsync: bool,
+        aa_config: AaConfig,
     ) where
         T: Iterator<Item = &'a u64>,
     {
@@ -67,6 +68,14 @@ impl Snapshot {
             format!("Frame Time (min): {:.2} ms", self.frame_time_min_ms),
             format!("Frame Time (max): {:.2} ms", self.frame_time_max_ms),
             format!("VSync: {}", if vsync { "on" } else { "off" }),
+            format!(
+                "AA method: {}",
+                match aa_config {
+                    AaConfig::Area => "Analytic Area",
+                    AaConfig::Msaa16 => "16xMSAA",
+                    AaConfig::Msaa8 => "8xMSAA",
+                }
+            ),
             format!("Resolution: {viewport_width}x{viewport_height}"),
         ];
         if let Some(bump) = &bump {

src/lib.rs

@@ -110,8 +110,8 @@ pub struct RendererOptions {
     pub timestamp_period: f32,
 
     /// If true, run all stages up to fine rasterization on the CPU.
-    /// TODO: Consider evolving this so that the CPU stages can be configured dynamically via
-    /// `RenderParams`.
+    // TODO: Consider evolving this so that the CPU stages can be configured dynamically via
+    // `RenderParams`.
     pub use_cpu: bool,
 
     /// The anti-aliasing specialization that should be used when creating the pipelines. `None`

src/render.rs

@@ -420,7 +420,9 @@ impl Render {
         match fine.aa_config {
             AaConfig::Area => {
                 recording.dispatch(
-                    shaders.fine_area.expect("unsupported AA mode: area"),
+                    shaders
+                        .fine_area
+                        .expect("shaders not configured to support AA mode: area"),
                     fine_wg_count,
                     [
                         fine.config_buf,
@@ -443,13 +445,17 @@ impl Render {
                     let buf = recording.upload("mask lut", mask_lut);
                     self.mask_buf = Some(buf.into());
                 }
-                let shader = match fine.aa_config {
-                    AaConfig::Msaa16 => shaders.fine_msaa16.expect("unsupported AA mode: msaa16"),
-                    AaConfig::Msaa8 => shaders.fine_msaa8.expect("unsupported AA mode: msaa8"),
+                let fine_shader = match fine.aa_config {
+                    AaConfig::Msaa16 => shaders
+                        .fine_msaa16
+                        .expect("shaders not configured to support AA mode: msaa16"),
+                    AaConfig::Msaa8 => shaders
+                        .fine_msaa8
+                        .expect("shaders not configured to support AA mode: msaa8"),
                     _ => unreachable!(),
                 };
                 recording.dispatch(
-                    shader,
+                    fine_shader,
                     fine_wg_count,
                     [
                         fine.config_buf,

src/shaders.rs

@@ -311,23 +311,21 @@ pub fn full_shaders(
         // Mask LUT buffer, used only when MSAA is enabled.
         BindType::BufReadOnly,
     ];
-    let fine_variants = {
-        const AA_MODES: [AaConfig; 3] = [AaConfig::Area, AaConfig::Msaa8, AaConfig::Msaa16];
-        const MSAA_INFO: [Option<(&str, &str)>; 3] = [
-            None,
-            Some(("fine_msaa8", "msaa8")),
-            Some(("fine_msaa16", "msaa16")),
+    let [fine_area, fine_msaa8, fine_msaa16] = {
+        const AA_MODES: [(AaConfig, Option<(&str, &str)>); 3] = [
+            (AaConfig::Area, None),
+            (AaConfig::Msaa8, Some(("fine_msaa8", "msaa8"))),
+            (AaConfig::Msaa16, Some(("fine_msaa16", "msaa16"))),
         ];
         let mut pipelines = [None, None, None];
-        for (i, aa_mode) in AA_MODES.iter().enumerate() {
+        for (i, (aa_mode, msaa_info)) in AA_MODES.iter().enumerate() {
             if options
                 .preferred_antialiasing_method
-                .as_ref()
-                .map_or(false, |m| m != aa_mode)
+                .map_or(false, |m| m != *aa_mode)
             {
                 continue;
             }
-            let (range_end_offset, label, aa_config) = match MSAA_INFO[i] {
+            let (range_end_offset, label, aa_config) = match *msaa_info {
                 Some((label, config)) => (0, label, Some(config)),
                 None => (1, "fine_area", None),
             };
@@ -365,9 +363,9 @@ pub fn full_shaders(
         coarse,
         path_tiling_setup,
         path_tiling,
-        fine_area: fine_variants[0],
-        fine_msaa8: fine_variants[1],
-        fine_msaa16: fine_variants[2],
+        fine_area,
+        fine_msaa8,
+        fine_msaa16,
         pathtag_is_cpu: false,
     })
 }