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

Make egui_glow painter to work on web #868

Merged
merged 30 commits into from
Nov 3, 2021
Merged

Make egui_glow painter to work on web #868

merged 30 commits into from
Nov 3, 2021

Conversation

KentaTheBugMaker
Copy link
Contributor

@KentaTheBugMaker KentaTheBugMaker commented Nov 2, 2021

I made PR #852 but this way affect many points.
These my works

  • eliminate glutin dependency from painter
  • port PostProcess for sRGB correct color for web from egui_web painter
  • eliminate VAO by emulating VAO
  • add feature flag painter_only to reuse glow painter
  • implement egui_web::Painter trait for wrapped glow painter
  • share GLSL ES and GLSL shader
    Current tested platforms
  • OpenGL 4.6 (Ryzen 5 5500U with AMD proprietary windows driver) (not tested but coded to work 2.0 or above)
  • WebGL2 (firefox android , MS edge)
  • WebGL1+sRGB (WebGL2 disabled firefox)
  • WebGL1 (servo nightly build)

@KentaTheBugMaker
Copy link
Contributor Author

I monitored by Spector.js and found glow painter 's draw call is half of egui_web.
which means lower CPU usage,lower power consumption,higher frame rate.

  • glow painter 132
  • webgl2 painter 269

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.

This looks really promising, nice work!

egui_glow/src/lib.rs Outdated Show resolved Hide resolved
egui_glow/src/misc_util.rs Outdated Show resolved Hide resolved
egui_glow/src/painter.rs Outdated Show resolved Hide resolved
egui_web/src/backend.rs Outdated Show resolved Hide resolved
egui_web/src/backend.rs Outdated Show resolved Hide resolved
egui_glow/src/shader/post_vertex_100es.glsl Outdated Show resolved Hide resolved
egui_glow/src/post_process.rs Outdated Show resolved Hide resolved
egui_glow/src/painter.rs Outdated Show resolved Hide resolved
egui_glow/src/epi_backend.rs Outdated Show resolved Hide resolved
egui_glow/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 137 to 143
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())
}
}
Copy link
Contributor

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.

Suggested change
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?

Copy link
Owner

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this procedure good enough to switch emulated mode .
Is there any suggestion?
vao_switch_procedure

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.

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)

egui_glow/CHANGELOG.md Outdated Show resolved Hide resolved
egui_glow/examples/pure_glow.rs Outdated Show resolved Hide resolved
egui_glow/src/painter.rs Show resolved Hide resolved
egui_glow/src/painter.rs Outdated Show resolved Hide resolved
egui_glow/src/painter.rs Outdated Show resolved Hide resolved
egui_glow/src/painter.rs Outdated Show resolved Hide resolved
egui_glow/src/painter.rs Outdated Show resolved Hide resolved
egui_glow/src/painter.rs Show resolved Hide resolved
egui_web/CHANGELOG.md Outdated Show resolved Hide resolved
KentaTheBugMaker and others added 10 commits November 3, 2021 21:39
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>
@KentaTheBugMaker
Copy link
Contributor Author

merging upstream change
Now i post some good photo
スクリーンショット 2021-11-03 230707
.

@emilk emilk changed the title glutin independent glow painter on native and web Make egui_glow painter to work on web Nov 3, 2021
@emilk
Copy link
Owner

emilk commented Nov 3, 2021

Thanks @t18b219k - great work!

@emilk emilk merged commit 804722a into emilk:master Nov 3, 2021
emilk added a commit that referenced this pull request Nov 3, 2021
@AlexApps99
Copy link
Contributor

Sorry I didn't have much feedback for it - busy with exams, I won't be able to contribute much for a bit.

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.

None yet

4 participants