Use point/size in ClipBox instead of rect. (#1776)

This fixes a layout bug I was having, but I think it's also an
indication of a bigger issue.

The issue is that in various parts of the layout code, we expand
dimensions to the nearest display point. Besides the fact that we should
be using pixels intead of display points (IMO), this is numerically
unstable: if because of some numerical stuff you have a dimension that's
a rounding error larger than an integer number of pixels, the layout
"jumps".

What does this have to do with ClipBox? Well, ClipBox was storing its
layout rect as a `Rect`, which is stored as the coordinates of opposite
corners. When scrolling the ClipBox, the size should stay fixed but the
origin should move. But moving the origin can *change the size* very
slightly because of numerical errors converting to/from the Rect
representation. These slight errors got magnified by the pixel expansion
in other parts of the layout code.

This PR fixes the numerical imprecision in the ClipBox code, but I
suspect that the real fix is to eliminate the numerical instability.

Author: jneem

druid/src/scroll_component.rs

@@ -218,9 +218,9 @@ impl ScrollComponent {
     }
 
     fn calc_bar_bounds(&self, axis: Axis, port: &Viewport, env: &Env) -> Option<Rect> {
-        let viewport_size = port.rect.size();
+        let viewport_size = port.view_size;
         let content_size = port.content_size;
-        let scroll_offset = port.rect.origin().to_vec2();
+        let scroll_offset = port.view_origin.to_vec2();
 
         let viewport_major = axis.major(viewport_size);
         let content_major = axis.major(content_size);
@@ -265,7 +265,7 @@ impl ScrollComponent {
 
     /// Draw scroll bars.
     pub fn draw_bars(&self, ctx: &mut PaintCtx, port: &Viewport, env: &Env) {
-        let scroll_offset = port.rect.origin().to_vec2();
+        let scroll_offset = port.view_origin.to_vec2();
 
         if self.enabled.is_none() || self.opacity <= 0.0 {
             return;
@@ -312,8 +312,8 @@ impl ScrollComponent {
         if !self.enabled.is_enabled(Axis::Vertical) {
             return false;
         }
-        let viewport_size = port.rect.size();
-        let scroll_offset = port.rect.origin().to_vec2();
+        let viewport_size = port.view_size;
+        let scroll_offset = port.view_origin.to_vec2();
 
         if let Some(mut bounds) = self.calc_vertical_bar_bounds(port, env) {
             // Stretch hitbox to edge of widget
@@ -331,8 +331,8 @@ impl ScrollComponent {
         if !self.enabled.is_enabled(Axis::Horizontal) {
             return false;
         }
-        let viewport_size = port.rect.size();
-        let scroll_offset = port.rect.origin().to_vec2();
+        let viewport_size = port.view_size;
+        let scroll_offset = port.view_origin.to_vec2();
 
         if let Some(mut bounds) = self.calc_horizontal_bar_bounds(port, env) {
             // Stretch hitbox to edge of widget
@@ -347,9 +347,9 @@ impl ScrollComponent {
     ///
     /// Make sure to call on every event
     pub fn event(&mut self, port: &mut Viewport, ctx: &mut EventCtx, event: &Event, env: &Env) {
-        let viewport_size = port.rect.size();
+        let viewport_size = port.view_size;
         let content_size = port.content_size;
-        let scroll_offset = port.rect.origin().to_vec2();
+        let scroll_offset = port.view_origin.to_vec2();
 
         let scrollbar_is_hovered = match event {
             Event::MouseMove(e) | Event::MouseUp(e) | Event::MouseDown(e) => {
@@ -532,15 +532,19 @@ mod tests {
         scroll_component.enabled = ScrollbarsEnabled::Vertical;
         let viewport = Viewport {
             content_size: Size::new(100.0, 100.0),
-            rect: Rect::new(0.0, 25.0, 100.0, 75.0),
+            view_origin: (0.0, 25.0).into(),
+            view_size: (100.0, 50.0).into(),
         };
 
         let scrollbar_rect = scroll_component
             .calc_vertical_bar_bounds(&viewport, &test_env())
             .unwrap();
 
         assert!(
-            rect_contains(viewport.rect.inset(TEST_SCROLLBAR_PAD), scrollbar_rect),
+            rect_contains(
+                viewport.view_rect().inset(TEST_SCROLLBAR_PAD),
+                scrollbar_rect
+            ),
             "scrollbar should be contained by viewport"
         );
         assert_eq!(scrollbar_rect, Rect::new(86.0, 38.0, 97.0, 63.0));
@@ -552,22 +556,26 @@ mod tests {
         scroll_component.enabled = ScrollbarsEnabled::Vertical;
         let viewport = Viewport {
             content_size: Size::new(100.0, 100.0),
-            rect: Rect::new(0.0, 0.0, 100.0, 50.0),
+            view_origin: Point::ZERO,
+            view_size: (100.0, 50.0).into(),
         };
 
         let scrollbar_rect = scroll_component
             .calc_vertical_bar_bounds(&viewport, &test_env())
             .unwrap();
 
         assert!(
-            rect_contains(viewport.rect.inset(TEST_SCROLLBAR_PAD), scrollbar_rect),
+            rect_contains(
+                viewport.view_rect().inset(TEST_SCROLLBAR_PAD),
+                scrollbar_rect
+            ),
             "scrollbar should be contained by viewport"
         );
         // scrollbar should be at start of viewport
         approx_eq!(
             f64,
             scrollbar_rect.y0,
-            viewport.rect.y0 + TEST_SCROLLBAR_PAD
+            viewport.view_rect().y0 + TEST_SCROLLBAR_PAD
         );
         assert_eq!(scrollbar_rect, Rect::new(86.0, 3.0, 97.0, 28.0));
     }
@@ -578,22 +586,26 @@ mod tests {
         scroll_component.enabled = ScrollbarsEnabled::Vertical;
         let viewport = Viewport {
             content_size: Size::new(100.0, 100.0),
-            rect: Rect::new(0.0, 50.0, 100.0, 100.0),
+            view_origin: (0.0, 50.0).into(),
+            view_size: (100.0, 50.0).into(),
         };
 
         let scrollbar_rect = scroll_component
             .calc_vertical_bar_bounds(&viewport, &test_env())
             .unwrap();
 
         assert!(
-            rect_contains(viewport.rect.inset(TEST_SCROLLBAR_PAD), scrollbar_rect),
+            rect_contains(
+                viewport.view_rect().inset(TEST_SCROLLBAR_PAD),
+                scrollbar_rect
+            ),
             "scrollbar should be contained by viewport"
         );
         // scrollbar should be at end of viewport
         approx_eq!(
             f64,
             scrollbar_rect.y1,
-            viewport.rect.y1 - TEST_SCROLLBAR_PAD
+            viewport.view_rect().y1 - TEST_SCROLLBAR_PAD
         );
         assert_eq!(scrollbar_rect, Rect::new(86.0, 72.0, 97.0, 97.0));
     }
@@ -604,14 +616,15 @@ mod tests {
         scroll_component.enabled = ScrollbarsEnabled::Vertical;
         let mut viewport = Viewport {
             content_size: Size::new(100.0, 100.0),
-            rect: Rect::new(0.0, 25.0, 100.0, 75.0),
+            view_origin: (0.0, 25.0).into(),
+            view_size: (100.0, 50.0).into(),
         };
 
         let scrollbar_rect_1 = scroll_component
             .calc_vertical_bar_bounds(&viewport, &test_env())
             .unwrap();
 
-        viewport.rect = viewport.rect + Vec2::new(0.0, 15.0);
+        viewport.view_origin += Vec2::new(0.0, 15.0);
 
         let scrollbar_rect_2 = scroll_component
             .calc_vertical_bar_bounds(&viewport, &test_env())
@@ -630,19 +643,23 @@ mod tests {
         scroll_component.enabled = ScrollbarsEnabled::Both;
         let viewport = Viewport {
             content_size: Size::new(100.0, 100.0),
-            rect: Rect::new(0.0, 50.0, 100.0, 100.0),
+            view_origin: (0.0, 50.0).into(),
+            view_size: (100.0, 50.0).into(),
         };
 
         let scrollbar_rect = scroll_component
             .calc_vertical_bar_bounds(&viewport, &test_env())
             .unwrap();
 
         assert!(
-            rect_contains(viewport.rect.inset(TEST_SCROLLBAR_PAD), scrollbar_rect),
+            rect_contains(
+                viewport.view_rect().inset(TEST_SCROLLBAR_PAD),
+                scrollbar_rect
+            ),
             "scrollbar should be contained by viewport"
         );
         assert!(
-            scrollbar_rect.y1 + TEST_SCROLLBAR_WIDTH <= viewport.rect.y1,
+            scrollbar_rect.y1 + TEST_SCROLLBAR_WIDTH <= viewport.view_rect().y1,
             "vertical scrollbar should leave space for the horizontal scrollbar when both enabled"
         );
         assert_eq!(scrollbar_rect, Rect::new(86.0, 61.0, 97.0, 86.0));
@@ -654,15 +671,19 @@ mod tests {
         scroll_component.enabled = ScrollbarsEnabled::Vertical;
         let viewport = Viewport {
             content_size: Size::new(100.0, 1000.0),
-            rect: Rect::new(0.0, 25.0, 100.0, 75.0),
+            view_origin: (0.0, 25.0).into(),
+            view_size: (100.0, 50.0).into(),
         };
 
         let scrollbar_rect = scroll_component
             .calc_vertical_bar_bounds(&viewport, &test_env())
             .unwrap();
 
         assert!(
-            rect_contains(viewport.rect.inset(TEST_SCROLLBAR_PAD), scrollbar_rect),
+            rect_contains(
+                viewport.view_rect().inset(TEST_SCROLLBAR_PAD),
+                scrollbar_rect
+            ),
             "scrollbar should be contained by viewport"
         );
         // scrollbar should use SCROLLBAR_MIN_SIZE when content is much bigger than viewport
@@ -676,27 +697,31 @@ mod tests {
         scroll_component.enabled = ScrollbarsEnabled::Vertical;
         let viewport = Viewport {
             content_size: Size::new(100.0, 100.0),
-            rect: Rect::new(0.0, 25.0, 100.0, 35.0),
+            view_origin: (0.0, 25.0).into(),
+            view_size: (100.0, 10.0).into(),
         };
 
         let scrollbar_rect = scroll_component
             .calc_vertical_bar_bounds(&viewport, &test_env())
             .unwrap();
 
         assert!(
-            rect_contains(viewport.rect.inset(TEST_SCROLLBAR_PAD), scrollbar_rect),
+            rect_contains(
+                viewport.view_rect().inset(TEST_SCROLLBAR_PAD),
+                scrollbar_rect
+            ),
             "scrollbar should be contained by viewport"
         );
         // scrollbar should fill viewport if too small for SCROLLBAR_MIN_SIZE
         approx_eq!(
             f64,
             scrollbar_rect.y0,
-            viewport.rect.y0 + TEST_SCROLLBAR_PAD
+            viewport.view_rect().y0 + TEST_SCROLLBAR_PAD
         );
         approx_eq!(
             f64,
             scrollbar_rect.y1,
-            viewport.rect.y1 - TEST_SCROLLBAR_PAD
+            viewport.view_rect().y1 - TEST_SCROLLBAR_PAD
         );
     }
 
@@ -706,7 +731,8 @@ mod tests {
         scroll_component.enabled = ScrollbarsEnabled::Vertical;
         let viewport = Viewport {
             content_size: Size::new(100.0, 100.0),
-            rect: Rect::new(0.0, 25.0, 100.0, 28.0),
+            view_origin: (0.0, 25.0).into(),
+            view_size: (100.0, 3.0).into(),
         };
 
         let scrollbar_rect = scroll_component.calc_vertical_bar_bounds(&viewport, &test_env());

druid/src/widget/clip_box.rs

@@ -23,11 +23,18 @@ use tracing::{instrument, trace};
 pub struct Viewport {
     /// The size of the area that we have a viewport into.
     pub content_size: Size,
-    /// The view rectangle.
-    pub rect: Rect,
+    /// The origin of the view rectangle.
+    pub view_origin: Point,
+    /// The size of the view rectangle.
+    pub view_size: Size,
 }
 
 impl Viewport {
+    /// The view rectangle.
+    pub fn view_rect(&self) -> Rect {
+        Rect::from_origin_size(self.view_origin, self.view_size)
+    }
+
     /// Tries to find a position for the view rectangle that is contained in the content rectangle.
     ///
     /// If the supplied origin is good, returns it; if it isn't, we try to return the nearest
@@ -37,11 +44,11 @@ impl Viewport {
     pub fn clamp_view_origin(&self, origin: Point) -> Point {
         let x = origin
             .x
-            .min(self.content_size.width - self.rect.width())
+            .min(self.content_size.width - self.view_size.width)
             .max(0.0);
         let y = origin
             .y
-            .min(self.content_size.height - self.rect.height())
+            .min(self.content_size.height - self.view_size.height)
             .max(0.0);
         Point::new(x, y)
     }
@@ -54,7 +61,7 @@ impl Viewport {
     /// bottom of the child widget, then the offset will not change and this function will return
     /// false.
     pub fn pan_by(&mut self, delta: Vec2) -> bool {
-        self.pan_to(self.rect.origin() + delta)
+        self.pan_to(self.view_origin + delta)
     }
 
     /// Sets the viewport origin to `pos`, while trying to keep the view rectangle inside the
@@ -65,8 +72,8 @@ impl Viewport {
     /// `pos`.
     pub fn pan_to(&mut self, origin: Point) -> bool {
         let new_origin = self.clamp_view_origin(origin);
-        if (new_origin - self.rect.origin()).hypot2() > 1e-12 {
-            self.rect = self.rect.with_origin(new_origin);
+        if (new_origin - self.view_origin).hypot2() > 1e-12 {
+            self.view_origin = new_origin;
             true
         } else {
             false
@@ -98,19 +105,20 @@ impl Viewport {
         // this means we will show the portion of the target region that
         // includes the origin.
         let target_size = Size::new(
-            rect.width().min(self.rect.width()),
-            rect.height().min(self.rect.height()),
+            rect.width().min(self.view_size.width),
+            rect.height().min(self.view_size.height),
         );
         let rect = rect.with_size(target_size);
 
-        let x0 = closest_on_axis(rect.min_x(), self.rect.min_x(), self.rect.max_x());
-        let x1 = closest_on_axis(rect.max_x(), self.rect.min_x(), self.rect.max_x());
-        let y0 = closest_on_axis(rect.min_y(), self.rect.min_y(), self.rect.max_y());
-        let y1 = closest_on_axis(rect.max_y(), self.rect.min_y(), self.rect.max_y());
+        let my_rect = self.view_rect();
+        let x0 = closest_on_axis(rect.min_x(), my_rect.min_x(), my_rect.max_x());
+        let x1 = closest_on_axis(rect.max_x(), my_rect.min_x(), my_rect.max_x());
+        let y0 = closest_on_axis(rect.min_y(), my_rect.min_y(), my_rect.max_y());
+        let y1 = closest_on_axis(rect.max_y(), my_rect.min_y(), my_rect.max_y());
 
         let delta_x = if x0.abs() > x1.abs() { x0 } else { x1 };
         let delta_y = if y0.abs() > y1.abs() { y0 } else { y1 };
-        let new_origin = self.rect.origin() + Vec2::new(delta_x, delta_y);
+        let new_origin = self.view_origin + Vec2::new(delta_x, delta_y);
         self.pan_to(new_origin)
     }
 }
@@ -180,15 +188,15 @@ impl<T, W> ClipBox<T, W> {
 
     /// Returns the origin of the viewport rectangle.
     pub fn viewport_origin(&self) -> Point {
-        self.port.rect.origin()
+        self.port.view_origin
     }
 
     /// Returns the size of the rectangular viewport into the child widget.
     /// To get the position of the viewport, see [`viewport_origin`].
     ///
     /// [`viewport_origin`]: struct.ClipBox.html#method.viewport_origin
     pub fn viewport_size(&self) -> Size {
-        self.port.rect.size()
+        self.port.view_size
     }
 
     /// Returns the size of the child widget.
@@ -349,7 +357,7 @@ impl<T: Data, W: Widget<T>> Widget<T> for ClipBox<T, W> {
         self.port.content_size = content_size;
         self.child.set_origin(ctx, data, env, Point::ORIGIN);
 
-        self.port.rect = self.port.rect.with_size(bc.constrain(content_size));
+        self.port.view_size = bc.constrain(content_size);
         let new_offset = self.port.clamp_view_origin(self.viewport_origin());
         self.pan_to(new_offset);
         trace!("Computed sized: {}", self.viewport_size());
@@ -380,19 +388,20 @@ mod tests {
     fn pan_to_visible() {
         let mut viewport = Viewport {
             content_size: Size::new(400., 400.),
-            rect: Rect::from_origin_size((20., 20.), (20., 20.)),
+            view_size: (20., 20.).into(),
+            view_origin: (20., 20.).into(),
         };
 
         assert!(!viewport.pan_to_visible(Rect::from_origin_size((22., 22.,), (5., 5.))));
         assert!(viewport.pan_to_visible(Rect::from_origin_size((10., 10.,), (5., 5.))));
-        assert_eq!(viewport.rect.origin(), Point::new(10., 10.));
-        assert_eq!(viewport.rect.size(), Size::new(20., 20.));
+        assert_eq!(viewport.view_origin, Point::new(10., 10.));
+        assert_eq!(viewport.view_size, Size::new(20., 20.));
         assert!(!viewport.pan_to_visible(Rect::from_origin_size((10., 10.,), (50., 50.))));
-        assert_eq!(viewport.rect.origin(), Point::new(10., 10.));
+        assert_eq!(viewport.view_origin, Point::new(10., 10.));
 
         assert!(viewport.pan_to_visible(Rect::from_origin_size((30., 10.,), (5., 5.))));
-        assert_eq!(viewport.rect.origin(), Point::new(15., 10.));
+        assert_eq!(viewport.view_origin, Point::new(15., 10.));
         assert!(viewport.pan_to_visible(Rect::from_origin_size((5., 5.,), (5., 5.))));
-        assert_eq!(viewport.rect.origin(), Point::new(5., 5.));
+        assert_eq!(viewport.view_origin, Point::new(5., 5.));
     }
 }