From 41e1b647ae08c2c59e0a186e4011b20c331c1b35 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 9 Mar 2025 21:07:09 +0530 Subject: [PATCH] [Sketcher] Adjust constraint changes when trimming 1. No longer applying equality constraints between new (circular) pieces since they may cause issues. 2. Only transfer equality with a different curve to one of the pieces. 3. Re-added certain constraints (that applied to both ends of the original curve) that were incorrectly excluded from modification/deletion at a certain step. 4. Use C++20 `std::erase_if()` in trim --- src/Mod/Sketcher/App/SketchObject.cpp | 77 +++++++++++++-------------- 1 file changed, 38 insertions(+), 39 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 7d992153c7..1692100df8 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -22,6 +22,7 @@ ***************************************************************************/ #include "PreCompiled.h" +#include #ifndef _PreComp_ #include #include @@ -3624,6 +3625,7 @@ std::unique_ptr transformPreexistingConstraintForTrim(const SketchOb * is set to Sketcher::Tangent, that the secondPos1 is captured from the PointOnObject, * and also make sure that the PointOnObject constraint is deleted. */ + // TODO: Symmetric and distance constraints (sometimes together) can be changed to something std::unique_ptr newConstr; if (!constr->involvesGeoId(cuttingGeoId) || !constr->involvesGeoIdAndPosId(GeoId, PointPos::none)) { @@ -3635,7 +3637,7 @@ std::unique_ptr transformPreexistingConstraintForTrim(const SketchOb // coincidence At this stage of the check the point has to be an end of `cuttingGeoId` // on the edge of `GeoId`. if (isPointAtPosition(obj, constr->First, constr->FirstPos, cutPointVec)) { - // This concerns the start portion of the trim + // We already know the point-on-object is on the whole of GeoId newConstr.reset(constr->copy()); newConstr->Type = Sketcher::Coincident; newConstr->Second = newGeoId; @@ -3924,34 +3926,23 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) // remove the constraints that we want to manually transfer // We could transfer beforehand but in case of exception that transfer is permanent if (!isClosedCurve(geoAsCurve)) { - if (cuttingGeoIds[0] != GeoEnum::GeoUndef) { - idsOfOldConstraints.erase( - std::remove_if( - idsOfOldConstraints.begin(), - idsOfOldConstraints.end(), - [&GeoId, &allConstraints](const auto& i) { - return (allConstraints[i]->involvesGeoIdAndPosId(GeoId, PointPos::start)); - }), - idsOfOldConstraints.end()); - } - if (cuttingGeoIds[1] != GeoEnum::GeoUndef) { - idsOfOldConstraints.erase( - std::remove_if(idsOfOldConstraints.begin(), - idsOfOldConstraints.end(), - [&GeoId, &allConstraints](const auto& i) { - return (allConstraints[i]->involvesGeoIdAndPosId(GeoId, - PointPos::end)); - }), - idsOfOldConstraints.end()); - } + std::erase_if(idsOfOldConstraints, + [&GeoId, &allConstraints, &cuttingGeoIds](const auto& i) { + auto* constr = allConstraints[i]; + bool involvesStart = + constr->involvesGeoIdAndPosId(GeoId, PointPos::start); + bool involvesEnd = constr->involvesGeoIdAndPosId(GeoId, PointPos::end); + bool keepStart = cuttingGeoIds[0] != GeoEnum::GeoUndef; + bool keepEnd = cuttingGeoIds[1] != GeoEnum::GeoUndef; + bool involvesBothButNotBothKept = + involvesStart && involvesEnd && !(keepStart && keepEnd); + return !involvesBothButNotBothKept + && ((involvesStart && keepStart) || (involvesEnd && keepEnd)); + }); } - idsOfOldConstraints.erase( - std::remove_if(idsOfOldConstraints.begin(), - idsOfOldConstraints.end(), - [&GeoId, &allConstraints](const auto& i) { - return (allConstraints[i]->involvesGeoIdAndPosId(GeoId, PointPos::mid)); - }), - idsOfOldConstraints.end()); + std::erase_if(idsOfOldConstraints, [&GeoId, &allConstraints](const auto& i) { + return (allConstraints[i]->involvesGeoIdAndPosId(GeoId, PointPos::mid)); + }); createNewConstraintsForTrim(this, GeoId, @@ -3966,7 +3957,7 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) //****************************************// // Constraints related to start/mid/end points of original - auto constrainAsEqual = [this](int GeoId1, int GeoId2) { + [[maybe_unused]] auto constrainAsEqual = [this](int GeoId1, int GeoId2) { auto newConstr = std::make_unique(); // Build Constraints associated with new pair of arcs @@ -3999,12 +3990,21 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) newConstraints.push_back(joint); // Any radius etc. equality constraints here - constrainAsEqual(newIds.front(), newIds.back()); + // TODO: There could be some form of equality between the constraints here. However, it + // may happen that this is imposed by an elaborate set of additional constraints. When + // that happens, this causes redundant constraints, and in worse cases (incorrect) + // complaints of over-constraint and solver failures. + + // if (std::none_of(newConstraints.begin(), newConstraints.end(), [](const auto& constr) + // { + // return constr->Type == ConstraintType::Equal; + // })) { + // constrainAsEqual(newIds.front(), newIds.back()); + // } // TODO: ensure alignment as well? } } - // TODO: Implement this replaceGeometries({GeoId}, newGeos); if (noRecomputes) { @@ -4016,12 +4016,9 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) changeConstraintAfterDeletingGeo(cons, deletedGeoId); } } - newConstraints.erase(std::remove_if(newConstraints.begin(), - newConstraints.end(), - [](const auto& constr) { - return constr->Type == ConstraintType::None; - }), - newConstraints.end()); + std::erase_if(newConstraints, [](const auto& constr) { + return constr->Type == ConstraintType::None; + }); delGeometries(geoIdsToBeDeleted.begin(), geoIdsToBeDeleted.end()); addConstraints(newConstraints); @@ -4152,8 +4149,10 @@ bool SketchObject::deriveConstraintsForPieces(const int oldId, case Radius: case Diameter: case Equal: { - // FIXME: This sounds good by itself, but results in redundant constraints when equality is applied between newIds - transferToAll = geo->is() || geo->is(); + // Only transfer to one of them (arbitrarily chosen here as the first) + Constraint* trans = con->copy(); + trans->substituteIndex(oldId, newIds.front()); + newConstraints.push_back(trans); break; } default: