Merge lp:~afrantzis/mir/rectangle-closed-open into lp:~mir-team/mir/trunk

[Alexandros Frantzis]

geometry: Use an closed-open representation for Rectangle end points

geometry: Use an closed-open representation for Rectangle end points

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:881
http://jenkins.qa.ubuntu.com/job/mir-ci/1081/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1412
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1297
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/319
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/319/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/699

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1081/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

I think the changes to the displacement/rectangles is ok, I'm still not sure though how the input coordinate system meshes with the graphics coordinate system though after this. The changes make sense with just graphics coordinates.

particularly, I think the way that these tests (tests/unit-tests/surfaces/test_surface_data.cpp) where before make more sense if the input events come in float/double coordinates instead of integer ones. Robert C, any thoughts on the matter? :)

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> particularly, I think the way that these tests (tests/unit-tests/surfaces/test_surface_data.cpp)
> where before make more sense if the input events come in float/double coordinates instead of
> integer ones. Robert C, any thoughts on the matter? :)

> // a 1x1 window at (1,1) will get events at (1,1), (1,2) (2,1), (2,2)

This indicates a coordinate system that's closed at both ends ([P1,P2]), with whole values (eg (0,0)) at the center of a pixels. There doesn't seem to be a sane way to express that the input region is a single pixel with this system (assuming integer values, which is what we are using in the external interfaces input has).

The new system is [P1,P2) with points between pixels (pixel center at .5 values) and I think it's a more versatile representation. However, I am not not familiar with android input peculiarities, either, so let's see what Robert has to say. Even if there is a need for a different system for input, I would propose that the input stack continues to use the global mir system for external communications and do any conversions internally. Otherwise we will end up with subtle communication problems between input and the rest of the system.

[Robert Carr (robertcarr)]

Coordinates are a little weird in the input stack. With pointer coordinates at times having floating point values, but some operations (such as hit testing) seem to involve the InputDispatcher casting to integer, etc...

This is an item that needs some work for me.

In the mean time this LGTM!

[Robert Carr (robertcarr)]

review: Approve

[Kevin DuBois (kdub)]

> > particularly, I think the way that these tests (tests/unit-
> tests/surfaces/test_surface_data.cpp)
> > where before make more sense if the input events come in float/double
> coordinates instead of
> > integer ones. Robert C, any thoughts on the matter? :)
>
> > // a 1x1 window at (1,1) will get events at (1,1), (1,2) (2,1), (2,2)
>
> This indicates a coordinate system that's closed at both ends ([P1,P2]), with
> whole values (eg (0,0)) at the center of a pixels. There doesn't seem to be a
> sane way to express that the input region is a single pixel with this system
> (assuming integer values, which is what we are using in the external
> interfaces input has).
>
> The new system is [P1,P2) with points between pixels (pixel center at .5
> values) and I think it's a more versatile representation. However, I am not
> not familiar with android input peculiarities, either, so let's see what
> Robert has to say. Even if there is a need for a different system for input, I
> would propose that the input stack continues to use the global mir system for
> external communications and do any conversions internally. Otherwise we will
> end up with subtle communication problems between input and the rest of the
> system.

I think we both want 0.5, 0.5 at the middle of the pixel. The only difference was which edges are included in the pixel or not. This solution makes the top and left edges included in the pixel, and the bottom/right not included. I'm ok with this, as each point would correspond to just one pixel.
Since its a bit fuzzier on the input side though, I think this is good to go

review: Approve

=== modified file 'include/shared/mir/geometry/displacement.h'
--- include/shared/mir/geometry/displacement.h	2013-07-18 06:44:57 +0000
+++ include/shared/mir/geometry/displacement.h	2013-07-23 09:49:30 +0000
@@ -31,13 +31,20 @@
 
 struct Displacement
 {
-    DeltaX dx;
-    DeltaY dy;
+    Displacement() {}
+    Displacement(Displacement const&) = default;
+    Displacement& operator=(Displacement const&) = default;
+
+    template<typename DeltaXType, typename DeltaYType>
+    Displacement(DeltaXType&& dx, DeltaYType&& dy) : dx{dx}, dy{dy} {}
 
     float length_squared() const
     {
         return dx.as_float() * dx.as_float() + dy.as_float() * dy.as_float();
     }
+
+    DeltaX dx;
+    DeltaY dy;
 };
 
 inline bool operator==(Displacement const& lhs, Displacement const& rhs)

