Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when changing viewport settings #4862

Merged
merged 4 commits into from
Aug 26, 2024
Merged

Fix crash when changing viewport settings #4862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1cd39cf
init fix
pm100 Jul 22, 2024
c1e9bfc
fix accidental delete of line
pm100 Jul 22, 2024
858a08d
Merge branch 'emilk:master' into fixwindestroy
pm100 Jul 31, 2024
645f061
fix wgpu recreate vp bug
pm100 Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions crates/eframe/src/native/epi_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,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;
}
}

Comment on lines +812 to +823
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need to add the same code to wgpu_integration.rs.

We just merged the update to latest winit (#4849), so please merge that into this branch and then test this PR with:

  • cargo r -p test_viewports
  • cargo r -p test_viewports -F wgpu
  • cargo r -p serial_windows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do, note that I found this #4894

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilk Am I reading this correctly. That clicking the close box on a child viewport does nothing unless I actually have code that checks for CloseRequested? And that you have to set the vp as not visible in order to destroy it . Thats not at all obvious. (Was trying to work out why the bug reporters app was not closing the child VP after I fixed the bugs I could find)

Copy link
Contributor Author

@pm100 pm100 Aug 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilk I added to wgpu the equivalent release of the drawing surface, in wgpu not releasing this resulted in the recreate of the vp leaving the original in place (rather than crashing as in glow).

Tested on windows and mac,

Note the wgpu has very strange defferred vp close behavior (also meaning that I did not need to add any close logic to wpgpu_integration). Because there is no close option only the option to no longer request its presence there is some very odd stuff happening. I worried that it was my bad but the current master shows the same thing. After CloseRequest is received you have to stop drawing secondary vp in update, BUT, this means you need another main window draw before the window disappears. I filed a bug on this #4894 . I tried to add the necessary code to the original reports code but could not - I thought it was simply a matter of doing request_repaint but I could not make that work. And the fact that the test_viewports app shows the same bug shows that its not simple.

I think it would be really ice to have an explicit vp.close call (even though that goes again the immediate model VPs are really not transient, the OS knows them (unlike stuff drawn on the gl surfaces). I will investigate and make a separate PR

Also I have not tested on linux becasue I cannot make it build on linux, it complains about a lot of pkg errors.

I will do a separate PR to add vp calls the need window recreation to test_viewports. Done #4907

In summary - I think this PR is complete, apart from not being tested on Linux.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

_ => {}
}

Expand Down Expand Up @@ -1365,6 +1377,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
View file
Open in desktop
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 @@ -929,8 +930,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 @@ -1053,7 +1060,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 @@ -1086,13 +1093,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 @@ -1137,6 +1145,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
Loading