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

Keep track of why request_discard was called #5134

Merged
merged 5 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions crates/eframe/src/web/app_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,8 @@ impl AppRunner {
ime,
#[cfg(feature = "accesskit")]
accesskit_update: _, // not currently implemented
num_completed_passes: _, // handled by `Context::run`
requested_discard: _, // handled by `Context::run`
num_completed_passes: _, // handled by `Context::run`
request_discard_reasons: _, // handled by `Context::run`
} = platform_output;

super::set_cursor_icon(cursor_icon);
Expand Down
4 changes: 2 additions & 2 deletions crates/egui-winit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -823,8 +823,8 @@ impl State {
ime,
#[cfg(feature = "accesskit")]
accesskit_update,
num_completed_passes: _, // `egui::Context::run` handles this
requested_discard: _, // `egui::Context::run` handles this
num_completed_passes: _, // `egui::Context::run` handles this
request_discard_reasons: _, // `egui::Context::run` handles this
} = platform_output;

self.set_cursor_icon(window, cursor_icon);
Expand Down
98 changes: 67 additions & 31 deletions crates/egui/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,19 +284,28 @@ pub struct ViewportState {
pub num_multipass_in_row: usize,
}

/// What called [`Context::request_repaint`]?
#[derive(Clone)]
/// What called [`Context::request_repaint`] or [`Context::request_discard`]?
#[derive(Clone, PartialEq, Eq, Hash)]
pub struct RepaintCause {
/// What file had the call that requested the repaint?
pub file: &'static str,

/// What line number of the call that requested the repaint?
pub line: u32,

/// Explicit reason; human readable.
pub reason: Cow<'static, str>,
}

impl std::fmt::Debug for RepaintCause {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}:{}", self.file, self.line)
write!(f, "{}:{} {}", self.file, self.line, self.reason)
}
}

impl std::fmt::Display for RepaintCause {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}:{} {}", self.file, self.line, self.reason)
}
}

Expand All @@ -309,13 +318,21 @@ impl RepaintCause {
Self {
file: caller.file(),
line: caller.line(),
reason: "".into(),
}
}
}

impl std::fmt::Display for RepaintCause {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}:{}", self.file, self.line)
/// Capture the file and line number of the call site,
/// as well as add a reason.
#[allow(clippy::new_without_default)]
#[track_caller]
pub fn new_reason(reason: impl Into<Cow<'static, str>>) -> Self {
let caller = Location::caller();
Self {
file: caller.file(),
line: caller.line(),
reason: reason.into(),
}
}
}

Expand Down Expand Up @@ -791,21 +808,21 @@ impl Context {
let viewport = ctx.viewport_for(viewport_id);
viewport.output.num_completed_passes =
std::mem::take(&mut output.platform_output.num_completed_passes);
output.platform_output.requested_discard = false;
output.platform_output.request_discard_reasons.clear();
});

self.begin_pass(new_input.take());
run_ui(self);
output.append(self.end_pass());
debug_assert!(0 < output.platform_output.num_completed_passes);