=== modified file 'include/shared/mir/geometry/rectangle.h'
--- include/shared/mir/geometry/rectangle.h	2013-07-18 06:44:57 +0000
+++ include/shared/mir/geometry/rectangle.h	2013-07-23 09:49:30 +0000
@@ -35,6 +35,12 @@
     Point top_left;
     Size size;
 
+    /** 
+     * The bottom right boundary point of the rectangle.
+     *
+     * Note that the returned point is *not* included in the rectangle
+     * area, that is, the rectangle is represented as [top_left,bottom_right).
+     */
     Point bottom_right() const;
     bool contains(Point const& p) const;
 };

=== modified file 'src/server/shell/graphics_display_layout.cpp'
--- src/server/shell/graphics_display_layout.cpp	2013-07-19 14:34:20 +0000
+++ src/server/shell/graphics_display_layout.cpp	2013-07-23 09:49:30 +0000
@@ -22,6 +22,7 @@
 
 #include "mir/geometry/rectangle.h"
 #include "mir/geometry/rectangles.h"
+#include "mir/geometry/displacement.h"
 
 namespace msh = mir::shell;
 namespace mg = mir::graphics;
@@ -41,16 +42,16 @@
         rect.size.width > geom::Width{0} && rect.size.height > geom::Height{0})
     {
         auto tl = rect.top_left;
-        auto br = rect.bottom_right();
+        auto br_closed = rect.bottom_right() - geom::Displacement{1,1};
 
         geom::Rectangles rectangles;
         rectangles.add(output);
 
-        rectangles.confine(br);
+        rectangles.confine(br_closed);
 
         rect.size =
-            geom::Size{br.x.as_int() - tl.x.as_int() + 1,
-                       br.y.as_int() - tl.y.as_int() + 1};
+            geom::Size{br_closed.x.as_int() - tl.x.as_int() + 1,
+                       br_closed.y.as_int() - tl.y.as_int() + 1};
     }
     else
     {

=== modified file 'src/server/surfaces/surface_data.cpp'
--- src/server/surfaces/surface_data.cpp	2013-07-16 15:49:19 +0000
+++ src/server/surfaces/surface_data.cpp	2013-07-23 09:49:30 +0000
@@ -153,35 +153,11 @@
     notify_change();
 }
 
-namespace
-{
-
-bool rectangle_contains_point(geom::Rectangle const& rectangle, uint32_t px, uint32_t py)
-{
-    auto width = rectangle.size.width.as_uint32_t();
-    auto height = rectangle.size.height.as_uint32_t();
-    auto x = rectangle.top_left.x.as_uint32_t();
-    auto y = rectangle.top_left.y.as_uint32_t();
-
-    //TODO: what if (width == 0) || (height == 0) ?
-    
-    if (px < x)
-        return false;
-    else if (py < y)
-        return false;
-    else if (px > x + width)   
-        return false;
-    else if (py > y + height)
-        return false;
-    return true;
-}
-
-}
 bool ms::SurfaceData::contains(geom::Point const& point) const
 {
     for (auto const& rectangle : input_rectangles)
     {
-        if (rectangle_contains_point(rectangle, point.x.as_uint32_t(), point.y.as_uint32_t()))
+        if (rectangle.contains(point))
         {
             return true;
         } 

=== modified file 'src/shared/geometry/rectangle.cpp'
--- src/shared/geometry/rectangle.cpp	2013-07-18 06:44:57 +0000
+++ src/shared/geometry/rectangle.cpp	2013-07-23 09:49:30 +0000
@@ -24,9 +24,8 @@
 
 geom::Point geom::Rectangle::bottom_right() const
 {
-    assert(size.width > geom::Width{0} && size.height > geom::Height{0});
-    return {top_left.x.as_int() + size.width.as_int() - 1,
-            top_left.y.as_int() + size.height.as_int() - 1};
+    return {top_left.x.as_int() + size.width.as_int(),
+            top_left.y.as_int() + size.height.as_int()};
 }
 
 bool geom::Rectangle::contains(Point const& p) const
@@ -35,6 +34,6 @@
         return false;
 
     auto br = bottom_right();
-    return p.x >= top_left.x && p.x <= br.x &&
-           p.y >= top_left.y && p.y <= br.y;
+    return p.x >= top_left.x && p.x < br.x &&
+           p.y >= top_left.y && p.y < br.y;
 }

=== modified file 'src/shared/geometry/rectangles.cpp'
--- src/shared/geometry/rectangles.cpp	2013-07-19 14:12:57 +0000
+++ src/shared/geometry/rectangles.cpp	2013-07-23 09:49:30 +0000
@@ -35,8 +35,8 @@
 geom::Rectangle rect_from_points(geom::Point const& tl, geom::Point const& br)
 {
     return {tl,
-            geom::Size{geom::Width{br.x.as_int() - tl.x.as_int() + 1},
-                       geom::Height{br.y.as_int() - tl.y.as_int() + 1}}};
+            geom::Size{geom::Width{br.x.as_int() - tl.x.as_int()},
+                       geom::Height{br.y.as_int() - tl.y.as_int()}}};
 }
 
 }
