Fix bbox snapping as reported in LP bug #562205

diff --git a/src/display/snap-indicator.cpp b/src/display/snap-indicator.cpp
index 1e4ca12a8254c4515a90c6424a1a9c077d6ce0a0..fe5bd0371927471d2d068a39ea66fe4b18caf769 100644 (file)
--- a/src/display/snap-indicator.cpp
+++ b/src/display/snap-indicator.cpp
@@ -55,6 +55,14 @@ SnapIndicator::set_new_snaptarget(Inkscape::SnappedPoint const &p, bool pre_snap
         return; // If we haven't snapped, then it is of no use to draw a snapindicator
     }
 
+    if (p.getTarget() == SNAPTARGET_CONSTRAINT) {
+        // This is not a real snap, although moving along the constraint did affect the mouse pointer's position.
+        // Maybe we should only show a snap indicator when the user explicitly asked for a constraint by pressing ctrl?
+        // We should not show a snap indicator when stretching a selection box, which is also constrained. That would be
+        // too much information.
+        return;
+    }
+
     Inkscape::Preferences *prefs = Inkscape::Preferences::get();
     bool value = prefs->getBool("/options/snapindicator/value", true);
 

diff --git a/src/object-snapper.cpp b/src/object-snapper.cpp
index 7d79a5e91a7aa0686dd66cf8694604aad3acb154..a3285b4069e57c531291a8e812aaffe5e12dc2ba 100644 (file)
--- a/src/object-snapper.cpp
+++ b/src/object-snapper.cpp
@@ -151,8 +151,8 @@ void Inkscape::ObjectSnapper::_findCandidates(SPObject* parent,
                             // This item is within snapping range, so record it as a candidate
                             _candidates->push_back(SnapCandidateItem(item, clip_or_mask, additional_affine));
                             // For debugging: print the id of the candidate to the console
-                            //SPObject *obj = (SPObject*)item;
-                            //std::cout << "Snap candidate added: " << obj->id << std::endl;
+                            // SPObject *obj = (SPObject*)item;
+                            // std::cout << "Snap candidate added: " << obj->getId() << std::endl;
                         }
                     }
                 }

diff --git a/src/seltrans.cpp b/src/seltrans.cpp
index 7a08e0a7e6d1b3df29890f5d5e98330483355fef..aef608420ac258085a45754a7240a6f7b4b3452d 100644 (file)
--- a/src/seltrans.cpp
+++ b/src/seltrans.cpp
@@ -999,8 +999,6 @@ gboolean Inkscape::SelTrans::scaleRequest(Geom::Point &pt, guint state)
         m.setup(_desktop, false, _items_const);
 
         Inkscape::SnappedPoint bb, sn;
