diff --git a/crates/egui/src/context.rs b/crates/egui/src/context.rs index 3b36150e4..30818a772 100644 --- a/crates/egui/src/context.rs +++ b/crates/egui/src/context.rs @@ -2636,6 +2636,19 @@ impl ContextImpl { } } + #[cfg(debug_assertions)] + let shapes = if self.memory.options.style().debug.warn_if_rect_changes_id { + let mut shapes = shapes; + warn_if_rect_changes_id( + &mut shapes, + &viewport.prev_pass.widgets, + &viewport.this_pass.widgets, + ); + shapes + } else { + shapes + }; + std::mem::swap(&mut viewport.prev_pass, &mut viewport.this_pass); if repaint_needed { @@ -4237,6 +4250,103 @@ fn context_impl_send_sync() { assert_send_sync::(); } +/// Check if any [`Rect`] appears with different [`Id`]s between two passes. +/// +/// This helps detect cases where the same screen area is claimed by different widget ids +/// across passes, which is often a sign of id instability. +#[cfg(debug_assertions)] +fn warn_if_rect_changes_id( + out_shapes: &mut Vec, + prev_widgets: &crate::WidgetRects, + new_widgets: &crate::WidgetRects, +) { + profiling::function_scope!(); + + use std::collections::BTreeMap; + + /// A wrapper around [`Rect`] that implements [`Ord`] using the bit representation of its floats. + #[derive(Clone, Copy, PartialEq, Eq)] + struct OrderedRect(Rect); + + impl PartialOrd for OrderedRect { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } + } + + impl Ord for OrderedRect { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + let lhs = self.0; + let rhs = other.0; + lhs.min + .x + .to_bits() + .cmp(&rhs.min.x.to_bits()) + .then(lhs.min.y.to_bits().cmp(&rhs.min.y.to_bits())) + .then(lhs.max.x.to_bits().cmp(&rhs.max.x.to_bits())) + .then(lhs.max.y.to_bits().cmp(&rhs.max.y.to_bits())) + } + } + + fn create_lookup<'a>( + widgets: impl Iterator, + ) -> BTreeMap> { + let mut lookup: BTreeMap> = BTreeMap::default(); + for w in widgets { + lookup.entry(OrderedRect(w.rect)).or_default().push(w); + } + lookup + } + + for (layer_id, new_layer_widgets) in new_widgets.layers() { + let prev = create_lookup(prev_widgets.get_layer(*layer_id)); + let new = create_lookup(new_layer_widgets.iter()); + + for (hashable_rect, new_at_rect) in new { + let Some(prev_at_rect) = prev.get(&hashable_rect) else { + continue; // this rect did not exist in the previous pass + }; + + if prev_at_rect + .iter() + .any(|w| new_at_rect.iter().any(|nw| nw.id == w.id)) + { + continue; // at least one id stayed the same, so this is not an id change + } + + // Only warn if at least one of the previous ids is gone from this layer entirely. + // If they all still exist (just at a different rect), then the rect match + // is just a coincidence caused by widgets shifting (e.g. a window being dragged). + if prev_at_rect.iter().all(|w| new_widgets.contains(w.id)) { + continue; + } + + let rect = new_at_rect[0].rect; + + log::warn!( + "Widget rect {rect:?} changed id between passes: prev ids: {:?}, new ids: {:?}", + prev_at_rect + .iter() + .map(|w| w.id.short_debug_format()) + .collect::>(), + new_at_rect + .iter() + .map(|w| w.id.short_debug_format()) + .collect::>(), + ); + out_shapes.push(ClippedShape { + clip_rect: Rect::EVERYTHING, + shape: epaint::Shape::rect_stroke( + rect, + 0, + (2.0, Color32::RED), + StrokeKind::Outside, + ), + }); + } + } +} + #[cfg(test)] mod test { use super::Context; diff --git a/crates/egui/src/style.rs b/crates/egui/src/style.rs index 56a347f0d..5cd980f6c 100644 --- a/crates/egui/src/style.rs +++ b/crates/egui/src/style.rs @@ -1303,6 +1303,9 @@ pub struct DebugOptions { /// Show interesting widgets under the mouse cursor. pub show_widget_hits: bool, + /// Show a warning if the same `Rect` had different `Id` on the previous frame. + pub warn_if_rect_changes_id: bool, + /// If true, highlight widgets that are not aligned to [`emath::GUI_ROUNDING`]. /// /// See [`emath::GuiRounding`] for more. @@ -1329,6 +1332,7 @@ impl Default for DebugOptions { show_resize: false, show_interactive_widgets: false, show_widget_hits: false, + warn_if_rect_changes_id: cfg!(debug_assertions), show_unaligned: cfg!(debug_assertions), show_focused_widget: false, } @@ -2491,6 +2495,7 @@ impl DebugOptions { show_resize, show_interactive_widgets, show_widget_hits, + warn_if_rect_changes_id, show_unaligned, show_focused_widget, } = self; @@ -2522,6 +2527,11 @@ impl DebugOptions { ui.checkbox(show_widget_hits, "Show widgets under mouse pointer"); + ui.checkbox( + warn_if_rect_changes_id, + "Warn if a Rect changes Id between frames", + ); + ui.checkbox( show_unaligned, "Show rectangles not aligned to integer point coordinates", diff --git a/tests/egui_tests/tests/regression_tests.rs b/tests/egui_tests/tests/regression_tests.rs index 9e76394cb..421b69d35 100644 --- a/tests/egui_tests/tests/regression_tests.rs +++ b/tests/egui_tests/tests/regression_tests.rs @@ -1,4 +1,5 @@ use egui::accesskit::Role; +use egui::epaint::Shape; use egui::{Align, Color32, Image, Label, Layout, RichText, Sense, TextWrapMode, include_image}; use egui_kittest::Harness; use egui_kittest::kittest::Queryable as _; @@ -118,3 +119,47 @@ fn interact_on_ui_response_should_be_stable() { drop(harness); assert_eq!(click_count, 10, "We missed some clicks!"); } + +fn has_red_warning_rect(output: &egui::FullOutput) -> bool { + output.shapes.iter().any(|clipped| { + matches!( + &clipped.shape, + Shape::Rect(rect_shape) + if rect_shape.stroke.color == Color32::RED + ) + }) +} + +/// A button that changes its text on hover, with the Id derived from the text. +/// This is a plausible bug: the widget keeps the same rect, but its Id changes +/// between frames because the label (and thus the Id salt) changes on hover. +/// The `warn_if_rect_changes_id` debug check should catch this. +#[test] +fn warn_if_rect_changes_id() { + let button_rect = egui::Rect::from_min_size(egui::pos2(10.0, 10.0), egui::vec2(100.0, 30.0)); + + let mut harness = Harness::builder().with_size((200.0, 50.0)).build_ui(|ui| { + // Simulate a buggy widget whose Id depends on its label text, + // and the label changes on hover: + let is_hovered = ui.rect_contains_pointer(button_rect); + let label = if is_hovered { "Hovering!" } else { "Click me" }; + let id = ui.id().with(label); + let _response = ui.interact(button_rect, id, Sense::click()); + }); + + // no hover — establishes stable prev_pass + harness.step(); + assert!( + !has_red_warning_rect(harness.output()), + "Should not warn without hover" + ); + + // Move the pointer over the button + harness.hover_at(button_rect.center()); + + harness.step(); + assert!( + has_red_warning_rect(harness.output()), + "Should warn when a widget rect changes Id between passes" + ); +}