@@ -78,9 +78,9 @@
         {
             auto br = rect.bottom_right();
             auto min_x = rect.top_left.x;
-            auto max_x = br.x;
+            auto max_x = geom::X{br.x.as_int() - 1};
             auto min_y = rect.top_left.y;
-            auto max_y = br.y;
+            auto max_y = geom::Y{br.y.as_int() - 1};
 
             geom::Point confined_point{clamp(point.x, min_x, max_x),
                                        clamp(point.y, min_y, max_y)};
@@ -113,25 +113,21 @@
 
     for (auto const& rect : rectangles)
     {
-        if (rect.size.width.as_int() > 0 &&
-            rect.size.height.as_int() > 0)
-        {
-            geom::Point rtl = rect.top_left;
-            geom::Point rbr = rect.bottom_right();
+        geom::Point rtl = rect.top_left;
+        geom::Point rbr = rect.bottom_right();
 
-            if (!points_initialized)
-            {
-                tl = rtl;
-                br = rbr;
-                points_initialized = true;
-            }
-            else
-            {
-                tl.x = std::min(rtl.x, tl.x);
-                tl.y = std::min(rtl.y, tl.y);
-                br.x = std::max(rbr.x, br.x);
-                br.y = std::max(rbr.y, br.y);
-            }
+        if (!points_initialized)
+        {
+            tl = rtl;
+            br = rbr;
+            points_initialized = true;
+        }
+        else
+        {
+            tl.x = std::min(rtl.x, tl.x);
+            tl.y = std::min(rtl.y, tl.y);
+            br.x = std::max(rbr.x, br.x);
+            br.y = std::max(rbr.y, br.y);
         }
     }
 

=== modified file 'tests/acceptance-tests/test_client_input.cpp'
--- tests/acceptance-tests/test_client_input.cpp	2013-07-19 14:27:14 +0000
+++ tests/acceptance-tests/test_client_input.cpp	2013-07-23 09:49:30 +0000
@@ -370,8 +370,8 @@
         {
             wait_until_client_appears(test_client_name);
             
-            fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(InputClient::surface_width,
-                                                                                 InputClient::surface_height));
+            fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(InputClient::surface_width - 1,
+                                                                                 InputClient::surface_height - 1));
             fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(2,2));
         }
     } server_config;
@@ -390,8 +390,8 @@
             // We should see the cursor enter
             EXPECT_CALL(*handler, handle_input(HoverEnterEvent())).Times(1);
             EXPECT_CALL(*handler, handle_input(
-                MotionEventWithPosition(InputClient::surface_width,
-                                        InputClient::surface_height))).Times(1)
+                MotionEventWithPosition(InputClient::surface_width - 1,
+                                        InputClient::surface_height - 1))).Times(1)
                 .WillOnce(mt::WakeUp(&events_received));
             // But we should not receive an event for the second movement outside of our surface!
         }

