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.

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

Add egui_glow backend as alternative to egui_glium #685

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

AlexApps99
Copy link
Contributor

@AlexApps99 AlexApps99 commented Aug 29, 2021

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)
    image
  • Move egui_glow into its own directory, once reviewing is complete

Closes #93.

@AlexApps99
Copy link
Contributor Author

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
Copy link
Contributor

ar37-rs commented Aug 31, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

ar37-rs commented Aug 31, 2021

@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
Copy link
Contributor Author

@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
Copy link
Contributor

ar37-rs commented Aug 31, 2021

@AlexApps99 👍

@AlexApps99
Copy link
Contributor Author

AlexApps99 commented Sep 1, 2021

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
Copy link
Contributor Author

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
Copy link
Owner

emilk commented Sep 2, 2021

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
Copy link
Owner

emilk commented Sep 3, 2021

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

AlexApps99 commented Sep 3, 2021

image
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
Copy link
Contributor Author

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
Copy link
Owner

emilk commented Sep 4, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@emilk
Copy link
Owner

emilk commented Sep 4, 2021

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
Copy link
Contributor Author

AlexApps99 commented Sep 4, 2021

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
image
So I'm not sure what's amiss

@emilk
Copy link
Owner

emilk commented Sep 4, 2021

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
Copy link
Owner

emilk commented Sep 9, 2021

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
Copy link
Owner

emilk commented Sep 9, 2021

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
Copy link
Owner

emilk commented Sep 9, 2021

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

@AlexApps99
Copy link
Contributor Author

AlexApps99 commented Sep 9, 2021

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
Copy link
Contributor Author

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

@emilk
Copy link
Owner

emilk commented Sep 20, 2021

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

Screen Shot 2021-09-20 at 21 54 09

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
Copy link
Contributor Author

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

@emilk
Copy link
Owner

emilk commented Sep 22, 2021

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.

emilk added a commit that referenced this pull request Sep 26, 2021
This extracts a lot of code from egui_glium into a reusable crate
`egui_for_winit`. This can be reused for `egui_glow`
(#685) and many other integrations.

This replaces the (somewhat outdated) 3rd party
[`egui_winit_platform`](https://github.com/hasenbanck/egui_winit_platform)
which is used by a few integrations:
<https://crates.io/crates/egui_winit_platform/reverse_dependencies>.

The name `egui_for_winit` is not great, but `egui_winit`
[was already taken](https://crates.io/crates/egui-winit).

I would welcome a better name suggestion!
@AlexApps99
Copy link
Contributor Author

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

@AlexApps99 AlexApps99 marked this pull request as ready for review September 29, 2021 01:14
@emilk
Copy link
Owner

emilk commented Sep 29, 2021

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!

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

awesome work!

egui_glium/src/lib.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
@AlexApps99
Copy link
Contributor Author

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.

egui_glium/src/painter.rs Outdated Show resolved Hide resolved
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

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!

egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/painter.rs Outdated Show resolved Hide resolved
egui_glium/src/persistence.rs Outdated Show resolved Hide resolved
@AlexApps99 AlexApps99 changed the title Replace Glium dependency with glow Add egui_glow backend as alternative to egui_glium Oct 18, 2021
@AlexApps99
Copy link
Contributor Author

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

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

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.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
egui_glow/CHANGELOG.md Outdated Show resolved Hide resolved
@emilk
Copy link
Owner

emilk commented Oct 18, 2021

🥳

thanks for all your work on this!

@leviska
Copy link

leviska commented Oct 18, 2021

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

@emilk
Copy link
Owner

emilk commented Oct 18, 2021

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

Screen Shot 2021-10-18 at 23 31 08
Screen Shot 2021-10-18 at 23 30 37

@AlexApps99
Copy link
Contributor Author

AlexApps99 commented Oct 18, 2021

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

@emilk
Copy link
Owner

emilk commented Oct 18, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Glium
4 participants