-        Geom::Coord bd(NR_HUGE);
-        Geom::Coord sd(NR_HUGE);
 
         if ((state & GDK_CONTROL_MASK) || _desktop->isToolboxButtonActive ("lock")) {
             // Scale is locked to a 1:1 aspect ratio, so that s[X] must be made to equal s[Y].
@@ -1017,27 +1015,17 @@ gboolean Inkscape::SelTrans::scaleRequest(Geom::Point &pt, guint state)
             // Snap along a suitable constraint vector from the origin.
             bb = m.constrainedSnapScale(_bbox_points, _point, default_scale, _origin_for_bboxpoints);
             sn = m.constrainedSnapScale(_snap_points, _point, geom_scale, _origin_for_specpoints);
-
-            /* Choose the smaller difference in scale.  Since s[X] == s[Y] we can
-            ** just compare difference in s[X].
-            */
-            bd = bb.getSnapped() ? fabs(bb.getTransformation()[Geom::X] - default_scale[Geom::X]) : NR_HUGE;
-            sd = sn.getSnapped() ? fabs(sn.getTransformation()[Geom::X] - geom_scale[Geom::X]) : NR_HUGE;
         } else {
             /* Scale aspect ratio is unlocked */
             bb = m.freeSnapScale(_bbox_points, _point, default_scale, _origin_for_bboxpoints);
             sn = m.freeSnapScale(_snap_points, _point, geom_scale, _origin_for_specpoints);
-
-            /* Pick the snap that puts us closest to the original scale */
-            bd = bb.getSnapped() ? fabs(Geom::L2(bb.getTransformation()) - Geom::L2(Geom::Point(default_scale[Geom::X], default_scale[Geom::Y]))) : NR_HUGE;
-            sd = sn.getSnapped() ? fabs(Geom::L2(sn.getTransformation()) - Geom::L2(Geom::Point(geom_scale[Geom::X], geom_scale[Geom::Y]))) : NR_HUGE;
         }
 
         if (!(bb.getSnapped() || sn.getSnapped())) {
             // We didn't snap at all! Don't update the handle position, just calculate the new transformation
             _calcAbsAffineDefault(default_scale);
             _desktop->snapindicator->remove_snaptarget();
-        } else if (bd < sd) {
+        } else if (bb.getSnapped() && !bb.isOtherSnapBetter(sn, false)) {
             // We snapped the bbox (which is either visual or geometric)
             _desktop->snapindicator->set_new_snaptarget(bb);
             default_scale = Geom::Scale(bb.getTransformation());
@@ -1109,8 +1097,6 @@ gboolean Inkscape::SelTrans::stretchRequest(SPSelTransHandle const &handle, Geom
 
         Inkscape::SnappedPoint bb, sn;
         g_assert(bb.getSnapped() == false); // Check initialization to catch any regression
-        Geom::Coord bd(NR_HUGE);
-        Geom::Coord sd(NR_HUGE);
 
         bool symmetrical = state & GDK_CONTROL_MASK;
 
@@ -1119,12 +1105,10 @@ gboolean Inkscape::SelTrans::stretchRequest(SPSelTransHandle const &handle, Geom
 
         if (bb.getSnapped()) {
             // We snapped the bbox (which is either visual or geometric)
-            bd = fabs(bb.getTransformation()[axis] - default_scale[axis]);
             default_scale[axis] = bb.getTransformation()[axis];
         }
 
         if (sn.getSnapped()) {
-            sd = fabs(sn.getTransformation()[axis] - geom_scale[axis]);
             geom_scale[axis] = sn.getTransformation()[axis];
         }
 
@@ -1139,7 +1123,7 @@ gboolean Inkscape::SelTrans::stretchRequest(SPSelTransHandle const &handle, Geom
             // We didn't snap at all! Don't update the handle position, just calculate the new transformation
             _calcAbsAffineDefault(default_scale);
             _desktop->snapindicator->remove_snaptarget();
-        } else if (bd < sd) {
+        } else if (bb.getSnapped() && !bb.isOtherSnapBetter(sn, false)) {
             _desktop->snapindicator->set_new_snaptarget(bb);
             // Calculate the new transformation and update the handle position
             pt = _calcAbsAffineDefault(default_scale);

diff --git a/src/snap-candidate.h b/src/snap-candidate.h
index bd378bec83e2d57baed389292f680f1ad2ba6b1d..a0fc3290cde5892d37c9e97188b728f3e2b62222 100644 (file)
--- a/src/snap-candidate.h
+++ b/src/snap-candidate.h
@@ -55,6 +55,7 @@ public:
     inline Inkscape::SnapSourceType getSourceType() const {return _source_type;}
     inline Inkscape::SnapTargetType getTargetType() const {return _target_type;}
     inline long getSourceNum() const {return _source_num;}
+    void setSourceNum(long num) {_source_num = num;}
     inline Geom::OptRect const getTargetBBox() const {return _target_bbox;}
 
 private:

diff --git a/src/snap.cpp b/src/snap.cpp
index b8b08dad5a9ac420ad1bdafbf99b1b7c6b377f37..8704ce3bb69200752fed6dba11088f10523053dc 100644 (file)
--- a/src/snap.cpp
+++ b/src/snap.cpp
@@ -524,7 +524,7 @@ Inkscape::SnappedPoint SnapManager::_snapTransformed(
     /* Quick check to see if we have any snappers that are enabled
     ** Also used to globally disable all snapping
     */
-    if (someSnapperMightSnap() == false) {
+    if (someSnapperMightSnap() == false || points.size() == 0) {
         return Inkscape::SnappedPoint(pointer);
     }
 
@@ -559,10 +559,11 @@ Inkscape::SnappedPoint SnapManager::_snapTransformed(
     g_assert(best_snapped_point.getAlwaysSnap() == false); // Check initialization of snapped point
     g_assert(best_snapped_point.getAtIntersection() == false);
 
-    std::vector<Inkscape::SnapCandidatePoint>::const_iterator j = transformed_points.begin();
+    std::vector<Inkscape::SnapCandidatePoint>::iterator j = transformed_points.begin();
 
 
     // std::cout << std::endl;
+    bool first_free_snap = true;
     for (std::vector<Inkscape::SnapCandidatePoint>::const_iterator i = points.begin(); i != points.end(); i++) {
 
         /* Snap it */
@@ -599,6 +600,17 @@ Inkscape::SnappedPoint SnapManager::_snapTransformed(
                 dedicated_constraint = Inkscape::Snapper::ConstraintLine(origin, component_vectors[c1]);
                 snapped_point = constrainedSnap(*j, dedicated_constraint, bbox);
             } else {
+                // If we have a collection of SnapCandidatePoints, with mixed constrained snapping and free snapping
+                // requirements, then freeSnap might never see the SnapCandidatePoint with source_num == 0. The freeSnap()
+                // method in the object snapper depends on this, because only for source-num == 0 the target nodes will
+                // be collected. Therefore we enforce that the first SnapCandidatePoint that is to be freeSnapped always
+                // has source_num == 0;
+                // TODO: This is a bit ugly so fix this; do we need sourcenum for anything else? if we don't then get rid
+                // of it and explicitely communicate to the object snapper that this is a first point
+                if (first_free_snap) {
+                    (*j).setSourceNum(0);
+                    first_free_snap = false;
+                }
                 snapped_point = freeSnap(*j, bbox);
             }
         }
@@ -902,6 +914,7 @@ Inkscape::SnappedPoint SnapManager::findBestSnap(Inkscape::SnapCandidatePoint co
                                                  bool allowOffScreen) const
 {
 
+
     /*
     std::cout << "Type and number of snapped constraints: " << std::endl;
     std::cout << "  Points      : " << sc.points.size() << std::endl;

diff --git a/src/snapped-point.cpp b/src/snapped-point.cpp
index 089aa4323e39b66fd6c91853bebeddcd00e43bd8..508ec8f62c1e49aaeb7f1bb1ccf489ef32e0e388 100644 (file)
--- a/src/snapped-point.cpp
+++ b/src/snapped-point.cpp
@@ -164,6 +164,15 @@ bool Inkscape::SnappedPoint::isOtherSnapBetter(Inkscape::SnappedPoint const &oth
         }
     }
 
+    // When snapping to a constraint line only, which is not really a snap but merely a projection
+    // to the constraint line, then give this snap a very low priority. Basically, any other snap will do
+    if (other_one.getTarget() == SNAPTARGET_CONSTRAINT) {
+        dist_other += NR_HUGE/2;
+    }
+    if (getTarget() == SNAPTARGET_CONSTRAINT) {
+        dist_this += NR_HUGE/2;
+    }
+
     // If it's closer
     bool c1 = dist_other < dist_this;
     // or, if it's for a snapper with "always snap" turned on, and the previous wasn't