Merge pull request #1210 from hannobraun/egui

Remove winit dependency from `fj-viewer`

Cargo.lock

@@ -853,11 +853,9 @@ checksum = "f12a8d8f92a429e2e9399003bc01d48f54aefa6fec126cbd3462e313c0ee0e91"
 dependencies = [
  "bytemuck",
  "egui",
- "pollster",
  "tracing",
  "type-map",
  "wgpu",
- "winit",
 ]
 
 [[package]]
@@ -1116,7 +1114,6 @@ dependencies = [
  "bytemuck",
  "egui",
  "egui-wgpu",
- "egui-winit",
  "fj-interop",
  "fj-math",
  "raw-window-handle 0.4.3",
@@ -1130,6 +1127,7 @@ dependencies = [
 name = "fj-window"
 version = "0.19.0"
 dependencies = [
+ "egui-winit",
  "fj-host",
  "fj-interop",
  "fj-operations",
@@ -2603,12 +2601,6 @@ dependencies = [
  "miniz_oxide",
 ]
 
-[[package]]
-name = "pollster"
-version = "0.2.5"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5da3b0203fd7ee5720aa0b5e790b591aa5d3f41c3ed2c34a3a393382198af2f7"
-
 [[package]]
 name = "ppv-lite86"
 version = "0.2.16"

crates/fj-viewer/Cargo.toml

@@ -13,7 +13,7 @@ categories.workspace = true
 [dependencies]
 bytemuck = "1.12.1"
 egui = "0.19.0"
-egui-winit = "0.19.0"
+egui-wgpu = "0.19.0"
 fj-interop.workspace = true
 fj-math.workspace = true
 raw-window-handle = "0.4.3"
@@ -21,7 +21,3 @@ thiserror = "1.0.35"
 tracing = "0.1.37"
 wgpu = "0.13.1"
 wgpu_glyph = "0.17.0"
-
-[dependencies.egui-wgpu]
-version = "0.19.0"
-features = ["winit"]

crates/fj-viewer/src/graphics/renderer.rs

@@ -1,6 +1,5 @@
 use std::{io, mem::size_of};
 
-use egui_winit::winit::event_loop::EventLoop;
 use fj_interop::status_report::StatusReport;
 use fj_math::{Aabb, Point};
 use thiserror::Error;
@@ -10,6 +9,7 @@ use wgpu_glyph::ab_glyph::InvalidFont;
 
 use crate::{
     camera::Camera,
+    gui::EguiState,
     screen::{Screen, Size},
 };
 
@@ -19,28 +19,6 @@ use super::{
     vertices::Vertices, DEPTH_FORMAT,
 };
 
-#[derive(Default)]
-struct EguiOptionsState {
-    show_trace: bool,
-    show_layout_debug_on_hover: bool,
-    show_debug_text_example: bool,
-    show_settings_ui: bool,
-    show_inspection_ui: bool,
-}
-
-pub struct EguiState {
-    pub winit_state: egui_winit::State,
-    pub context: egui::Context,
-    rpass: egui_wgpu::renderer::RenderPass,
-    options: EguiOptionsState,
-}
-
-impl std::fmt::Debug for EguiState {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.write_str("EguiState {}")
-    }
-}
-
 /// Graphics rendering state and target abstraction
 #[derive(Debug)]
 pub struct Renderer {
@@ -64,65 +42,9 @@ pub struct Renderer {
 
 impl Renderer {
     /// Returns a new `Renderer`.
-    pub async fn new(
+    pub async fn new(screen: &impl Screen) -> Result<Self, InitError> {
-        screen: &impl Screen<Window = egui_winit::winit::window::Window>,
-        event_loop: &EventLoop<()>,
-    ) -> Result<Self, InitError> {
         let instance = wgpu::Instance::new(wgpu::Backends::PRIMARY);
 
-        //
-        // NOTE: The implementation of the integration with `egui` is
-        //       likely to need to change "significantly"[0] depending
-        //       on what architecture approach is chosen going
-        //       forward.
-        //
-        //       The current implementation is somewhat complicated by
-        //       virtue of "sitting somewhere in the middle" in
-        //       relation to being neither a standalone integration
-        //       nor fully using `egui` as a framework.
-        //
-        //       This is a result of a combination of the current
-        //       integration being "proof of concept" level; and, using
-        //       `egui-winit` & `egui-wgpu` which are both relatively
-        //       new additions to the core `egui` ecosystem.
-        //
-        //       It is recommended to read the following for additional
-        //       helpful context for choosing an architecture:
-        //
-        //         * <https://github.com/emilk/egui/blob/eeae485629fca24a81a7251739460b671e1420f7/README.md#what-is-the-difference-between-egui-and-eframe>
-        //
-        //         * <https://github.com/emilk/egui/blob/eeae485629fca24a81a7251739460b671e1420f7/README.md#how-do-i-render-3d-stuff-in-an-egui-area>
-        //
-        //       [0] By way of specific example, the recent addition
-        //           of Android support lead to considerable API
-        //           change related to `wgpu` & `winit`, see:
-        //
-        //             * <https://github.com/emilk/egui/commit/a5076d4cc491536b07b16dced1772c7b6bf7cc29>
-        //
-
-        //
-        // NOTE: If at some point you use `Painter` or similar and you
-        //       get this error:
-        //
-        //         `VK_ERROR_NATIVE_WINDOW_IN_USE_KHR`
-        //
-        //       and/or:
-        //
-        //         `wgpu_core::device: surface configuration failed: Native window is in use`
-        //
-        //       it's *probably(?)* because the swapchain has already
-        //       been created for the window (e.g. by an integration)
-        //       and *not* because of a regression of this issue
-        //       (probably):
-        //
-        //         <https://github.com/gfx-rs/wgpu/issues/1492>
-        //
-        //       Don't ask me how I know.
-        //
-
-        let egui_winit_state = egui_winit::State::new(event_loop);
-        let egui_context = egui::Context::default();
-
         // This is sound, as `window` is an object to create a surface upon.
         let surface = unsafe { instance.create_surface(screen.window()) };
 
@@ -140,10 +62,10 @@ impl Renderer {
             let available_features = adapter.features();
 
             // By requesting the intersection of desired and available features,
-            // we ensure two things:
+            // we prevent two things:
             //
-            // 1. That requesting the device doesn't panic, which would happen
+            // 1. That requesting the device panics, which would happen if we
-            //    if we requested unavailable features.
+            //    requested unavailable features.
             // 2. That a developer ends up accidentally using features that
             //    happen to be available on their machine, but that aren't
             //    necessarily available for all the users.
@@ -229,23 +151,7 @@ impl Renderer {
         let pipelines =
             Pipelines::new(&device, &bind_group_layout, color_format);
 
-        //
+        let egui = EguiState::new(&device, surface_config.format);
-        // Note: We need to hold on to this otherwise (from my memory)
-        //       it causes the egui font texture to get dropped after
-        //       drawing one frame.
-        //
-        //       This then results in an `egui_wgpu_backend` error of
-        //       `BackendError::Internal` with message:
-        //
-        //           "Texture 0 used but not live"
-        //
-        //       See also: <https://github.com/hasenbanck/egui_wgpu_backend/blob/b2d3e7967351690c6425f37cd6d4ffb083a7e8e6/src/lib.rs#L373>
-        //
-        let egui_rpass = egui_wgpu::renderer::RenderPass::new(
-            &device,
-            surface_config.format,
-            1,
-        );
 
         Ok(Self {
             surface,
@@ -262,12 +168,7 @@ impl Renderer {
             geometries,
             pipelines,
 
-            egui: EguiState {
+            egui,
-                context: egui_context,
-                winit_state: egui_winit_state,
-                rpass: egui_rpass,
-                options: Default::default(),
-            },
         })
     }
 
@@ -301,8 +202,9 @@ impl Renderer {
         &mut self,
         camera: &Camera,
         config: &mut DrawConfig,
-        window: &egui_winit::winit::window::Window,
+        scale_factor: f32,
         status: &mut StatusReport,
+        egui_input: egui::RawInput,
     ) -> Result<(), DrawError> {
         let aspect_ratio = self.surface_config.width as f64
             / self.surface_config.height as f64;
@@ -373,42 +275,6 @@ impl Renderer {
             }
         }
 
-        //
-        // NOTE: The following comment was written for the original
-        //       proof-of-concept which targeted older versions of
-        //       Fornjot & `egui`, so some details may be outdated &
-        //       not entirely apply to this updated implementation.
-        //
-        //       It's included here in case it still provides some
-        //       useful context.
-        //
-        //
-        // This integration is basically the result of locating the
-        // `.present()` call in the `egui` example, here:
-        //
-        //     <https://github.com/hasenbanck/egui_example/blob/ca1262a701daf0b20e097ef627fc301ab63339d9/src/main.rs#L177>
-        //
-        // and then the equivalent call in `renderer.rs`, here:
-        //
-        //     <https://github.com/hannobraun/Fornjot/blob/15294c2ca2fa5ac5016bb29853943b28952f2dae/fj-app/src/graphics/renderer.rs#L245>
-        //
-        // Then working backwards from there to merge the functionality.
-        //
-        // In addition, the following examples were also referenced:
-        //
-        //  * "Make the example more like an actual use case #17"
-        //    <https://github.com/hasenbanck/egui_example/pull/17/files>
-        //    This removes some non-essential code from the example
-        //    which helps clarify what's *actually* necessary.
-        //
-        //  * "Update to 0.17, use official winit backend #18"
-        //    <https://github.com/hasenbanck/egui_example/pull/18/files>
-        //    This uses a more up-to-date `egui` version which
-        //    included some API changes.
-        //    It's still not the *latest* `egui` version though.
-        //
-
-        let egui_input = self.egui.winit_state.take_egui_input(window);
         self.egui.context.begin_frame(egui_input);
 
         fn get_bbox_size_text(aabb: &Aabb<3>) -> String {
@@ -582,7 +448,7 @@ impl Renderer {
             // Note: `scale_factor` can be overridden via `WINIT_X11_SCALE_FACTOR` environment variable,
             //       see: <https://docs.rs/winit/0.26.1/winit/window/struct.Window.html#method.scale_factor>
             //
-            window.scale_factor() as f32,
+            scale_factor,
             egui::Rgba::TRANSPARENT,
             &egui_paint_jobs,
             &egui_output.textures_delta,
@@ -737,7 +603,7 @@ impl Renderer {
         };
 
         for (id, image_delta) in &textures_delta.set {
-            self.egui.rpass.update_texture(
+            self.egui.render_pass.update_texture(
                 &self.device,
                 &self.queue,
                 *id,
@@ -745,10 +611,10 @@ impl Renderer {
             );
         }
         for id in &textures_delta.free {
-            self.egui.rpass.free_texture(id);
+            self.egui.render_pass.free_texture(id);
         }
 
-        self.egui.rpass.update_buffers(
+        self.egui.render_pass.update_buffers(
             &self.device,
             &self.queue,
             clipped_primitives,
@@ -779,7 +645,7 @@ impl Renderer {
         };
 
         // Record all render passes.
-        self.egui.rpass.execute(
+        self.egui.render_pass.execute(
             encoder,
             output_view,
             clipped_primitives,

crates/fj-viewer/src/gui.rs

@@ -0,0 +1,83 @@
+//! GUI-related code
+//!
+//! If at some point you use `Painter` or similar and you get this error:
+//!
+//! `VK_ERROR_NATIVE_WINDOW_IN_USE_KHR`
+//!
+//! and/or:
+//!
+//! `wgpu_core::device: surface configuration failed: Native window is in use`
+//!
+//! it's *probably(?)* because the swap chain has already been created for the
+//! window (e.g. by an integration) and *not* because of a regression of this
+//! issue (probably):
+//!
+//! <https://github.com/gfx-rs/wgpu/issues/1492>
+
+#[derive(Default)]
+pub struct EguiOptionsState {
+    pub show_trace: bool,
+    pub show_layout_debug_on_hover: bool,
+    pub show_debug_text_example: bool,
+    pub show_settings_ui: bool,
+    pub show_inspection_ui: bool,
+}
+
+pub struct EguiState {
+    pub context: egui::Context,
+    pub render_pass: egui_wgpu::renderer::RenderPass,
+    pub options: EguiOptionsState,
+}
+
+impl EguiState {
+    pub fn new(
+        device: &wgpu::Device,
+        texture_format: wgpu::TextureFormat,
+    ) -> Self {
+        // The implementation of the integration with `egui` is likely to need
+        // to change "significantly" depending on what architecture approach is
+        // chosen going forward.
+        //
+        // The current implementation is somewhat complicated by virtue of
+        // "sitting somewhere in the middle" in relation to being neither a
+        // standalone integration nor fully using `egui` as a framework.
+        //
+        // This is a result of a combination of the current integration being
+        // "proof of concept" level, and using `egui-winit` & `egui-wgpu`, which
+        // are both relatively new additions to the core `egui` ecosystem.
+        //
+        // It is recommended to read the following for additional helpful
+        // context for choosing an architecture:
+        //
+        // - https://github.com/emilk/egui/blob/eeae485629fca24a81a7251739460b671e1420f7/README.md#what-is-the-difference-between-egui-and-eframe
+        // - https://github.com/emilk/egui/blob/eeae485629fca24a81a7251739460b671e1420f7/README.md#how-do-i-render-3d-stuff-in-an-egui-area
+
+        let context = egui::Context::default();
+
+        // We need to hold on to this, otherwise it might cause the egui font
+        // texture to get dropped after drawing one frame.
+        //
+        // This then results in an `egui_wgpu_backend` error of
+        // `BackendError::Internal` with message:
+        //
+        // ```
+        // Texture 0 used but not live
+        // ```
+        //
+        // See also: <https://github.com/hasenbanck/egui_wgpu_backend/blob/b2d3e7967351690c6425f37cd6d4ffb083a7e8e6/src/lib.rs#L373>
+        let render_pass =
+            egui_wgpu::renderer::RenderPass::new(device, texture_format, 1);
+
+        Self {
+            context,
+            render_pass,
+            options: Default::default(),
+        }
+    }
+}
+
+impl std::fmt::Debug for EguiState {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str("EguiState {}")
+    }
+}

crates/fj-viewer/src/lib.rs

@@ -18,3 +18,5 @@ pub mod camera;
 pub mod graphics;
 pub mod input;
 pub mod screen;
+
+mod gui;

crates/fj-window/Cargo.toml

@@ -15,6 +15,7 @@ fj-host.workspace = true
 fj-operations.workspace = true
 fj-viewer.workspace = true
 fj-interop.workspace = true
+egui-winit = "0.19.0"
 futures = "0.3.24"
 thiserror = "1.0.35"
 tracing = "0.1.37"

crates/fj-window/src/run.rs

@@ -42,7 +42,8 @@ pub fn run(
     let mut focus_point = None;
 
     let mut input_handler = input::Handler::default();
-    let mut renderer = block_on(Renderer::new(&window, &event_loop))?;
+    let mut renderer = block_on(Renderer::new(&window))?;
+    let mut egui_winit_state = egui_winit::State::new(&event_loop);
 
     let mut draw_config = DrawConfig::default();
 
@@ -96,26 +97,15 @@ pub fn run(
             ..
         } = &event
         {
+            // In theory we could/should check if `egui` wants "exclusive" use
+            // of this event here. But with the current integration with Fornjot
+            // we're kinda blurring the lines between "app" and "platform", so
+            // for the moment we pass every event to both `egui` & Fornjot.
             //
-            // Note: In theory we could/should check if `egui` wants "exclusive" use
+            // The primary visible impact of this currently is that if you drag
-            //       of this event here.
+            // a title bar that overlaps the model then both the model & window
-            //
+            // get moved.
-            //       But with the current integration with Fornjot we're kinda blurring
+            egui_winit_state.on_event(&renderer.egui.context, window_event);
-            //       the lines between "app" and "platform", so for the moment we pass
-            //       every event to both `egui` & Fornjot.
-            //
-            //       The primary visible impact of this currently is that if you drag
-            //       a title bar that overlaps the model then both the model & window
-            //       get moved.
-            //
-            // TODO: Revisit this.
-            //
-            // TODO: Encapsulate the egui state/context access better.
-            //
-            renderer
-                .egui
-                .winit_state
-                .on_event(&renderer.egui.context, window_event);
         }
 
         // fj-window events
@@ -182,11 +172,15 @@ pub fn run(
                     camera.update_planes(&shape.aabb);
                 }
 
+                let egui_input =
+                    egui_winit_state.take_egui_input(window.window());
+
                 if let Err(err) = renderer.draw(
                     &camera,
                     &mut draw_config,
-                    window.window(),
+                    window.window().scale_factor() as f32,
                     &mut status,
+                    egui_input,
                 ) {
                     warn!("Draw error: {}", err);
                 }