Properly wait to request buffer mapping until after the command to copy the screenshot texture to the buffer has been submitted.

CHANGELOG.md

@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Removed
 
 ### Fixed
+- Fixed bug that would sometimes cause taking a screenshot to panic because a buffer was mapped a the wrong time.
 
 ## [0.12.0] - 2022-02-19
 

voxygen/src/render/renderer/drawer.rs

@@ -553,42 +553,48 @@ impl<'frame> Drop for Drawer<'frame> {
         // If taking a screenshot and the blit pipeline is available
         // NOTE: blit pipeline should always be available for now so we don't report an
         // error if it isn't
-        if let Some((screenshot, blit)) = self
+        let download_and_handle_screenshot = self
             .taking_screenshot
             .take()
             .zip(self.borrow.pipelines.blit())
-        {
+            .map(|(screenshot, blit)| {
-            // Image needs to be copied from the screenshot texture to the swapchain texture
+                // Image needs to be copied from the screenshot texture to the swapchain texture
-            let mut render_pass = encoder.scoped_render_pass(
+                let mut render_pass = encoder.scoped_render_pass(
-                "screenshot blit",
+                    "screenshot blit",
-                self.borrow.device,
+                    self.borrow.device,
-                &wgpu::RenderPassDescriptor {
+                    &wgpu::RenderPassDescriptor {
-                    label: Some("Blit screenshot pass"),
+                        label: Some("Blit screenshot pass"),
-                    color_attachments: &[wgpu::RenderPassColorAttachment {
+                        color_attachments: &[wgpu::RenderPassColorAttachment {
-                        view: &self.swap_tex.view,
+                            view: &self.swap_tex.view,
-                        resolve_target: None,
+                            resolve_target: None,
-                        ops: wgpu::Operations {
+                            ops: wgpu::Operations {
-                            load: wgpu::LoadOp::Clear(wgpu::Color::TRANSPARENT),
+                                load: wgpu::LoadOp::Clear(wgpu::Color::TRANSPARENT),
-                            store: true,
+                                store: true,
-                        },
+                            },
-                    }],
+                        }],
-                    depth_stencil_attachment: None,
+                        depth_stencil_attachment: None,
-                },
+                    },
-            );
+                );
-            render_pass.set_pipeline(&blit.pipeline);
+                render_pass.set_pipeline(&blit.pipeline);
-            render_pass.set_bind_group(0, screenshot.bind_group(), &[]);
+                render_pass.set_bind_group(0, screenshot.bind_group(), &[]);
-            render_pass.draw(0..3, 0..1);
+                render_pass.draw(0..3, 0..1);
-            drop(render_pass);
+                drop(render_pass);
-            // Issues a command to copy from the texture to a buffer and then sends the
+                // Issues a command to copy from the texture to a buffer and returns a closure
-            // buffer off to another thread to be mapped and processed
+                // that will send the buffer off to another thread to be mapped
-            screenshot.download_and_handle(&mut encoder);
+                // and processed.
-        }
+                screenshot.copy_to_buffer(&mut encoder)
+            });
 
         let (mut encoder, profiler) = encoder.end_scope();
         profiler.resolve_queries(&mut encoder);
 
         // It is recommended to only do one submit per frame
         self.borrow.queue.submit(std::iter::once(encoder.finish()));
+        // Need to call this after submit so the async mapping doesn't occur before
+        // copying the screenshot to the buffer which will be mapped.
+        if let Some(f) = download_and_handle_screenshot {
+            f();
+        }
 
         profiler
             .end_frame()

voxygen/src/render/renderer/screenshot.rs

@@ -88,9 +88,13 @@ impl TakeScreenshot {
     /// swapchain image
     pub fn bind_group(&self) -> &wgpu::BindGroup { &self.bind_group.bind_group }
 
-    /// NOTE: spawns thread
     /// Call this after rendering to the screenshot texture
-    pub fn download_and_handle(self, encoder: &mut wgpu::CommandEncoder) {
+    ///
+    /// Issues a command to copy from the texture to a buffer and returns a
+    /// closure that needs to be called after submitting the encoder
+    /// to the queue. When called, the closure will spawn a new thread for
+    /// async mapping of the buffer and downloading of the screenshot.
+    pub fn copy_to_buffer(self, encoder: &mut wgpu::CommandEncoder) -> impl FnOnce() {
         // Calculate padded bytes per row
         let padded_bytes_per_row = padded_bytes_per_row(self.width, self.bytes_per_pixel);
         // Copy image to a buffer
@@ -114,14 +118,17 @@ impl TakeScreenshot {
                 depth_or_array_layers: 1,
             },
         );
-        // Send buffer to another thread for async mapping, downloading, and passing to
+
-        // the given handler function (which probably saves it to the disk)
+        move || {
-        std::thread::Builder::new()
+            // Send buffer to another thread for async mapping, downloading, and passing to
-            .name("screenshot".into())
+            // the given handler function (which probably saves it to the disk)
-            .spawn(move || {
+            std::thread::Builder::new()
-                self.download_and_handle_internal();
+                .name("screenshot".into())
-            })
+                .spawn(move || {
-            .expect("Failed to spawn screenshot thread");
+                    self.download_and_handle_internal();
+                })
+                .expect("Failed to spawn screenshot thread");
+        }
     }
 
     fn download_and_handle_internal(self) {