From 0ded3a4af1c91f3d76b845f832a75d5781c84535 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Fri, 23 Aug 2024 05:52:43 +0530 Subject: [PATCH] [Sketcher] Refactor `SketchObject::trim()` 1. Use `Part::GeomCurve::createArc()` 2. Refactor constraint logic in `trim` --- src/Mod/Sketcher/App/SketchObject.cpp | 248 +++++++++----------------- 1 file changed, 85 insertions(+), 163 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 9b2acf87d7..eb17c2ae6d 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -3566,18 +3566,18 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) return ((point1 - point2).Length() < 500 * Precision::Confusion()) ; }; - auto isPointAtPosition = - [this, &arePointsWithinPrecision](int GeoId1, PointPos pos1, Base::Vector3d point) { - Base::Vector3d pp = getPoint(GeoId1, pos1); + auto isPointAtPosition = [this, &arePointsWithinPrecision] + (int GeoId1, PointPos pos1, Base::Vector3d point) { + Base::Vector3d pp = getPoint(GeoId1, pos1); - return arePointsWithinPrecision(point, pp); - }; + return arePointsWithinPrecision(point, pp); + }; // Checks whether preexisting constraints must be converted to new constraints. // Preexisting point on object constraints get converted to coincidents, unless an end-to-end // tangency is more relevant. returns by reference: - // - The type of constraint that should be used to constraint GeoId1 and GeoId - // - The element of GeoId1 to which the constraint should be applied. + // - The type of constraint that should be used to constraint GeoId1 and GeoId + // - The element of GeoId1 to which the constraint should be applied. auto transformPreexistingConstraint = [this, isPointAtPosition](int GeoId, int GeoId1, Base::Vector3d point1, @@ -3681,10 +3681,10 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) double lastParam = geoAsCurve->getLastParameter(); double pointParam, point1Param, point2Param; if (!getIntersectionParameters( - geo, point, pointParam, point1, point1Param, point2, point2Param)) + geo, point, pointParam, point1, point1Param, point2, point2Param)) { return -1; + } - bool ok = false; bool startPointRemains = false; bool endPointRemains = false; std::vector > paramsOfNewGeos; @@ -3693,121 +3693,29 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) std::vector newGeosAsConsts; bool oldGeoIsConstruction = GeometryFacade::getConstruction(static_cast(geo)); - auto setUpNewGeoLimitsFromNonPeriodic = - [&]() { - if (GeoId1 != GeoEnum::GeoUndef) { - startPointRemains = true; - paramsOfNewGeos.emplace_back(firstParam, point1Param); - } - if (GeoId2 != GeoEnum::GeoUndef) { - endPointRemains = true; - paramsOfNewGeos.emplace_back(point2Param, lastParam); - } - }; - - auto setUpNewGeoLimitsFromPeriodic = - [&]() { - startPointRemains = false; - endPointRemains = false; - if (GeoId1 != GeoEnum::GeoUndef && GeoId2 != GeoEnum::GeoUndef) { - paramsOfNewGeos.emplace_back(point2Param, point1Param); - } - }; - - auto createArcGeos = [&](const Part::GeomTrimmedCurve* curve) { - for (auto& [u1, u2] : paramsOfNewGeos) { - auto newArc = std::unique_ptr( - static_cast(curve->copy())); - newArc->setRange(u1, u2); - newGeos.push_back(newArc.release()); + if (isClosedCurve(geo)) { + startPointRemains = false; + endPointRemains = false; + if (GeoId1 != GeoEnum::GeoUndef && GeoId2 != GeoEnum::GeoUndef) { + paramsOfNewGeos.emplace_back(point2Param, point1Param); } - - return true; - }; - - auto createArcGeosForCircle = [&](const Part::GeomCircle* curve) { - for (auto& [u1, u2] : paramsOfNewGeos) { - auto newArc = std::make_unique( - Handle(Geom_Circle)::DownCast(curve->handle()->Copy())); - newArc->setRange(u1, u2, false); - newGeos.push_back(newArc.release()); - // int newId(GeoEnum::GeoUndef); - // newId = addGeometry(std::move(newArc)); - // if (newId < 0) { - // return false; - // } - // newIds.push_back(newId); - // setConstruction(newId, GeometryFacade::getConstruction(curve)); - // exposeInternalGeometry(newId); - } - - return true; - }; - - auto createArcGeosForEllipse = [&](const Part::GeomEllipse* curve) { - for (auto& [u1, u2] : paramsOfNewGeos) { - auto newArc = std::make_unique( - Handle(Geom_Ellipse)::DownCast(curve->handle()->Copy())); - newArc->setRange(u1, u2, false); - newGeos.push_back(newArc.release()); - } - - return true; - }; - - auto createArcGeosForBSplineCurve = [&](const Part::GeomBSplineCurve* curve) { - for (auto& [u1, u2] : paramsOfNewGeos) { - auto newBsp = std::unique_ptr( - static_cast(curve->copy())); - newBsp->Trim(u1, u2); - newGeos.push_back(newBsp.release()); - } - - return true; - }; - - if (geo->is()) { - setUpNewGeoLimitsFromNonPeriodic(); - - ok = createArcGeos(static_cast(geo)); } - else if (geo->is()) { - setUpNewGeoLimitsFromPeriodic(); - - ok = createArcGeosForCircle(static_cast(geo)); - } - else if (geo->is()) { - setUpNewGeoLimitsFromPeriodic(); - - ok = createArcGeosForEllipse(static_cast(geo)); - } - else if (geo->is()) { - setUpNewGeoLimitsFromNonPeriodic(); - - ok = createArcGeos(static_cast(geo)); - } - else if (geo->isDerivedFrom()) { - setUpNewGeoLimitsFromNonPeriodic(); - - ok = createArcGeos(static_cast(geo)); - } - else if (geo->is()) { - const Part::GeomBSplineCurve* bsp = static_cast(geo); - - // what to do for periodic b-splines? - if (bsp->isPeriodic()) { - setUpNewGeoLimitsFromPeriodic(); + else { + if (GeoId1 != GeoEnum::GeoUndef) { + startPointRemains = true; + paramsOfNewGeos.emplace_back(firstParam, point1Param); } - else { - setUpNewGeoLimitsFromNonPeriodic(); + if (GeoId2 != GeoEnum::GeoUndef) { + endPointRemains = true; + paramsOfNewGeos.emplace_back(point2Param, lastParam); } - - ok = createArcGeosForBSplineCurve(bsp); } - if (!ok) { - delGeometries(newIds); - return -1; + for (auto& [u1, u2] : paramsOfNewGeos) { + auto newGeo = static_cast(geo)->createArc(u1, u2); + assert(newGeo); + newGeos.push_back(newGeo); + newGeosAsConsts.push_back(newGeo); } switch (newGeos.size()) { @@ -3832,16 +3740,15 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) } } - for (auto& newGeo : newGeos) { - newGeosAsConsts.push_back(newGeo); - } - // Now that we have the new curves, change constraints as needed // Some are covered with `deriveConstraintsForPieces`, others are specific to trim // FIXME: We are using non-smart pointers since that's what's needed in `addConstraints`. std::vector newConstraints; - // A geoId we expect to never exist during this operation. We use it because newIds.front() is supposed to be `GeoId`, but we will delete constraints on it down the line, even if we will need it later. As a result, note that this will cause some malformed constraints until they are transferred back. + // A geoId we expect to never exist during this operation (but still not `GeoEnum::GeoUndef`). + // We use it because newIds.front() is supposed to be `GeoId`, but we will delete constraints + // on it down the line, even if we may need it later. + // Note that this will cause some malformed constraints until they are transferred back. int nonexistentGeoId = getHighestCurveIndex() + newIds.size(); newIds.front() = nonexistentGeoId; @@ -3879,79 +3786,94 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) const auto& allConstraints = this->Constraints.getValues(); - bool isGeoId1CoincidentOnPoint1 = false; - bool isGeoId2CoincidentOnPoint2 = false; + bool isPoint1ConstrainedOnGeoId1 = false; + bool isPoint2ConstrainedOnGeoId2 = false; // Workaround to avoid https://github.com/FreeCAD/FreeCAD/issues/15484. // We will delete its internal geometry first and then replace GeoId with one of the curves. // Of course, this will change `newIds`, and that's why we offset them. - std::vector geoIdsToBeDeleted; + std::set< int, std::greater<> > geoIdsToBeDeleted; // geoIdsToBeDeleted.push_back(GeoId); - if (hasInternalGeometry(geo)) { - for (const auto& oldConstrId: idsOfOldConstraints) { - if (allConstraints[oldConstrId]->Type == InternalAlignment) { - geoIdsToBeDeleted.push_back(allConstraints[oldConstrId]->First); - } - } - - // NOTE: Assuming no duplication here. - // If there are redundants for some pathological reason, use std::unique. - std::sort(geoIdsToBeDeleted.begin(), geoIdsToBeDeleted.end(), std::greater<>()); - - // keep constraints on internal geometries so they are deleted - // when the old curve is deleted - } for (const auto& oldConstrId: idsOfOldConstraints) { + // trim-specific changes first const Constraint* con = allConstraints[oldConstrId]; - bool newConstraintCreated = deriveConstraintsForPieces(GeoId, newIds, newGeosAsConsts, con, newConstraints); - // trim-specific changes once general changes are done + bool constraintWasModified = false; switch (con->Type) { case PointOnObject: { - if (!newConstraintCreated) { - break; - } - Constraint* newConstr = newConstraints.back(); // we might want to transform this (and the new point-on-object constraints) into a coincidence - if (newConstr->Second == newIds.front() && - isPointAtPosition(newConstr->First, newConstr->FirstPos, point1)) { + if (con->First == GeoId1 && con->Second == GeoId + && isPointAtPosition(con->First, con->FirstPos, point1)) { // This concerns the start portion of the trim + Constraint* newConstr = con->copy(); newConstr->Type = Sketcher::Coincident; + newConstr->Second = newIds.front(); newConstr->SecondPos = PointPos::end; - if (newConstr->First == GeoId1) { - isGeoId1CoincidentOnPoint1 = true; - } + newConstraints.push_back(newConstr); + isPoint1ConstrainedOnGeoId1 = true; + constraintWasModified = true; } - if (newConstr->Second == newIds.back() && - isPointAtPosition(newConstr->First, newConstr->FirstPos, point2)) { + else if (con->First == GeoId2 && con->Second == GeoId + && isPointAtPosition(con->First, con->FirstPos, point2)) { // This concerns the end portion of the trim + Constraint* newConstr = con->copy(); newConstr->Type = Sketcher::Coincident; + newConstr->Second = newIds.back(); newConstr->SecondPos = PointPos::start; - if (newConstr->First == GeoId2) { - isGeoId2CoincidentOnPoint2 = true; - } + newConstraints.push_back(newConstr); + isPoint2ConstrainedOnGeoId2 = true; + constraintWasModified = true; } break; } case Tangent: case Perpendicular: { - // TODO Tangent may have to be turned into endpoint-to-endpoint - // we might want to transform this into a coincidence - if (!newConstraintCreated) { - break; + // These may have to be turned into endpoint-to-endpoint or endpoint-to-edge + // TODO: could there be tangent/perpendicular constraints not involving the trim that are modified below? + if (con->involvesGeoId(GeoId1) && con->involvesGeoIdAndPosId(GeoId, PointPos::none)) { + Constraint* newConstr = con->copy(); + newConstr->substituteIndexAndPos(GeoId, PointPos::none, newIds.front(), PointPos::end); + newConstraints.push_back(newConstr); + isPoint1ConstrainedOnGeoId1 = true; + constraintWasModified = true; } + if (con->involvesGeoId(GeoId2) && con->involvesGeoIdAndPosId(GeoId, PointPos::none)) { + Constraint* newConstr = con->copy(); + newConstr->substituteIndexAndPos(GeoId, PointPos::none, newIds.back(), PointPos::start); + newConstraints.push_back(newConstr); + isPoint2ConstrainedOnGeoId2 = true; + constraintWasModified = true; + } + if (constraintWasModified) { + // make sure the first position is a point + if (newConstraints.back()->FirstPos == PointPos::none) { + std::swap(newConstraints.back()->First, newConstraints.back()->Second); + std::swap(newConstraints.back()->FirstPos, newConstraints.back()->SecondPos); + } + // there is no need for the third point if it exists + newConstraints.back()->Third = GeoEnum::GeoUndef; + newConstraints.back()->ThirdPos = PointPos::none; + } + break; + } + case InternalAlignment: { + geoIdsToBeDeleted.insert(con->First); break; } default: break; } + if (!constraintWasModified) { + deriveConstraintsForPieces(GeoId, newIds, newGeosAsConsts, con, newConstraints); + } } // Add point-on-object/coincidence constraints with the newly exposed points. - // This will need to account for the constraints that were already converted to coincident or end-to-end tangency/perpendicularity. + // This will need to account for the constraints that were already converted + // to coincident or end-to-end tangency/perpendicularity. // TODO: Tangent/perpendicular not yet covered - if (GeoId1 != GeoEnum::GeoUndef && !isGeoId1CoincidentOnPoint1) { + if (GeoId1 != GeoEnum::GeoUndef && !isPoint1ConstrainedOnGeoId1) { Constraint* newConstr = new Constraint(); newConstr->First = newIds.front(); newConstr->FirstPos = PointPos::end; @@ -3972,7 +3894,7 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) newConstraints.push_back(newConstr); } - if (GeoId2 != GeoEnum::GeoUndef && !isGeoId2CoincidentOnPoint2) { + if (GeoId2 != GeoEnum::GeoUndef && !isPoint2ConstrainedOnGeoId2) { Constraint* newConstr = new Constraint(); newConstr->First = newIds.back(); newConstr->FirstPos = PointPos::start;