if !output.platform_output.requested_discard {
if !output.platform_output.requested_discard() {
break; // no need for another pass
}

if max_passes <= output.platform_output.num_completed_passes {
#[cfg(feature = "log")]
log::debug!("Ignoring call request_discard, because max_passes={max_passes}");
log::debug!("Ignoring call request_discard, because max_passes={max_passes}. Requested from {:?}", output.platform_output.request_discard_reasons);

break;
}
Expand Down Expand Up @@ -1641,8 +1658,14 @@ impl Context {
///
/// You should be very conservative with when you call [`Self::request_discard`],
/// as it will cause an extra ui pass, potentially leading to extra CPU use and frame judder.
pub fn request_discard(&self) {
self.output_mut(|o| o.requested_discard = true);
///
/// The given reason should be a human-readable string that explains why `request_discard`
/// was called. This will be shown in certain debug situations, to help you figure out
/// why a pass was discarded.
#[track_caller]
pub fn request_discard(&self, reason: impl Into<Cow<'static, str>>) {
let cause = RepaintCause::new_reason(reason);
self.output_mut(|o| o.request_discard_reasons.push(cause));

#[cfg(feature = "log")]
log::trace!(
Expand All @@ -1664,7 +1687,7 @@ impl Context {
self.write(|ctx| {
let vp = ctx.viewport();
// NOTE: `num_passes` is incremented
vp.output.requested_discard
vp.output.requested_discard()
&& vp.output.num_completed_passes + 1 < ctx.memory.options.max_passes.get()
})
}
Expand Down Expand Up @@ -2197,12 +2220,16 @@ impl Context {
if 3 <= num_multipass_in_row {
// If you see this message, it means we've been paying the cost of multi-pass for multiple frames in a row.
// This is likely a bug. `request_discard` should only be called in rare situations, when some layout changes.
self.debug_painter().debug_text(
Pos2::ZERO,
Align2::LEFT_TOP,
Color32::RED,
format!("egui PERF WARNING: request_discard has been called {num_multipass_in_row} frames in a row"),
);

let mut warning = format!("egui PERF WARNING: request_discard has been called {num_multipass_in_row} frames in a row");
self.viewport(|vp| {
for reason in &vp.output.request_discard_reasons {
warning += &format!("\n {reason}");
}
});

self.debug_painter()
.debug_text(Pos2::ZERO, Align2::LEFT_TOP, Color32::RED, warning);
}
}
}
Expand Down Expand Up @@ -3734,28 +3761,37 @@ mod test {
let output = ctx.run(Default::default(), |ctx| {
num_calls += 1;
assert_eq!(ctx.output(|o| o.num_completed_passes), 0);
assert!(!ctx.output(|o| o.requested_discard));
assert!(!ctx.output(|o| o.requested_discard()));
assert!(!ctx.will_discard());
});
assert_eq!(num_calls, 1);
assert_eq!(output.platform_output.num_completed_passes, 1);
assert!(!output.platform_output.requested_discard);
assert!(!output.platform_output.requested_discard());
}

// A single call, with a denied request to discard:
{
let mut num_calls = 0;
let output = ctx.run(Default::default(), |ctx| {
num_calls += 1;
ctx.request_discard();
ctx.request_discard("test");
assert!(!ctx.will_discard(), "The request should have been denied");
});
assert_eq!(num_calls, 1);
assert_eq!(output.platform_output.num_completed_passes, 1);
assert!(
output.platform_output.requested_discard,
output.platform_output.requested_discard(),
"The request should be reported"
);
assert_eq!(
output
.platform_output
.request_discard_reasons
.first()
.unwrap()
.reason,
"test"
);
}
}

Expand All @@ -3769,13 +3805,13 @@ mod test {
let mut num_calls = 0;
let output = ctx.run(Default::default(), |ctx| {
assert_eq!(ctx.output(|o| o.num_completed_passes), 0);
assert!(!ctx.output(|o| o.requested_discard));
assert!(!ctx.output(|o| o.requested_discard()));
assert!(!ctx.will_discard());
num_calls += 1;
});
assert_eq!(num_calls, 1);
assert_eq!(output.platform_output.num_completed_passes, 1);
assert!(!output.platform_output.requested_discard);
assert!(!output.platform_output.requested_discard());
}

// Request discard once:
Expand All @@ -3786,7 +3822,7 @@ mod test {

assert!(!ctx.will_discard());
if num_calls == 0 {
ctx.request_discard();
ctx.request_discard("test");
assert!(ctx.will_discard());
}

Expand All @@ -3795,7 +3831,7 @@ mod test {
assert_eq!(num_calls, 2);
assert_eq!(output.platform_output.num_completed_passes, 2);
assert!(
!output.platform_output.requested_discard,
!output.platform_output.requested_discard(),
"The request should have been cleared when fulfilled"
);
}
Expand All @@ -3807,7 +3843,7 @@ mod test {
assert_eq!(ctx.output(|o| o.num_completed_passes), num_calls);

assert!(!ctx.will_discard());
ctx.request_discard();
ctx.request_discard("test");
if num_calls == 0 {
assert!(ctx.will_discard(), "First request granted");
} else {
Expand All @@ -3819,7 +3855,7 @@ mod test {
assert_eq!(num_calls, 2);
assert_eq!(output.platform_output.num_completed_passes, 2);
assert!(
output.platform_output.requested_discard,
output.platform_output.requested_discard(),
"The unfulfilled request should be reported"
);
}
Expand All @@ -3838,7 +3874,7 @@ mod test {

assert!(!ctx.will_discard());
if num_calls <= 2 {
ctx.request_discard();
ctx.request_discard("test");
assert!(ctx.will_discard());
}

Expand All @@ -3847,7 +3883,7 @@ mod test {
assert_eq!(num_calls, 4);
assert_eq!(output.platform_output.num_completed_passes, 4);
assert!(
!output.platform_output.requested_discard,
!output.platform_output.requested_discard(),
"The request should have been cleared when fulfilled"
);
}
Expand Down
19 changes: 15 additions & 4 deletions crates/egui/src/data/output.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! All the data egui returns to the backend at the end of each frame.

use crate::{ViewportIdMap, ViewportOutput, WidgetType};
use crate::{RepaintCause, ViewportIdMap, ViewportOutput, WidgetType};

/// What egui emits each frame from [`crate::Context::run`].
///
Expand Down Expand Up @@ -133,7 +133,12 @@ pub struct PlatformOutput {
pub num_completed_passes: usize,

/// Was [`crate::Context::request_discard`] called during the latest pass?
pub requested_discard: bool,
///
/// If so, what was the reason(s) for it?
///
/// If empty, there was never any calls.
#[cfg_attr(feature = "serde", serde(skip))]
pub request_discard_reasons: Vec<RepaintCause>,
}

impl PlatformOutput {
Expand Down Expand Up @@ -167,7 +172,7 @@ impl PlatformOutput {
#[cfg(feature = "accesskit")]
accesskit_update,
num_completed_passes,
requested_discard,
mut request_discard_reasons,
} = newer;

self.cursor_icon = cursor_icon;
Expand All @@ -181,7 +186,8 @@ impl PlatformOutput {
self.mutable_text_under_cursor = mutable_text_under_cursor;
self.ime = ime.or(self.ime);
self.num_completed_passes += num_completed_passes;
self.requested_discard |= requested_discard;
self.request_discard_reasons
.append(&mut request_discard_reasons);

#[cfg(feature = "accesskit")]
{
Expand All @@ -197,6 +203,11 @@ impl PlatformOutput {
self.cursor_icon = taken.cursor_icon; // everything else is ephemeral
taken
}

/// Was [`crate::Context::request_discard`] called?
pub fn requested_discard(&self) -> bool {
!self.request_discard_reasons.is_empty()
}
}

/// What URL to open, and how.
Expand Down
2 changes: 1 addition & 1 deletion crates/egui/src/grid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ impl Grid {

if ui.is_visible() {
// Try to cover up the glitchy initial frame:
ui.ctx().request_discard();
ui.ctx().request_discard("new Grid");
}

// Hide the ui this frame, and make things as narrow as possible:
Expand Down
2 changes: 1 addition & 1 deletion crates/egui_demo_app/src/backend_panel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ impl BackendPanel {

ui.horizontal(|ui| {
if ui.button("Request discard").clicked() {
ui.ctx().request_discard();
ui.ctx().request_discard("Manual button click");

if !ui.ctx().will_discard() {
ui.label("Discard denied!");
Expand Down
Loading