-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Make egui_glow painter to work on web #868
Conversation
egui_web add `use_glow_painter` feature. egui_glow add `painter_only` feature.
I monitored by Spector.js and found glow painter 's draw call is half of egui_web.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really promising, nice work!
egui_glow/src/misc_util.rs
Outdated
pub(crate) unsafe fn new(gl: &glow::Context, is_native_vao: bool) -> Self { | ||
if is_native_vao { | ||
Self::Native(gl.create_vertex_array().unwrap()) | ||
} else { | ||
Self::Emulated(crate::vao_emulate::EmulatedVao::new()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nitpicky, but I think it would make sense to offer two constructors here instead of one.
pub(crate) unsafe fn new(gl: &glow::Context, is_native_vao: bool) -> Self { | |
if is_native_vao { | |
Self::Native(gl.create_vertex_array().unwrap()) | |
} else { | |
Self::Emulated(crate::vao_emulate::EmulatedVao::new()) | |
} | |
} | |
pub(crate) unsafe fn native(gl: &glow::Context) -> Self { | |
Self::Native(gl.create_vertex_array().unwrap()) | |
} | |
pub(crate) fn emulated() -> Self { | |
Self::Emulated(crate::vao_emulate::EmulatedVao::new()) | |
} |
Also, wouldn't the API be nicer if the constructor captured the &glow::Context
here so it doesn't need to be passed to the other methods? That might also help reduce some confusion about why VAO::bind_vertex_array
is unsafe, but the others are not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about two constructors 👍
Capturing &glow::Context
leads to a lot of ugly lifetime issues. Passing it in by reference is also more explicit, i.e. easier to see what is actually doing graphics calls. This was discussed in the original glow PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok i will split constructor.
but is there good way to switch emulated mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capturing
&glow::Context
leads to a lot of ugly lifetime issues. Passing it in by reference is also more explicit, i.e. easier to see what is actually doing graphics calls. This was discussed in the original glow PR.
Ok! Good to know, I didn't follow that conversation closely.
Ok i will split constructor.
but is there good way to switch emulated mode?
This question implies there is a way to switch the emulation mode with just one constructor? I don't believe that is the case before or after the suggested patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of things that needs testing with "Color test":
- Native glow
- Web glow on WebGL2
- Web glow on WebGL1 with sRGBA
- Web glow on WebGL1 without sRGBA (won't pass the color test, but should look as good as the current WebGL painter)
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
glow does not provide dummy from_webgl_context for native
Thanks @t18b219k - great work! |
Sorry I didn't have much feedback for it - busy with exams, I won't be able to contribute much for a bit. |
I made PR #852 but this way affect many points.
These my works
painter_only
to reuse glow painterCurrent tested platforms