=== modified file 'tests/unit-tests/geometry/test-rectangle.cpp'
--- tests/unit-tests/geometry/test-rectangle.cpp	2013-07-11 16:54:41 +0000
+++ tests/unit-tests/geometry/test-rectangle.cpp	2013-07-23 09:49:30 +0000
@@ -43,3 +43,31 @@
     EXPECT_EQ(Size(), default_rect.size);
     EXPECT_NE(rect, default_rect);
 }
+
+TEST(geometry, rectangle_bottom_right)
+{
+    using namespace testing;
+    using namespace geom;
+
+    Rectangle const rect{{0,0}, {1,1}};
+    Rectangle const rect_empty{{2,2}, {0,0}};
+
+    EXPECT_EQ(Point(1,1), rect.bottom_right());
+    EXPECT_EQ(Point(2,2), rect_empty.bottom_right());
+}
+
+TEST(geometry, rectangle_contains)
+{
+    using namespace testing;
+    using namespace geom;
+
+    Rectangle const rect{{0,0}, {1,1}};
+    Rectangle const rect_empty{{2,2}, {0,0}};
+
+    EXPECT_TRUE(rect.contains({0,0}));
+    EXPECT_FALSE(rect.contains({0,1}));
+    EXPECT_FALSE(rect.contains({1,0}));
+    EXPECT_FALSE(rect.contains({1,1}));
+
+    EXPECT_FALSE(rect_empty.contains({2,2}));
+}

=== modified file 'tests/unit-tests/surfaces/test_surface_data.cpp'
--- tests/unit-tests/surfaces/test_surface_data.cpp	2013-07-18 14:41:31 +0000
+++ tests/unit-tests/surfaces/test_surface_data.cpp	2013-07-23 09:49:30 +0000
@@ -188,17 +188,17 @@
     surface_state.frame_posted();
 }
 
-// a 1x1 window at (1,1) will get events at (1,1), (1,2) (2,1), (2,2)
+// a 1x1 window at (1,1) will get events at (1,1)
 TEST_F(SurfaceDataTest, default_region_is_surface_rectangle)
 {
     geom::Point pt(1,1);
     geom::Size one_by_one{geom::Width{1}, geom::Height{1}};
     ms::SurfaceData surface_state{name, geom::Rectangle{pt, one_by_one}, mock_change_cb};
 
-    std::initializer_list <geom::Point> contained_pt{geom::Point{geom::X{1}, geom::Y{1}},
-                                       geom::Point{geom::X{1}, geom::Y{2}},
-                                       geom::Point{geom::X{2}, geom::Y{1}},
-                                       geom::Point{geom::X{2}, geom::Y{2}}};
+    std::vector<geom::Point> contained_pt
+    {
+        geom::Point{geom::X{1}, geom::Y{1}}
+    };
 
     for(auto x = 0; x <= 3; x++)
     {
@@ -228,17 +228,13 @@
     ms::SurfaceData surface_state{name, rect, mock_change_cb};
     surface_state.set_input_region(rectangles);
 
-    std::initializer_list <geom::Point> contained_pt{//region0 points
-                                                     geom::Point{geom::X{0}, geom::Y{0}},
-                                                     geom::Point{geom::X{0}, geom::Y{1}},
-                                                     geom::Point{geom::X{1}, geom::Y{0}},
-                                                     //overlapping point
-                                                     geom::Point{geom::X{1}, geom::Y{1}},
-                                                     //region1 points
-                                                     geom::Point{geom::X{1}, geom::Y{2}},
-                                                     geom::Point{geom::X{2}, geom::Y{1}},
-                                                     geom::Point{geom::X{2}, geom::Y{2}},
-                                                    };
+    std::vector<geom::Point> contained_pt
+    {
+        //region0 points
+        geom::Point{geom::X{0}, geom::Y{0}},
+        //region1 points
+        geom::Point{geom::X{1}, geom::Y{1}},
+    };
 
     for(auto x = 0; x <= 3; x++)
     {