Add egui_glow backend as alternative to egui_glium

[AlexApps99]

I have opened this draft PR to move discussion about egui_glow from #93 to keep conversation focused.
As of latest commit, this fork no longer builds. This is because of issues with lifetimes that I will need to fix.

Feel free to give feedback, but keep in mind that a lot of things are still subject to change.

TODOs:

• Get it working
• Use SRGB to match egui_glium
• Fix remaining lifetime issues
• Ensure there are no OpenGL resource leaks
• Detect OpenGL version for GLSL shaders
• Fix graphical glitch as shown in screenshot below (black rectangle in top left corner)
  
• Move egui_glow into its own directory, once reviewing is complete

Closes #93.

[AlexApps99]

I'm not sure if a lifetime restriction is the way to go, as glutin's event loop is 'static and that generates some problems:

error[E0597]: `gl` does not live long enough
   --> egui_glium/examples/pure.rs:38:55
    |
38  |     let mut egui = egui_glow::EguiGlow::new(&display, &gl);
    |                    -----------------------------------^^^-
    |                    |                                  |
    |                    |                                  borrowed value does not live long enough
    |                    argument requires that `gl` is borrowed for `'static`
...
113 | }
    | - `gl` dropped here while still borrowed

error[E0505]: cannot move out of `gl` because it is borrowed
  --> egui_glium/examples/pure.rs:40:20
   |
38 |     let mut egui = egui_glow::EguiGlow::new(&display, &gl);
   |                    ---------------------------------------
   |                    |                                  |
   |                    |                                  borrow of `gl` occurs here
   |                    argument requires that `gl` is borrowed for `'static`
