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 Continuous mode to ctx.memory.options #4214

Draft
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

rustbasic
Copy link
Contributor

Add Continuous mode to ctx.memory.options

let continuous_mode = egui_ctx.options(|options| options.continuous_mode);
if continuous_mode {
let viewport_id = egui_ctx.viewport_id();
// TODO : Do not recall until the next repaint.
Copy link
Contributor

Choose a reason for hiding this comment

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

The contributing guidelines say:

Add your github handle to the TODO:s you write, e.g: TODO(emilk): clean this up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The contributing guidelines say:

Add your github handle to the TODO:s you write, e.g: TODO(emilk): clean this up

thank you.

Copy link
Contributor Author

@rustbasic rustbasic Mar 23, 2024

Choose a reason for hiding this comment

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

The contributing guidelines say:

Add your github handle to the TODO:s you write, e.g: TODO(emilk): clean this up

If you know how to pass the doc test, please let me know.
It doesn't work with the "no_run" option.

It would be better to exclude what is in egui_demo_app because it seems that we do not want to write it in the doc.
Still, I'd like to know how.

crates/egui/src/memory.rs Show resolved Hide resolved
// TODO(rustbasic) : Do not recall until the next repaint.
// 1000 millis / 60 fps = 16.67 millis
self.egui_ctx
.request_repaint_after_for(std::time::Duration::from_millis(8), viewport_id);
Copy link
Owner

Choose a reason for hiding this comment

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

why 8 ms delay?

Copy link
Contributor Author

@rustbasic rustbasic Mar 25, 2024

Choose a reason for hiding this comment

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

why 8 ms delay?

It takes about two attempts.
It is the most efficient.
If you do 16.67ms, 60 fps cannot be achieved.

In conclusion, this method solves everything with only 1% CPU usage.
However, having subviewports increases usage by about 10%.
Currently, if you keep calling request_repaint(), more than 10% of your CPU is being used even without doing anything.

Copy link
Owner

Choose a reason for hiding this comment

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

But why not request an immediate repaint? eframe has vsync anyways

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But why not request an immediate repaint? eframe has vsync anyways

But why not request an immediate repaint? eframe has vsync anyways

This is an unnecessary waste of CPU usage.
It would be a good idea to apply this and continue to monitor and upgrade.
It can't be perfect from the beginning.

Copy link
Owner

Choose a reason for hiding this comment

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

It's not a waste of CPU, because of vsync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a waste of CPU, because of vsync.

It will be more effective if you actually use it and check the CPU usage.

There is also the advantage that the user does not have to worry about request_repaint().

@@ -191,6 +191,10 @@ pub struct Options {
/// Controls the tessellator.
pub tessellation_options: epaint::TessellationOptions,

/// If `true`, This will call `egui::Context::request_repaint()` at the end of each frame
/// If `false` (default), egui is only updated if are input events (like mouse movements) or there are some animations in the GUI.
pub continuous_mode: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we should just move enum RunMode { into egui from crates/egui_demo_app/src/backend_panel.rs. At least we should copy its documentation.

@@ -65,10 +65,11 @@ impl BackendPanel {
match self.run_mode {
Copy link
Owner

Choose a reason for hiding this comment

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

self.run_mode can be replaced with options.continuous_mode

@rustbasic rustbasic requested a review from emilk March 30, 2024 08:59
@emilk emilk marked this pull request as draft April 21, 2024 10:03
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.

3 participants