Skip to content

Commit

Permalink
Fix crash when changing viewport settings (#4862)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
pm100 authored Aug 26, 2024
1 parent 5a196f6 commit 0c528fb
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 10 deletions.
4 changes: 0 additions & 4 deletions crates/eframe/src/native/epi_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
13 changes: 13 additions & 0 deletions crates/eframe/src/native/glow_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

_ => {}
}

Expand Down Expand Up @@ -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);
Expand Down
26 changes: 20 additions & 6 deletions crates/eframe/src/native/wgpu_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 0c528fb

Please sign in to comment.