Selector tool: improve responsiveness for snapping a path's internal intersections (was unbearable already for paths having 20+ segments)

(bzr r11937)

7 files changed, 28 insertions, 67 deletions

diff --git a/src/selection.cpp b/src/selection.cpp
index d769b402c..72f50137c 100644
--- a/src/selection.cpp
+++ b/src/selection.cpp
@@ -443,15 +443,13 @@ std::vector<Inkscape::SnapCandidatePoint> Selection::getSnapPoints(SnapPreferenc
 
     SnapPreferences snapprefs_dummy = *snapprefs; // create a local copy of the snapping prefs
     snapprefs_dummy.setTargetSnappable(Inkscape::SNAPTARGET_ROTATION_CENTER, false); // locally disable snapping to the item center
-    //snapprefs_dummy.setTargetSnappable(Inkscape::SNAPTARGET_NODE_CUSP, true); // consider any type of nodes as a snap source
-    //snapprefs_dummy.setTargetSnappable(Inkscape::SNAPTARGET_NODE_SMOOTH, true); // i.e. disregard the smooth / cusp node preference
     std::vector<Inkscape::SnapCandidatePoint> p;
     for (GSList const *iter = items; iter != NULL; iter = iter->next) {
         SPItem *this_item = SP_ITEM(iter->data);
         this_item->getSnappoints(p, &snapprefs_dummy);
 
         //Include the transformation origin for snapping
-        //For a selection or group only the overall origin is considered
+        //For a selection or group only the overall center is considered, not for each item individually
         if (snapprefs != NULL && snapprefs->isTargetSnappable(Inkscape::SNAPTARGET_ROTATION_CENTER)) {
             p.push_back(Inkscape::SnapCandidatePoint(this_item->getCenter(), SNAPSOURCE_ROTATION_CENTER));
         }
@@ -460,36 +458,6 @@ std::vector<Inkscape::SnapCandidatePoint> Selection::getSnapPoints(SnapPreferenc
     return p;
 }
 
-// TODO: both getSnapPoints and getSnapPointsConvexHull are called, subsequently. Can we do this more efficient?
-// Why do we need to include the transformation center in one case and not the other?
-std::vector<Inkscape::SnapCandidatePoint> Selection::getSnapPointsConvexHull(SnapPreferences const *snapprefs) const {
-    GSList const *items = const_cast<Selection *>(this)->itemList();
-
-    SnapPreferences snapprefs_dummy = *snapprefs; // create a local copy of the snapping prefs
-    snapprefs_dummy.setTargetSnappable(Inkscape::SNAPTARGET_NODE_CUSP, true); // consider any type of nodes as a snap source
-    snapprefs_dummy.setTargetSnappable(Inkscape::SNAPTARGET_NODE_SMOOTH, true); // i.e. disregard the smooth / cusp node preference
-
-    std::vector<Inkscape::SnapCandidatePoint> p;
-    for (GSList const *iter = items; iter != NULL; iter = iter->next) {
-        SP_ITEM(iter->data)->getSnappoints(p, &snapprefs_dummy);
-    }
-
-    std::vector<Inkscape::SnapCandidatePoint> pHull;
-    if (!p.empty()) {
-        Geom::Rect cvh((p.front()).getPoint(), (p.front()).getPoint());
-        for (std::vector<Inkscape::SnapCandidatePoint>::iterator i = p.begin(); i != p.end(); ++i) {
-            // these are the points we get back
-            cvh.expandTo((*i).getPoint());
-        }
-
-        for ( unsigned i = 0 ; i < 4 ; ++i ) {
-            pHull.push_back(Inkscape::SnapCandidatePoint(cvh.corner(i), SNAPSOURCE_CONVEX_HULL_CORNER));
-        }
-    }
-
-    return pHull;
-}
-
 void Selection::_removeObjectDescendants(SPObject *obj) {
     GSList *iter, *next;
     for ( iter = _objs ; iter ; iter = next ) {
diff --git a/src/selection.h b/src/selection.h
index 168c70dd4..85e06a5c6 100644
--- a/src/selection.h
+++ b/src/selection.h
@@ -267,12 +267,6 @@ public:
     std::vector<Inkscape::SnapCandidatePoint> getSnapPoints(SnapPreferences const *snapprefs) const;
 
     /**
-     * Gets the snap points of a selection that form a convex hull.
-     * @return Selection's convex hull points
-     */
-    std::vector<Inkscape::SnapCandidatePoint> getSnapPointsConvexHull(SnapPreferences const *snapprefs) const;
-
-    /**
      * Connects a slot to be notified of selection changes.
      *
      * This method connects the given slot such that it will
diff --git a/src/seltrans.cpp b/src/seltrans.cpp
index 15cea9198..64bb95508 100644
--- a/src/seltrans.cpp
+++ b/src/seltrans.cpp
@@ -298,30 +298,21 @@ void Inkscape::SelTrans::grab(Geom::Point const &p, gdouble x, gdouble y, bool s
     // Next, get all points to consider for snapping
     SnapManager const &m = _desktop->namedview->snap_manager;
     _snap_points.clear();
-    _snap_points = selection->getSnapPoints(&m.snapprefs);
-    std::vector<Inkscape::SnapCandidatePoint> snap_points_hull = selection->getSnapPointsConvexHull(&m.snapprefs);
+    if (m.someSnapperMightSnap(false)) { // Only search for snap sources when really needed, to avoid unnecessary delays
+        _snap_points = selection->getSnapPoints(&m.snapprefs); // This might take some time!
+    }
     if (_snap_points.size() > 200 && !(prefs->getBool("/options/snapclosestonly/value", false))) {
         /* Snapping a huge number of nodes will take way too long, so limit the number of snappable nodes
         A typical user would rarely ever try to snap such a large number of nodes anyway, because
         (s)he would hardly be able to discern which node would be snapping */
-        _snap_points = snap_points_hull;
-        //}
+        _snap_points.resize(200);
         // Unfortunately, by now we will have lost the font-baseline snappoints :-(
     }
 
     // Find bbox hulling all special points, which excludes stroke width. Here we need to include the
     // path nodes, for example because a rectangle which has been converted to a path doesn't have
     // any other special points
-    Geom::Rect snap_points_bbox;
-    if ( snap_points_hull.empty() == false ) {
-        std::vector<Inkscape::SnapCandidatePoint>::iterator i = snap_points_hull.begin();
-        snap_points_bbox = Geom::Rect((*i).getPoint(), (*i).getPoint());
-        i++;
-        while (i != snap_points_hull.end()) {
-            snap_points_bbox.expandTo((*i).getPoint());
-            i++;
-        }
-    }
+    Geom::OptRect snap_points_bbox = selection->bounds(SPItem::GEOMETRIC_BBOX);
 
     _bbox_points.clear();
     // Collect the bounding box's corners and midpoints for each selected item
@@ -329,11 +320,8 @@ void Inkscape::SelTrans::grab(Geom::Point const &p, gdouble x, gdouble y, bool s
         bool c = m.snapprefs.isTargetSnappable(SNAPTARGET_BBOX_CORNER);
         bool mp = m.snapprefs.isTargetSnappable(SNAPTARGET_BBOX_MIDPOINT);
         bool emp = m.snapprefs.isTargetSnappable(SNAPTARGET_BBOX_EDGE_MIDPOINT);
-        // 1) Preferably we'd use the bbox of each selected item, instead of the bbox of the selection as a whole; for translations
-        // this is easy to do, but when snapping the visual bbox while scaling we will have to compensate for the scaling of the
-        // stroke width. (see get_scale_transform_for_stroke()). This however is currently only implemented for a single bbox.
-        // 2) More than 50 items will produce at least 200 bbox points, which might make Inkscape crawl
-        // (see the comment a few lines above). In that case we will use the bbox of the selection as a whole
+        // Preferably we'd use the bbox of each selected item, but for example 50 items will produce at least 200 bbox points,
+        // which might make Inkscape crawl(see the comment a few lines above). In that case we will use the bbox of the selection as a whole
         bool c1 = (_items.size() > 0) && (_items.size() < 50);
         bool c2 = prefs->getBool("/options/snapclosestonly/value", false);
         if (translating && (c1 || c2)) {
@@ -353,10 +341,14 @@ void Inkscape::SelTrans::grab(Geom::Point const &p, gdouble x, gdouble y, bool s
         //  - one for snapping the boundingbox, which can be either visual or geometric
         //  - one for snapping the special points
         // The "opposite" in case of a geometric boundingbox always coincides with the "opposite" for the special points
-        // These distinct "opposites" are needed in the snapmanager to avoid bugs such as #sf1540195 (in which
+        // These distinct "opposites" are needed in the snapmanager to avoid bugs such as LP167905 (in which
         // a box is caught between two guides)
         _opposite_for_bboxpoints = _bbox->min() + _bbox->dimensions() * Geom::Scale(1-x, 1-y);
-        _opposite_for_specpoints = snap_points_bbox.min() + snap_points_bbox.dimensions() * Geom::Scale(1-x, 1-y);
+        if (snap_points_bbox) {
+            _opposite_for_specpoints = (*snap_points_bbox).min() + (*snap_points_bbox).dimensions() * Geom::Scale(1-x, 1-y);
+        } else {
+            _opposite_for_specpoints = _opposite_for_bboxpoints;
+        }
         _opposite = _opposite_for_bboxpoints;
     }
 
diff --git a/src/snap-candidate.h b/src/snap-candidate.h
index da65d4ea3..8bb7cb52f 100644
--- a/src/snap-candidate.h
+++ b/src/snap-candidate.h
@@ -25,6 +25,8 @@ namespace Inkscape {
 class SnapCandidatePoint
 {
 public:
+    SnapCandidatePoint() {}; // only needed / used for resizing() of a vector in seltrans.cpp; do not use uninitialized instances!
+
     SnapCandidatePoint(Geom::Point const &point, Inkscape::SnapSourceType const source, long const source_num, Inkscape::SnapTargetType const target, Geom::OptRect const &bbox)
         : _point(point),
         _source_type(source),
@@ -39,7 +41,7 @@ public:
         : _point(point),
         _source_type(source),
         _target_type(target),
-	_target_bbox(Geom::OptRect()),
+        _target_bbox(Geom::OptRect()),
         _dist()
     {
         _source_num = -1;
@@ -50,7 +52,7 @@ public:
         _source_type(source),
         _target_type(Inkscape::SNAPTARGET_UNDEFINED),
         _source_num(-1),
-	_target_bbox(Geom::OptRect()),
+        _target_bbox(Geom::OptRect()),
         _dist()
     {
     };
diff --git a/src/snap.cpp b/src/snap.cpp
index 1ee55fcf8..695424194 100644
--- a/src/snap.cpp
+++ b/src/snap.cpp
@@ -77,9 +77,14 @@ SnapManager::SnapperList SnapManager::getGridSnappers() const
     return s;
 }
 
-bool SnapManager::someSnapperMightSnap() const
+bool SnapManager::someSnapperMightSnap(bool immediately) const
 {
-    if ( !snapprefs.getSnapEnabledGlobally() || snapprefs.getSnapPostponedGlobally() ) {
+    if ( !snapprefs.getSnapEnabledGlobally() ) {
+        return false;
+    }
+
+    // If we're asking if some snapper might snap RIGHT NOW (without the snap being postponed)...
+    if ( immediately && snapprefs.getSnapPostponedGlobally() ) {
         return false;
     }
 
diff --git a/src/snap.h b/src/snap.h
index 3a8e18ad3..67af20063 100644
--- a/src/snap.h
+++ b/src/snap.h
@@ -93,7 +93,7 @@ public:
      *
      * @return true if one of the snappers will try to snap to something.
      */
-    bool someSnapperMightSnap() const;
+    bool someSnapperMightSnap(bool immediately = true) const;
 
     /**
      * @return true if one of the grids might be snapped to.
diff --git a/src/sp-shape.cpp b/src/sp-shape.cpp
index ad73f226c..d5556ba9e 100644
--- a/src/sp-shape.cpp
+++ b/src/sp-shape.cpp
@@ -1193,10 +1193,10 @@ void SPShape::sp_shape_snappoints(SPItem const *item, std::vector<Inkscape::Snap
 
         // Find the internal intersections of each path and consider these for snapping
         // (using "Method 1" as described in Inkscape::ObjectSnapper::_collectNodes())
-        if (snapprefs->isTargetSnappable(Inkscape::SNAPTARGET_PATH_INTERSECTION)) {
+        if (snapprefs->isTargetSnappable(Inkscape::SNAPTARGET_PATH_INTERSECTION) || snapprefs->isSourceSnappable(Inkscape::SNAPSOURCE_PATH_INTERSECTION)) {
             Geom::Crossings cs;
             try {
-                cs = self_crossings(*path_it);
+                cs = self_crossings(*path_it); // This can be slow!
                 if (!cs.empty()) { // There might be multiple intersections...
                     for (Geom::Crossings::const_iterator i = cs.begin(); i != cs.end(); ++i) {
                         Geom::Point p_ix = (*path_it).pointAt((*i).ta);