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.

Sign up for GitHub

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

Jump to bottom

Make request_repaint_after actually respect the delay #3925

Closed
wants to merge 1 commit into from
Closed

Make request_repaint_after actually respect the delay #3925

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/egui/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,13 @@ impl ContextImpl {
let viewport = self.viewports.entry(viewport_id).or_default();

viewport.repaint.prev_frame_paint_delay = viewport.repaint.repaint_delay;
viewport.repaint.outstanding = viewport.repaint.outstanding.saturating_sub(1);
Copy link
Owner

Choose a reason for hiding this comment

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

The point of outstanding it to run two refreshes on request_repaint, and on startup, but with this PR that is now ignored.

That said, thanks to #3930 we might not need that "repaint twice" logic

Copy link
Author

@athre0z athre0z Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah okay, I didn't realize that you actually wanted two invocations. It does make sense though, as it serves as a simple form of resilience against all kinds of "frame updates stuff and then expects it to show up on screen soon" -- let's try to keep that.

Note however that the previous logic didn't actually work: the == 0 branch was previously never taken at all, because begin_frame_repaint_logic isn't ever actually called once outstanding reached 0. You can verify this by throwing in a breakpoint or panic!().

Would it make sense to move the decrement as suggested in this PR and then simply explicitly set outstanding to 2 in both the constructor and request_repaint? I believe that this would also convey the intention of redrawing twice more clearly.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah okay, I didn't realize that you actually wanted two invocations. It does make sense though, as it serves as a simple form of resilience against all kinds of "frame updates stuff and then expects it to show up on screen soon"

Exactly. It's a guard-rail, that's is nice to have. I'm not sure it is actually needed for anything (I'm turning it off now and testing, and nothing is obviously broken). Especially with #3930 merged the need should be smaller.

If you go to the backend panel of egui_demo_app there is a simple test for some of this:

image

Clicking that button once will produce exactly 2 frames to be repainted after a five second delay, so it's not all broken.

Copy link
Author

@athre0z athre0z Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm yeah, true. If you just call request_repaint_after once, it is working as intended. However, if you call it on every frame (like my sample code / diff does) in order to request a redraw e.g. at least once per second, it falls over because outstanding == 0 is never reached.


if viewport.repaint.outstanding == 0 {
// We are repainting now, so we can wait a while for the next repaint.
viewport.repaint.repaint_delay = Duration::MAX;
} else {
viewport.repaint.repaint_delay = Duration::ZERO;
viewport.repaint.outstanding -= 1;
if let Some(callback) = &self.request_repaint_callback {
(callback)(RequestRepaintInfo {
viewport_id,
Expand Down
Loading