Add egui_glow backend as alternative to egui_glium #685
Conversation
|
I'm not sure if a lifetime restriction is the way to go, as I don't have that much experience with lifetimes, and try to avoid them wherever possible - have I made any mistakes in adding them? |
|
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). 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 |
|
@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 |
|
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 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 Please give me feedback, so I can improve it. |
|
I think the cleanest solution it to pass in This means the users having to call a |
|
@AlexApps99 I've removed the Less code to duplicate between |
|
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 |
|
Edit: It happens on glium too |
|
It should be possible to unify all shaders into a single file using preprocessor ifdefs for each version, as GLSL provides the |
|
I tried it just now on my Intel Mac (macOS Mojave 10.14.6) and got this: The offending line: |
|
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 |
|
Here it is, see what it says: |
|
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: So it seems my drivers can't handle 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 From GLSL 1.20 spec |
|
|
|
FWIW, https://github.com/grovesNL/glow/blob/main/examples/hello/src/main.rs work on my mac (when run with |
|
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 |
|
There is some other issues though: scaling the window leaves the framebuffer in disarray. Maybe that is not only on Mac? |
|
I'll merge the changes you've made (thanks for letting me know, I wasn't aware that attrib_loc was required) |
|
Please give master a try now, fingers crossed the issues should be ironed out |
|
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 |
|
I've made a commit that debug prints the resize event, please let me know if it prints anything out of the ordinary |
|
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. |
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!
|
Now that I also think it might be worth:
These can either be addressed now, or in separate PRs, depending on what is best to proceed. |
Yes, let's wait on this a while longer. There are some more winit-changes coming down the pipe (#756).
Code we could consider sharing:
Once those three files are gone then I can help out with this!
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.
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.
Yes please! All errors that can be caught and reported should be so.
Let me think about how to move some more code out of |
|
If this is good, I will rebase and flatten, and move it into a separate folder |
|
I believe this is ready to merge, once the final review is done. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
eframeto useegui_glowinstead ofegui_glium. - Try to remove as much duplicated code as possible between
egui_gliumandegui_glow, but without going crazy.
|
🥳 thanks for all your work on this! |
|
hooray, been waiting this for some time, thank you all for your work 😍🎉 |
|
I noticed (after merging, d'oh!) that the problem when resizing the glow window is back again:
|
|
Is this to do with |
|
Found it! if let glutin::event::WindowEvent::Resized(physical_size) = event {
gl_window.resize(physical_size);
}was only added to |
I have opened this draft PR to move discussion about
egui_glowfrom #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:
egui_gliumegui_glowinto its own directory, once reviewing is completeCloses #93.