From d190791228fa8662b1de5a4d67265585c6ea663c Mon Sep 17 00:00:00 2001 From: Diederik van Lierop Date: Sun, 18 Apr 2010 21:43:05 +0200 Subject: [PATCH] Fix bbox snapping as reported in LP bug #562205 --- src/display/snap-indicator.cpp | 8 ++++++++ src/object-snapper.cpp | 4 ++-- src/seltrans.cpp | 20 ++------------------ src/snap-candidate.h | 1 + src/snap.cpp | 17 +++++++++++++++-- src/snapped-point.cpp | 9 +++++++++ 6 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/display/snap-indicator.cpp b/src/display/snap-indicator.cpp index 1e4ca12a8..fe5bd0371 100644 --- 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 7d79a5e91..a3285b406 100644 --- 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 7a08e0a7e..aef608420 100644 --- 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 bd378bec8..a0fc3290c 100644 --- 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 b8b08dad5..8704ce3bb 100644 --- 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::const_iterator j = transformed_points.begin(); + std::vector::iterator j = transformed_points.begin(); // std::cout << std::endl; + bool first_free_snap = true; for (std::vector::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 089aa4323..508ec8f62 100644 --- 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 -- 2.30.2