Fix crash when changing viewport settings (#4862)

pm100 · web-flow · commit 0c528fb862d0 · 2024-08-26T08:55:34.000+02:00
* Fixes #3959 There are two bugs racing each other here, which is why it sometimes crashes and sometimes the app just silently exists Bug 1 When the window is recreated a Destroyed event arrives (due to the Drop of the old window). The code that receives this event does not look to see if its the main viewport or a secondary one and unconditionally closes the app. The code path for other platforms is slightly different and does check. I have moved the code that handles the destroy to be in the same place and have the same behavior as the other platforms. Bug 2 At recreate time the window and winit entries of the viewport are set to None (forcin g them to be recreated). But the surface is still bound to the old window, this causes the next context switch to fail. So I simply added a viewport.gl_surface = None too, This is my first egui PR so I hope I have not broken anything. If nothing else I understand a little better how egui works.
diff --git a/crates/eframe/src/native/epi_integration.rs b/crates/eframe/src/native/epi_integration.rs
@@ -236,10 +236,6 @@ impl EpiIntegration {
         use winit::event::{ElementState, MouseButton, WindowEvent};
 
         match event {
-            WindowEvent::Destroyed => {
-                log::debug!("Received WindowEvent::Destroyed");
-                self.close = true;
-            }
             WindowEvent::MouseInput {
                 button: MouseButton::Left,
                 state: ElementState::Pressed,
diff --git a/crates/eframe/src/native/glow_integration.rs b/crates/eframe/src/native/glow_integration.rs
@@ -803,6 +803,18 @@ impl GlowWinitRunning {
                 }
             }
 
+            winit::event::WindowEvent::Destroyed => {
+                log::debug!(
+                    "Received WindowEvent::Destroyed for viewport {:?}",
+                    viewport_id
+                );
+                if viewport_id == Some(ViewportId::ROOT) {
+                    return EventResult::Exit;
+                } else {
+                    return EventResult::Wait;
+                }
+            }
+
             _ => {}
         }
 
@@ -1360,6 +1372,7 @@ fn initialize_or_update_viewport(
                 );
                 viewport.window = None;
                 viewport.egui_winit = None;
+                viewport.gl_surface = None;
             }
 
             viewport.deferred_commands.append(&mut delta_commands);
diff --git a/crates/eframe/src/native/wgpu_integration.rs b/crates/eframe/src/native/wgpu_integration.rs
@@ -158,6 +158,7 @@ impl WgpuWinitApp {
             ViewportClass::Root,
             self.native_options.viewport.clone(),
             None,
+            painter,
         )
         .initialize_window(event_loop, egui_ctx, viewport_from_window, painter);
     }
@@ -927,8 +928,14 @@ fn render_immediate_viewport(
             ..
         } = &mut *shared.borrow_mut();
 
-        let viewport =
-            initialize_or_update_viewport(viewports, ids, ViewportClass::Immediate, builder, None);
+        let viewport = initialize_or_update_viewport(
+            viewports,
+            ids,
+            ViewportClass::Immediate,
+            builder,
+            None,
+            painter,
+        );
         if viewport.window.is_none() {
             event_loop_context::with_current_event_loop(|event_loop| {
                 viewport.initialize_window(event_loop, egui_ctx, viewport_from_window, painter);
@@ -1051,7 +1058,7 @@ fn handle_viewport_output(
         let ids = ViewportIdPair::from_self_and_parent(viewport_id, parent);
 
         let viewport =
-            initialize_or_update_viewport(viewports, ids, class, builder, viewport_ui_cb);
+            initialize_or_update_viewport(viewports, ids, class, builder, viewport_ui_cb, painter);
 
         if let Some(window) = viewport.window.as_ref() {
             let old_inner_size = window.inner_size();
@@ -1084,13 +1091,14 @@ fn handle_viewport_output(
     remove_viewports_not_in(viewports, painter, viewport_from_window, viewport_output);
 }
 
-fn initialize_or_update_viewport(
-    viewports: &mut Viewports,
+fn initialize_or_update_viewport<'a>(
+    viewports: &'a mut Viewports,
     ids: ViewportIdPair,
     class: ViewportClass,
     mut builder: ViewportBuilder,
     viewport_ui_cb: Option<Arc<dyn Fn(&egui::Context) + Send + Sync>>,
-) -> &mut Viewport {
+    painter: &mut egui_wgpu::winit::Painter,
+) -> &'a mut Viewport {
     crate::profile_function!();
 
     if builder.icon.is_none() {
@@ -1135,6 +1143,12 @@ fn initialize_or_update_viewport(
                 );
                 viewport.window = None;
                 viewport.egui_winit = None;
+                if let Err(err) = pollster::block_on(painter.set_window(viewport.ids.this, None)) {
+                    log::error!(
+                        "when rendering viewport_id={:?}, set_window Error {err}",
+                        viewport.ids.this
+                    );
+                }
             }
 
             viewport.deferred_commands.append(&mut delta_commands);

Original file line number	Diff line number	Diff line change
`@@ -803,6 +803,18 @@ impl GlowWinitRunning {`
`803`	`803`	`}`
`804`	`804`	`}`
`805`	`805`
	`806`	`+ winit::event::WindowEvent::Destroyed => {`
	`807`	`+ log::debug!(`
	`808`	`+ "Received WindowEvent::Destroyed for viewport {:?}",`
	`809`	`+ viewport_id`
	`810`	`+ );`
	`811`	`+ if viewport_id == Some(ViewportId::ROOT) {`
	`812`	`+ return EventResult::Exit;`
	`813`	`+ } else {`
	`814`	`+ return EventResult::Wait;`
	`815`	`+ }`
	`816`	`+ }`
	`817`	`+`
`806`	`818`	`_ => {}`
`807`	`819`	`}`
`808`	`820`
`@@ -1360,6 +1372,7 @@ fn initialize_or_update_viewport(`
`1360`	`1372`	`);`
`1361`	`1373`	`viewport.window = None;`
`1362`	`1374`	`viewport.egui_winit = None;`
	`1375`	`+ viewport.gl_surface = None;`
`1363`	`1376`	`}`
`1364`	`1377`
`1365`	`1378`	`viewport.deferred_commands.append(&mut delta_commands);`