39 |
40 |     event_loop.run(move |event, _, control_flow| {
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ move out of `gl` occurs here
...
79 |                     (&gl).clear_color(
   |                       -- move occurs due to use in closure

I don't have that much experience with lifetimes, and try to avoid them wherever possible - have I made any mistakes in adding them?

[ar37-rs]

hi @AlexApps99 forked from your master branch now it's working with no lifetime: https://github.com/Ar37-rs/egui_glow but the current the problem is screen positioning, i have no idea how to positioning the screen after mutated

[AlexApps99]

hi @AlexApps99 forked from your master branch now it's working with no lifetime: https://github.com/Ar37-rs/egui_glow but the current the problem is screen positioning, i have no idea how to positioning the screen after mutated

I had a quick look, I don't think this would work when people want to integrate egui with their existing rendering (as is the reason why I'm writing this in the first place).
EguiGlow can't take ownership of glow::Context because then the rendering code integrating egui wouldn't be able to own it.

There is a version of this that doesn't have lifetimes (see the commit before the latest one, it works and is how I got the screenshot), but the issue is that it's awkward to destruct OpenGL objects. I don't think that there will be any elegant way to hold glow::Context in any way that will not substantially break integration code, so I might just have to revert the commit and abandon the lifetime idea. @emilk your thoughts?

[ar37-rs]

@AlexApps99 honesty i don't much about opengl, but forgot to mention that if there's graphical glitches it's better to deconstruct the mesh.vertices to uv, pos and color fist before casting as_u8_slice rather than casting all of them directly, because each uv.x,y pos.x,y is f32 but each color is u8. or cast using bytemuck if glitches still appear.

[AlexApps99]

@AlexApps99 honesty i don't much about opengl, but forgot to mention that if there's graphical glitches it's better to deconstruct the mesh.vertices to uv, pos and color fist before casting as_u8_slice rather than casting all of them directly, because each uv.x,y pos.x,y is f32 but each color is u8. or cast using bytemuck if glitches still appear.

I doubt the cast is what is causing the error, I have tested it in Renderdoc and none of the vertex data seems to be amiss when compared to glow_glium's output. It's true I could use another, safe, method for converting into GPU format, but other methods would have to allocate, and as such would have a memory and performance cost. I think the graphical glitch is probably a problem with either my scissoring, or something else to do with OpenGL state. I haven't spent much time on debugging it yet as I want to finish the other aspects before I do that, but I am confident that I will be able to pin it down with Renderdoc once I start working on it.

[ar37-rs]

@AlexApps99 👍

[AlexApps99]

I fixed the weird box artifacts, and it was even easier than I would have thought. All I needed to do was to disable scissor testing before clearing the screen!
I'm still unsure on what to do about lifetimes vs no lifetimes, so I'm waiting for feedback from emilk. I'm currently working on a lifetime-less branch with the goal of no leaks, then I will port it over to lifetimes if that ends up being the desired method.

[AlexApps99]

I have made a lifetime-less version at AlexApps99/egui@nolife, I cannot 100% confirm there are no leaks but as far as I know, everything should be properly deleted. I made it panic on drop in debug mode if destruct() has not been called, to ensure people will use it properly, and I have fixed all remaining issues that I noticed.

Please give me feedback, so I can improve it.
I'm more than happy to change the structure, add more error checking, use lifetimes instead of having a destruct() method, or any other little thing.

[emilk]

I think the cleanest solution it to pass in gl: &glow::Context to all functions that need it. This means storing no references to it (no lifetime problems) and not storing glow::Context (which plays badly with people who already are using glow and want to add on egui support).

This means the users having to call a egui_glow.destroy(gl); to free up the textures used by egui, but that's the clean solution imho.

[emilk]

@AlexApps99 I've removed the http feature from all of egui_glium (and epi/eframe/egui_web): #697

Less code to duplicate between egui_glium and egui_glow!

[AlexApps99]

The only TODO I can think of is detecting the GLSL version, which shouldn't be too hard once I get round to implementing it. In the mean time, I'd be happy for people to try out the library and look through the code and give feedback. Hopefully I can implement GLSL detection by the end of today

[AlexApps99]

Opened the window with a very narrow size, is this a regression or can this happen with the glium backend too

Edit: It happens on glium too

[AlexApps99]

It should be possible to unify all shaders into a single file using preprocessor ifdefs for each version, as GLSL provides the GL_ES and __VERSION__ definitions. Some code would need to add the #version ... line, but the rest can be generalized. That means 2 total GLSL files can be shared between egui_web, egui_glium, and egui_glow, probably

[emilk]

I tried it just now on my Intel Mac (macOS Mojave 10.14.6) and got this:

4.10
4.10
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', egui_glium/src/painter.rs:165:83

The offending line:

let u_screen_size = gl.get_uniform_location(program, "u_screen_size").unwrap();

[AlexApps99]

I think macOS GL contexts need special stuff? I'll make a branch with GL debug prints for you to use so the bug can be identified

[AlexApps99]

Here it is, see what it says:
https://github.com/AlexApps99/egui/tree/debug

[emilk]

I get no extra debug printing from that, but adding this:

            if !gl.get_shader_compile_status(v) {
                panic!(
                    "Failed to compile vertex shader: {}",
                    gl.get_shader_info_log(v)
                );
            }

Gets me this:

Failed to compile vertex shader: ERROR: 0:2: '' : syntax error: incorrect preprocessor directive
ERROR: 0:2: '' : syntax error: unexpected tokens following #if preprocessor directive - expected a newline

So it seems my drivers can't handle #if !defined(GL_ES) && __VERSION >= 140 for whatever reason.

I'm on version 140 btw.

I need to go soon too, so can't be much more of a help today I'm afraid.

[AlexApps99]

I get no extra debug printing from that, but adding this:

            if !gl.get_shader_compile_status(v) {
                panic!(
                    "Failed to compile vertex shader: {}",
                    gl.get_shader_info_log(v)
                );
            }

Gets me this:

Failed to compile vertex shader: ERROR: 0:2: '' : syntax error: incorrect preprocessor directive
ERROR: 0:2: '' : syntax error: unexpected tokens following #if preprocessor directive - expected a newline

So it seems my drivers can't handle #if !defined(GL_ES) && __VERSION >= 140 for whatever reason.

I'm on version 140 btw.

I need to go soon too, so can't be much more of a help today I'm afraid.

Using __VERSION was a mistake, it should have been __VERSION__.
Since the error occurs at column 2 though, I imagine the ! might be invalid? I will take a look at the spec and see

From GLSL 1.20 spec

So I'm not sure what's amiss

[emilk]

0:2 means "file zero, line two", I believe. __VERSION__ fixed it for me, as in it now starts. But I get an empty window. (just a menu bar, and then everything else is transparent). Removing the call to .with_transparent(native_options.transparent) gives me a black window instead. :/

[emilk]

FWIW, https://github.com/grovesNL/glow/blob/main/examples/hello/src/main.rs work on my mac (when run with cargo run --features=glutin)

[emilk]

Ok, I got it to work! You were using hard-coded attribute locations, which is not portable. This is the changes that makes it work on my mac:

diff --git a/egui_glium/src/painter.rs b/egui_glium/src/painter.rs
index b6e694e..6ebf12c 100644
--- a/egui_glium/src/painter.rs
+++ b/egui_glium/src/painter.rs
@@ -150,15 +150,34 @@ impl Painter {
         // TODO error handling
         unsafe {
             let v = gl.create_shader(glow::VERTEX_SHADER).unwrap();
-            let f = gl.create_shader(glow::FRAGMENT_SHADER).unwrap();
             gl.shader_source(v, &v_src);
-            gl.shader_source(f, &f_src);
             gl.compile_shader(v);
+            if !gl.get_shader_compile_status(v) {
+                panic!(
+                    "Failed to compile vertex shader: {}",
+                    gl.get_shader_info_log(v)
+                );
+            }
+
+            let f = gl.create_shader(glow::FRAGMENT_SHADER).unwrap();
+            gl.shader_source(f, &f_src);
             gl.compile_shader(f);
+            if !gl.get_shader_compile_status(f) {
+                panic!(
+                    "Failed to compile fragment shader: {}",
+                    gl.get_shader_info_log(f)
+                );
+            }
+
             let program = gl.create_program().unwrap();
             gl.attach_shader(program, v);
             gl.attach_shader(program, f);
             gl.link_program(program);
+            if !gl.get_program_link_status(program) {
+                panic!("{}", gl.get_program_info_log(program));
+            }
+            gl.detach_shader(program, v);
+            gl.detach_shader(program, f);
             gl.delete_shader(v);
             gl.delete_shader(f);
 
@@ -174,35 +193,39 @@ impl Painter {
             gl.bind_vertex_array(Some(va));
             gl.bind_buffer(glow::ARRAY_BUFFER, Some(vb));
 
+            let a_pos_loc = gl.get_attrib_location(program, "a_pos").unwrap();
+            let a_tc_loc = gl.get_attrib_location(program, "a_tc").unwrap();
+            let a_srgba_loc = gl.get_attrib_location(program, "a_srgba").unwrap();
+
             gl.vertex_attrib_pointer_f32(
-                0,
+                a_pos_loc,
                 2,
                 glow::FLOAT,
                 false,
                 std::mem::size_of::<Vertex>() as i32,
                 0,
             );
-            gl.enable_vertex_attrib_array(0);
+            gl.enable_vertex_attrib_array(a_pos_loc);
 
             gl.vertex_attrib_pointer_f32(
-                2,
+                a_tc_loc,
                 2,
                 glow::FLOAT,
                 false,
                 std::mem::size_of::<Vertex>() as i32,
                 2 * std::mem::size_of::<f32>() as i32,
             );
-            gl.enable_vertex_attrib_array(2);
+            gl.enable_vertex_attrib_array(a_tc_loc);
 
             gl.vertex_attrib_pointer_f32(
-                1,
+                a_srgba_loc,
                 4,
                 glow::UNSIGNED_BYTE,
                 false,
                 std::mem::size_of::<Vertex>() as i32,
                 4 * std::mem::size_of::<f32>() as i32,
             );
-            gl.enable_vertex_attrib_array(1);
+            gl.enable_vertex_attrib_array(a_srgba_loc);
             assert_eq!(gl.get_error(), glow::NO_ERROR);
 
             Painter {
diff --git a/egui_glium/src/shader/vertex.glsl b/egui_glium/src/shader/vertex.glsl
index df21a02..cf2309a 100644
--- a/egui_glium/src/shader/vertex.glsl
+++ b/egui_glium/src/shader/vertex.glsl
@@ -1,4 +1,4 @@
-#if !defined(GL_ES) && __VERSION >= 140
+#if !defined(GL_ES) && __VERSION__ >= 140
 #define I in
 #define O out
 #define V(x) x

[emilk]

There is some other issues though: scaling the window leaves the framebuffer in disarray. Maybe that is not only on Mac?

[AlexApps99]

I'll merge the changes you've made (thanks for letting me know, I wasn't aware that attrib_loc was required)
As for scaling, I don't recall testing it so I'll do that now
Edit: yes happens on mine too (can't believe I forgot to test that 🤦) I'll add a call to glViewport on resize now

[AlexApps99]

Please give master a try now, fingers crossed the issues should be ironed out

[emilk]

Weirdly enough, the window resizing bug still persists on my mac. When I make my window slightly more narrow this is the result:

it's as if it is still dividing the vertex coordinates with the old window size, yet the values in gl.uniform_2_f32(Some(&self.u_screen_size), width_in_points, height_in_points); seem to be correct every frame

[AlexApps99]

I've made a commit that debug prints the resize event, please let me know if it prints anything out of the ordinary

[emilk]

I figured it out (thanks to looking at the glow examples). This is the required fix:

iff --git a/egui_glium/src/backend.rs b/egui_glium/src/backend.rs
index 97562735..77c075ed 100644
--- a/egui_glium/src/backend.rs
+++ b/egui_glium/src/backend.rs
@@ -289,16 +289,8 @@ pub fn run(mut app: Box<dyn epi::App>, native_options: epi::NativeOptions) {
                         running = false;
                     }
 
-                    if let glutin::event::WindowEvent::Resized(glutin::dpi::PhysicalSize {
-                        width: width_in_pixels,
-                        height: height_in_pixels,
-                    }) = event
-                    {
-                        println!("{}x{}", width_in_pixels, height_in_pixels);
-                        unsafe {
-                            use glow::HasContext;
-                            gl.viewport(0, 0, width_in_pixels as i32, height_in_pixels as i32);
-                        }
+                    if let glutin::event::WindowEvent::Resized(physical_size) = event {
+                        gl_window.resize(physical_size);
                     }
 
                     if let glutin::event::WindowEvent::Focused(new_focused) = event {
diff --git a/egui_glium/src/painter.rs b/egui_glium/src/painter.rs
index e6ab8d6f..1fdb727b 100644
--- a/egui_glium/src/painter.rs
+++ b/egui_glium/src/painter.rs
@@ -302,6 +302,8 @@ impl Painter {
         let width_in_points = width_in_pixels as f32 / pixels_per_point;
         let height_in_points = height_in_pixels as f32 / pixels_per_point;
 
+        gl.viewport(0, 0, width_in_pixels as i32, height_in_pixels as i32);
+
         gl.use_program(Some(self.program));
 
         // The texture coordinates for text are so that both nearest and linear should work with the egui font texture.

[AlexApps99]

Now that egui_winit has been merged, I have rebased everything and it doesn't seem to conflict. I'd be happy to make this ready for review now.
There are still a few more things to do, like renaming the crate to egui_glow once reviews are complete, and reimplementing the examples that were added recently. The library code itself can be considered mostly complete for the time being.

I also think it might be worth:

• sharing more code between egui_glium and egui_glow (this is made easy if egui_glium passes the underlying glutin::window::Window where functions don't need the stuff in Display)
• sharing the GLSL shader with egui_web as well as egui_glium/egui_glow, and maybe testing egui_glow on web (glow supports WebGL)
• making the OpenGL code in painter.rs more robust, as glow doesn't really have any safety measures and it's probably possible for some errors to occur

These can either be addressed now, or in separate PRs, depending on what is best to proceed.
Please let me know what you think

[emilk]

There are still a few more things to do, like renaming the crate to egui_glow once reviews are complete

Yes, let's wait on this a while longer. There are some more winit-changes coming down the pipe (#756).

---

sharing more code between egui_glium and egui_glow (this is made easy if egui_glium passes the underlying glutin::window::Window where functions don't need the stuff in Display)

Code we could consider sharing:

• backend.rs: the event loop is almost identical. If we can create a good abstraction then perhaps most of it can be moved to egui-winit ?
• peristence.rs - perhaps make it part of epi behind a feature flag?
• window_settings.rs - could perhaps be made part of egui-winit too?

Once those three files are gone then egui_glium and egui_glow will have almost no common code (which is great!).

I can help out with this!

---

sharing the GLSL shader with egui_web as well as egui_glium/egui_glow

The glow shader code is quite complex due to having to support different glsl version, so I think we should keep it separate. Some code duplication for the relatively small shader files is fine imho.

---

maybe testing egui_glow on web (glow supports WebGL)

This is mostly interesting for glow users that are using glow on the web and want to use egui. Probably not a priority until such a user reveals themself.

---

making the OpenGL code in painter.rs more robust, as glow doesn't really have any safety measures and it's probably possible for some errors to occur

Yes please! All errors that can be caught and reported should be so.

---

These can either be addressed now, or in separate PRs, depending on what is best to proceed. Please let me know what you think

Let me think about how to move some more code out of egui_glium before you split off egui_glow!

[emilk]

awesome work!

[AlexApps99]

If this is good, I will rebase and flatten, and move it into a separate folder egui_glow.
Then, I will put some additional work into removing shared code, before reviewing for what will hopefully be the final time.

[emilk]

Looks really good! I think this is ready to be split off to its own folder now.

I think it is fine with some duplicated code (backend.rs and peristence.rs) for now - we can fix that in a later PR!

[AlexApps99]

I believe this is ready to merge, once the final review is done.
Thanks for being patient, I really appreciate your help.

[emilk]

Just a few small things, then this is good to go!

Things to follow up with later (after this PR is merged):

• Add a feature-flag to eframe to use egui_glow instead of egui_glium.
• Try to remove as much duplicated code as possible between egui_glium and egui_glow, but without going crazy.

[emilk]

🥳

thanks for all your work on this!

[leviska]

hooray, been waiting this for some time, thank you all for your work 😍🎉

[emilk]

I noticed (after merging, d'oh!) that the problem when resizing the glow window is back again:

[AlexApps99]

Is this to do with egui-winit, or to do with some sort of regression in egui_glow?

[emilk]

Found it!

                if let glutin::event::WindowEvent::Resized(physical_size) = event {
                    gl_window.resize(physical_size);
                }

was only added to egui_glow/examples/pure.rs, not to egui_glow/src/backend.rs. Fixed in cf273e3