From 0f3aff0eacd19dcd6199ed28fa16adb8fc873f9b Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 12 Aug 2024 00:39:38 +0530 Subject: [PATCH 01/26] [Sketcher][test] Create test for `getConstraintAfterDeletingGeo`+ [Sketcher] Add null case to constraint change on deletion checks --- src/Mod/Sketcher/App/SketchObject.h | 2 + tests/src/Mod/Sketcher/App/SketchObject.cpp | 69 +++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/src/Mod/Sketcher/App/SketchObject.h b/src/Mod/Sketcher/App/SketchObject.h index f9e95e0615..b132017bff 100644 --- a/src/Mod/Sketcher/App/SketchObject.h +++ b/src/Mod/Sketcher/App/SketchObject.h @@ -982,6 +982,8 @@ protected: int thirdGeoId = GeoEnum::GeoUndef, Sketcher::PointPos thirdPos = Sketcher::PointPos::none); +public: + // FIXME: These may not need to be public. Decide before merging. std::unique_ptr getConstraintAfterDeletingGeo(const Constraint* constr, const int deletedGeoId) const; diff --git a/tests/src/Mod/Sketcher/App/SketchObject.cpp b/tests/src/Mod/Sketcher/App/SketchObject.cpp index 69c4fdcd54..9010c1749a 100644 --- a/tests/src/Mod/Sketcher/App/SketchObject.cpp +++ b/tests/src/Mod/Sketcher/App/SketchObject.cpp @@ -562,6 +562,75 @@ TEST_F(SketchObjectTest, testGetPointFromGeomBSplineCurvePeriodic) EXPECT_DOUBLE_EQ(ptStart[1], ptEnd[1]); } +TEST_F(SketchObjectTest, testConstraintAfterDeletingGeo) +{ + // Arrange + int geoId1 = 42, geoId2 = 10, geoId3 = 0, geoId4 = -8; + + Sketcher::Constraint* nullConstr = nullptr; + + Sketcher::Constraint constr1; + constr1.Type = Sketcher::ConstraintType::Coincident; + constr1.First = geoId1; + constr1.FirstPos = Sketcher::PointPos::start; + constr1.Second = geoId2; + constr1.SecondPos = Sketcher::PointPos::end; + + Sketcher::Constraint constr2; + constr2.Type = Sketcher::ConstraintType::Tangent; + constr2.First = geoId4; + constr2.FirstPos = Sketcher::PointPos::none; + constr2.Second = geoId3; + constr2.SecondPos = Sketcher::PointPos::none; + constr2.Third = geoId1; + constr2.ThirdPos = Sketcher::PointPos::start; + + // Act + auto nullConstrAfter = getObject()->getConstraintAfterDeletingGeo(nullConstr, 5); + + // Assert + EXPECT_EQ(nullConstrAfter, nullptr); + + // Act + getObject()->changeConstraintAfterDeletingGeo(nullConstr, 5); + + // Assert + EXPECT_EQ(nullConstr, nullptr); + + // Act + // delete typical in-sketch geo + auto constr1PtrAfter1 = getObject()->getConstraintAfterDeletingGeo(&constr1, 5); + // delete external geo (negative id) + auto constr1PtrAfter2 = getObject()->getConstraintAfterDeletingGeo(&constr1, -5); + // Delete a geo involved in the constraint + auto constr1PtrAfter3 = getObject()->getConstraintAfterDeletingGeo(&constr1, 10); + + // Assert + EXPECT_EQ(constr1.Type, Sketcher::ConstraintType::Coincident); + EXPECT_EQ(constr1.First, geoId1); + EXPECT_EQ(constr1.Second, geoId2); + EXPECT_EQ(constr1PtrAfter1->First, geoId1 - 1); + EXPECT_EQ(constr1PtrAfter1->Second, geoId2 - 1); + EXPECT_EQ(constr1PtrAfter2->Third, Sketcher::GeoEnum::GeoUndef); + EXPECT_EQ(constr1PtrAfter3.get(), nullptr); + + // Act + getObject()->changeConstraintAfterDeletingGeo(&constr2, -3); + + // Assert + EXPECT_EQ(constr2.Type, Sketcher::ConstraintType::Tangent); + EXPECT_EQ(constr2.First, geoId4 + 1); + EXPECT_EQ(constr2.Second, geoId3); + EXPECT_EQ(constr2.Third, geoId1); + + // Act + // Delete a geo involved in the constraint + getObject()->changeConstraintAfterDeletingGeo(&constr2, 0); + + // Assert + EXPECT_EQ(constr2.Type, Sketcher::ConstraintType::None); +} + TEST_F(SketchObjectTest, testSplitLineSegment) { // Arrange From 5a4072587bb3685018c65c0b6c40b646f200551c Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 11 Aug 2024 22:20:56 +0530 Subject: [PATCH 02/26] [Sketcher] Some trivial `for` loop changes in `SketchObject` No tests added since this should be no more different than existing code. --- src/Mod/Sketcher/App/SketchObject.cpp | 206 +++++++++++++------------- 1 file changed, 104 insertions(+), 102 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index ba9bedba3d..ae18fc8904 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -1865,7 +1865,6 @@ int SketchObject::delGeometriesExclusiveList(const std::vector& GeoIds) if (sGeoIds.front() < 0 || sGeoIds.back() >= int(vals.size())) return -1; - std::vector newVals(vals); for (auto it = sGeoIds.rbegin(); it != sGeoIds.rend(); ++it) { int GeoId = *it; @@ -1880,8 +1879,6 @@ int SketchObject::delGeometriesExclusiveList(const std::vector& GeoIds) delConstraintOnPoint(GeoId, PosId, true /* only coincidence */); transferConstraints(GeoIdList[0], PosIdList[0], GeoIdList[1], PosIdList[1]); } - // loop through [start, end, mid] - PosId = (PosId == PointPos::start) ? PointPos::end : PointPos::mid; } } @@ -1892,23 +1889,22 @@ int SketchObject::delGeometriesExclusiveList(const std::vector& GeoIds) std::vector filteredConstraints(0); for (auto itGeo = sGeoIds.rbegin(); itGeo != sGeoIds.rend(); ++itGeo) { int GeoId = *itGeo; - for (std::vector::const_iterator it = constraints.begin(); - it != constraints.end(); - ++it) { - - Constraint* copiedConstr(*it); - if ((*it)->First != GeoId && (*it)->Second != GeoId && (*it)->Third != GeoId) { - if (copiedConstr->First > GeoId) - copiedConstr->First -= 1; - if (copiedConstr->Second > GeoId) - copiedConstr->Second -= 1; - if (copiedConstr->Third > GeoId) - copiedConstr->Third -= 1; - filteredConstraints.push_back(copiedConstr); - } - else { + for (const auto& constr : constraints) { + Constraint* copiedConstr(constr); + if (constr->First == GeoId || + constr->Second == GeoId || + constr->Third == GeoId) { delete copiedConstr; + continue; } + + if (copiedConstr->First > GeoId) + copiedConstr->First -= 1; + if (copiedConstr->Second > GeoId) + copiedConstr->Second -= 1; + if (copiedConstr->Third > GeoId) + copiedConstr->Third -= 1; + filteredConstraints.push_back(copiedConstr); } constraints = filteredConstraints; @@ -9500,57 +9496,58 @@ const std::vector> SketchObject::getCoincidenc std::vector> coincidenttree; // push the constraints - for (std::vector::const_iterator it = vals.begin(); it != vals.end(); - ++it) { - if ((*it)->Type == Sketcher::Coincident) { - int firstpresentin = -1; - int secondpresentin = -1; + for (const auto& constr : vals) { + if (constr->Type != Sketcher::Coincident) { + continue; + } - int i = 0; + int firstpresentin = -1; + int secondpresentin = -1; - for (std::vector>::const_iterator iti = - coincidenttree.begin(); - iti != coincidenttree.end(); - ++iti, i++) { - // First - std::map::const_iterator filiterator; - filiterator = (*iti).find((*it)->First); - if (filiterator != (*iti).end()) { - if ((*it)->FirstPos == (*filiterator).second) - firstpresentin = i; - } - // Second - filiterator = (*iti).find((*it)->Second); - if (filiterator != (*iti).end()) { - if ((*it)->SecondPos == (*filiterator).second) - secondpresentin = i; - } - } + int i = 0; - if (firstpresentin != -1 && secondpresentin != -1) { - // we have to merge those sets into one - coincidenttree[firstpresentin].insert(coincidenttree[secondpresentin].begin(), - coincidenttree[secondpresentin].end()); - coincidenttree.erase(coincidenttree.begin() + secondpresentin); + for (std::vector>::const_iterator iti = + coincidenttree.begin(); + iti != coincidenttree.end(); + ++iti, ++i) { + // First + std::map::const_iterator filiterator; + filiterator = (*iti).find(constr->First); + if (filiterator != (*iti).end()) { + if (constr->FirstPos == (*filiterator).second) + firstpresentin = i; } - else if (firstpresentin == -1 && secondpresentin == -1) { - // we do not have any of the values, so create a setCursor - std::map tmp; - tmp.insert(std::pair((*it)->First, (*it)->FirstPos)); - tmp.insert(std::pair((*it)->Second, (*it)->SecondPos)); - coincidenttree.push_back(tmp); - } - else if (firstpresentin != -1) { - // add to existing group - coincidenttree[firstpresentin].insert( - std::pair((*it)->Second, (*it)->SecondPos)); - } - else {// secondpresentin != -1 - // add to existing group - coincidenttree[secondpresentin].insert( - std::pair((*it)->First, (*it)->FirstPos)); + // Second + filiterator = (*iti).find(constr->Second); + if (filiterator != (*iti).end()) { + if (constr->SecondPos == (*filiterator).second) + secondpresentin = i; } } + + if (firstpresentin != -1 && secondpresentin != -1) { + // we have to merge those sets into one + coincidenttree[firstpresentin].insert(coincidenttree[secondpresentin].begin(), + coincidenttree[secondpresentin].end()); + coincidenttree.erase(coincidenttree.begin() + secondpresentin); + } + else if (firstpresentin == -1 && secondpresentin == -1) { + // we do not have any of the values, so create a setCursor + std::map tmp; + tmp.insert(std::pair(constr->First, constr->FirstPos)); + tmp.insert(std::pair(constr->Second, constr->SecondPos)); + coincidenttree.push_back(tmp); + } + else if (firstpresentin != -1) { + // add to existing group + coincidenttree[firstpresentin].insert( + std::pair(constr->Second, constr->SecondPos)); + } + else {// secondpresentin != -1 + // add to existing group + coincidenttree[secondpresentin].insert( + std::pair(constr->First, constr->FirstPos)); + } } return coincidenttree; @@ -9751,50 +9748,55 @@ void SketchObject::getGeometryWithDependentParameters( { auto geos = getInternalGeometry(); - int geoid = 0; + int geoid = -1; for (auto geo : geos) { - if (geo) { - if (geo->hasExtension(Sketcher::SolverGeometryExtension::getClassTypeId())) { + ++geoid; - auto solvext = std::static_pointer_cast( - geo->getExtension(Sketcher::SolverGeometryExtension::getClassTypeId()).lock()); - - if (solvext->getGeometry() - == Sketcher::SolverGeometryExtension::NotFullyConstraint) { - // The solver differentiates whether the parameters that are dependent are not - // those of start, end, mid, and assigns them to the edge (edge params = curve - // params - parms of start, end, mid). The user looking at the UI expects that - // the edge of a NotFullyConstraint geometry will always move, even if the edge - // parameters are independent, for example if mid is the only dependent - // parameter. In other words, the user could reasonably restrict the edge to - // reach a fully constrained element. Under this understanding, the edge - // parameter would always be dependent, unless the element is fully constrained. - // - // While this is ok from a user visual expectation point of view, it leads to a - // loss of information of whether restricting the point start, end, mid that is - // dependent may suffice, or even if such points are restricted, the edge would - // still need to be restricted. - // - // Because Python gets the information in this function, it would lead to Python - // users having access to a lower amount of detail. - // - // For this reason, this function returns edge as dependent parameter if and - // only if constraining the parameters of the points would not suffice to - // constraint the element. - if (solvext->getEdge() == SolverGeometryExtension::Dependent) - geometrymap.emplace_back(geoid, Sketcher::PointPos::none); - if (solvext->getStart() == SolverGeometryExtension::Dependent) - geometrymap.emplace_back(geoid, Sketcher::PointPos::start); - if (solvext->getEnd() == SolverGeometryExtension::Dependent) - geometrymap.emplace_back(geoid, Sketcher::PointPos::start); - if (solvext->getMid() == SolverGeometryExtension::Dependent) - geometrymap.emplace_back(geoid, Sketcher::PointPos::start); - } - } + if (!geo) { + continue; } - geoid++; + if (!geo->hasExtension(Sketcher::SolverGeometryExtension::getClassTypeId())) { + continue; + } + + auto solvext = std::static_pointer_cast( + geo->getExtension(Sketcher::SolverGeometryExtension::getClassTypeId()).lock()); + + if (solvext->getGeometry() + != Sketcher::SolverGeometryExtension::NotFullyConstraint) { + continue; + } + + // The solver differentiates whether the parameters that are dependent are not + // those of start, end, mid, and assigns them to the edge (edge params = curve + // params - parms of start, end, mid). The user looking at the UI expects that + // the edge of a NotFullyConstraint geometry will always move, even if the edge + // parameters are independent, for example if mid is the only dependent + // parameter. In other words, the user could reasonably restrict the edge to + // reach a fully constrained element. Under this understanding, the edge + // parameter would always be dependent, unless the element is fully constrained. + // + // While this is ok from a user visual expectation point of view, it leads to a + // loss of information of whether restricting the point start, end, mid that is + // dependent may suffice, or even if such points are restricted, the edge would + // still need to be restricted. + // + // Because Python gets the information in this function, it would lead to Python + // users having access to a lower amount of detail. + // + // For this reason, this function returns edge as dependent parameter if and + // only if constraining the parameters of the points would not suffice to + // constraint the element. + if (solvext->getEdge() == SolverGeometryExtension::Dependent) + geometrymap.emplace_back(geoid, Sketcher::PointPos::none); + if (solvext->getStart() == SolverGeometryExtension::Dependent) + geometrymap.emplace_back(geoid, Sketcher::PointPos::start); + if (solvext->getEnd() == SolverGeometryExtension::Dependent) + geometrymap.emplace_back(geoid, Sketcher::PointPos::start); + if (solvext->getMid() == SolverGeometryExtension::Dependent) + geometrymap.emplace_back(geoid, Sketcher::PointPos::start); } } From 5c523534809d43812651c736d0d66db4d3138610 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Fri, 28 Jun 2024 15:15:28 +0530 Subject: [PATCH 03/26] [Sketcher] Refactor `SketchObject::join()` (expected trivial) No tests are added since this commit only adjusts if-then statements. --- src/Mod/Sketcher/App/SketchObject.cpp | 32 ++++++++++++--------------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index ae18fc8904..754889d05e 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -4449,11 +4449,9 @@ int SketchObject::join(int geoId1, Sketcher::PointPos posId1, int geoId2, Sketch // end knots can have a multiplicity of (degree + 1) if (bsp1->getDegree() < newMults.back()) { + newMults.back() = bsp1->getDegree(); if (makeC1Continuous) { - newMults.back() = bsp1->getDegree()-1; - } - else { - newMults.back() = bsp1->getDegree(); + newMults.back() -= 1; } } @@ -4471,24 +4469,22 @@ int SketchObject::join(int geoId1, Sketcher::PointPos posId1, int geoId2, Sketch THROWM(ValueError, "Failed to create joined curve."); return -1; } - else { - exposeInternalGeometry(newGeoId); - setConstruction(newGeoId, GeometryFacade::getConstruction(geo1)); - // TODO: transfer constraints on the non-connected ends - auto otherPosId1 = (Sketcher::PointPos::start == posId1) ? Sketcher::PointPos::end - : Sketcher::PointPos::start; - auto otherPosId2 = (Sketcher::PointPos::start == posId2) ? Sketcher::PointPos::end - : Sketcher::PointPos::start; + exposeInternalGeometry(newGeoId); + setConstruction(newGeoId, GeometryFacade::getConstruction(geo1)); - transferConstraints(geoId1, otherPosId1, newGeoId, PointPos::start, true); - transferConstraints(geoId2, otherPosId2, newGeoId, PointPos::end, true); + // TODO: transfer constraints on the non-connected ends + auto otherPosId1 = (Sketcher::PointPos::start == posId1) ? Sketcher::PointPos::end + : Sketcher::PointPos::start; + auto otherPosId2 = (Sketcher::PointPos::start == posId2) ? Sketcher::PointPos::end + : Sketcher::PointPos::start; - delGeometries({geoId1, geoId2}); - return 0; - } + transferConstraints(geoId1, otherPosId1, newGeoId, PointPos::start, true); + transferConstraints(geoId2, otherPosId2, newGeoId, PointPos::end, true); - return -1; + delGeometries({geoId1, geoId2}); + + return 0; } bool SketchObject::isExternalAllowed(App::Document* pDoc, App::DocumentObject* pObj, From 2249c99b33c1695e983981ec6bced8509d668f44 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Thu, 2 Jan 2025 10:27:41 +0530 Subject: [PATCH 04/26] [Sketcher] Write `SketchObject::replaceGeometries()` --- src/Mod/Sketcher/App/SketchObject.cpp | 43 ++++++++++++++++++++++++++- src/Mod/Sketcher/App/SketchObject.h | 6 ++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 754889d05e..f9056c56ba 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -1804,12 +1804,19 @@ int SketchObject::delGeometry(int GeoId, bool deleteinternalgeo) } int SketchObject::delGeometries(const std::vector& GeoIds) +{ + return delGeometries(GeoIds.begin(), GeoIds.end()); +} + +template +int SketchObject::delGeometries(InputIt first, InputIt last) { std::vector sGeoIds; std::vector negativeGeoIds; // Separate GeoIds into negative (external) and non-negative GeoIds - for (int geoId : GeoIds) { + for (auto it = first; it != last; ++it) { + int geoId = *it; if (geoId < 0 && geoId <= GeoEnum::RefExt) { negativeGeoIds.push_back(geoId); } @@ -1928,6 +1935,40 @@ int SketchObject::delGeometriesExclusiveList(const std::vector& GeoIds) return 0; } +void SketchObject::replaceGeometries(std::vector oldGeoIds, + std::vector& newGeos) +{ + auto vals = getInternalGeometry(); + auto newVals(vals); + + if (std::any_of(oldGeoIds.begin(), oldGeoIds.end(), [](auto geoId) { + return geoId < 0; + })) { + THROWM(ValueError, "Cannot replace external geometries and axes."); + } + + auto oldGeoIdIter = oldGeoIds.begin(); + auto newGeoIter = newGeos.begin(); + + for (; oldGeoIdIter != oldGeoIds.end() && newGeoIter != newGeos.end(); + ++oldGeoIdIter, ++newGeoIter) { + GeometryFacade::copyId(getGeometry(*oldGeoIdIter), *newGeoIter); + newVals[*oldGeoIdIter] = *newGeoIter; + } + + if (newGeoIter != newGeos.end()) { + for (; newGeoIter != newGeos.end(); ++newGeoIter) { + generateId(*newGeoIter); + newVals.push_back(*newGeoIter); + } + } + else { + delGeometries(oldGeoIdIter, oldGeoIds.end()); + } + + Geometry.setValues(std::move(newVals)); +} + int SketchObject::deleteAllGeometry() { // no need to check input data validity as this is an sketchobject managed operation. diff --git a/src/Mod/Sketcher/App/SketchObject.h b/src/Mod/Sketcher/App/SketchObject.h index b132017bff..62a13821dd 100644 --- a/src/Mod/Sketcher/App/SketchObject.h +++ b/src/Mod/Sketcher/App/SketchObject.h @@ -157,6 +157,8 @@ public: int delGeometriesExclusiveList(const std::vector& GeoIds); /// Does the same as \a delGeometry but allows one to delete several geometries in one step int delGeometries(const std::vector& GeoIds); + template + int delGeometries(InputIt first, InputIt last); /// deletes all the elements/constraints of the sketch except for external geometry int deleteAllGeometry(); /// deletes all the constraints of the sketch @@ -171,6 +173,10 @@ public: int addConstraint(std::unique_ptr constraint); /// delete constraint int delConstraint(int ConstrId); + /// Replaces geometries at `oldGeoIds` with `newGeos`, lower Ids first. + /// If `oldGeoIds` is bigger, deletes the remaining. + /// If `newGeos` is bigger, adds the remaining geometries at the end. + void replaceGeometries(std::vector oldGeoIds, std::vector& newGeos); /** deletes a group of constraints at once, if norecomputes is active, the default behaviour is * that it will solve the sketch. * From bc2b35da4e278c19af95630fc19b239fac75c485 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sat, 29 Jun 2024 11:29:23 +0530 Subject: [PATCH 05/26] [Sketcher] Helper functions for deleting geometry Some tests probably needed --- src/Mod/Sketcher/App/SketchObject.cpp | 459 +++++++++++++++----------- 1 file changed, 269 insertions(+), 190 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index f9056c56ba..4cc9f34867 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -1770,19 +1770,11 @@ int SketchObject::delGeometry(int GeoId, bool deleteinternalgeo) const std::vector& constraints = this->Constraints.getValues(); std::vector newConstraints; newConstraints.reserve(constraints.size()); - for (auto cstr : constraints) { - if (cstr->First == GeoId || cstr->Second == GeoId || cstr->Third == GeoId) - continue; - if (cstr->First > GeoId || cstr->Second > GeoId || cstr->Third > GeoId) { - cstr = cstr->clone(); - if (cstr->First > GeoId) - cstr->First -= 1; - if (cstr->Second > GeoId) - cstr->Second -= 1; - if (cstr->Third > GeoId) - cstr->Third -= 1; + for (const auto& constr : constraints) { + auto newConstr = getConstraintAfterDeletingGeo(constr, GeoId); + if (newConstr) { + newConstraints.push_back(newConstr.release()); } - newConstraints.push_back(cstr); } // Block acceptGeometry in OnChanged to avoid unnecessary checks and updates @@ -1891,32 +1883,22 @@ int SketchObject::delGeometriesExclusiveList(const std::vector& GeoIds) // Copy the original constraints std::vector constraints; - for (const auto ptr : this->Constraints.getValues()) + for (const auto& ptr : this->Constraints.getValues()) { constraints.push_back(ptr->clone()); - std::vector filteredConstraints(0); - for (auto itGeo = sGeoIds.rbegin(); itGeo != sGeoIds.rend(); ++itGeo) { - int GeoId = *itGeo; - for (const auto& constr : constraints) { - Constraint* copiedConstr(constr); - if (constr->First == GeoId || - constr->Second == GeoId || - constr->Third == GeoId) { - delete copiedConstr; - continue; - } - - if (copiedConstr->First > GeoId) - copiedConstr->First -= 1; - if (copiedConstr->Second > GeoId) - copiedConstr->Second -= 1; - if (copiedConstr->Third > GeoId) - copiedConstr->Third -= 1; - filteredConstraints.push_back(copiedConstr); - } - - constraints = filteredConstraints; - filteredConstraints.clear(); } + for (auto itGeo = sGeoIds.rbegin(); itGeo != sGeoIds.rend(); ++itGeo) { + const int GeoId = *itGeo; + for (auto& constr : constraints) { + changeConstraintAfterDeletingGeo(constr, GeoId); + } + } + + constraints.erase(std::remove_if(constraints.begin(), + constraints.end(), + [](const auto& constr) { + return constr->Type == ConstraintType::None; + }), + constraints.end()); // Block acceptGeometry in OnChanged to avoid unnecessary checks and updates { @@ -3476,6 +3458,62 @@ void SketchObject::addConstraint(Sketcher::ConstraintType constrType, int firstG this->addConstraint(std::move(newConstr)); } +std::unique_ptr +SketchObject::getConstraintAfterDeletingGeo(const Constraint* constr, + const int deletedGeoId) const +{ + if (!constr) { + return nullptr; + } + + // TODO: While this is not incorrect, it recreates all constraints regardless of whether or not we need to. + auto newConstr = std::unique_ptr(constr->clone()); + + changeConstraintAfterDeletingGeo(newConstr.get(), deletedGeoId); + + if (newConstr->Type == ConstraintType::None) { + return nullptr; + } + + return newConstr; +} + +void SketchObject::changeConstraintAfterDeletingGeo(Constraint* constr, + const int deletedGeoId) const +{ + if (!constr) { + return; + } + + if (constr->First == deletedGeoId || + constr->Second == deletedGeoId || + constr->Third == deletedGeoId) { + constr->Type = ConstraintType::None; + return; + } + + int step = 1; + std::function needsUpdate = [&deletedGeoId](const int& givenId) -> bool { + return givenId > deletedGeoId; + }; + if (deletedGeoId < 0) { + step = -1; + needsUpdate = [&deletedGeoId](const int& givenId) -> bool { + return givenId < deletedGeoId && givenId != GeoEnum::GeoUndef; + }; + } + + if (needsUpdate(constr->First)) { + constr->First -= step; + } + if (needsUpdate(constr->Second)) { + constr->Second -= step; + } + if (needsUpdate(constr->Third)) { + constr->Third -= step; + } +} + bool SketchObject::seekTrimPoints(int GeoId, const Base::Vector3d& point, int& GeoId1, Base::Vector3d& intersect1, int& GeoId2, Base::Vector3d& intersect2) @@ -3528,31 +3566,47 @@ 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); - }; -#if 0 // 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, Constraint* constr) { - // TODO: Move code currently later in this method (that does as per the following description) here. /* It is possible that the trimming entity has both a PointOnObject constraint to the * trimmed entity, and a simple Tangent constraint to the trimmed entity. In this case we * want to change to a single end-to-end tangency, i.e we want to ensure that constrType1 is * set to Sketcher::Tangent, that the secondPos1 is captured from the PointOnObject, and * also make sure that the PointOnObject constraint is deleted. The below loop ensures this, * also in case the ordering of the constraints is first Tangent and then PointOnObject. */ + // if (!(constr->Type == Sketcher::Tangent + // || constr->Type == Sketcher::Perpendicular)) { + // return; + // } + + // if (constr->First == GeoId1 && constr->Second == GeoId) { + // constr->Type = constr->Type; + // if (secondPos == Sketcher::PointPos::none) + // secondPos = constr->FirstPos; + // delete_list.push_back(constrId); + // } + // else if (constr->First == GeoId && constr->Second == GeoId1) { + // constr->Type = constr->Type; + // if (secondPos == Sketcher::PointPos::none) + // secondPos = constr->SecondPos; + // delete_list.push_back(constrId); + // } }; -#endif + // makes an equality constraint between GeoId1 and GeoId2 auto constrainAsEqual = [this](int GeoId1, int GeoId2) { auto newConstr = std::make_unique(); @@ -3627,40 +3681,133 @@ 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; std::vector newIds; std::vector newGeos; std::vector newGeosAsConsts; + bool oldGeoIsConstruction = GeometryFacade::getConstruction(static_cast(geo)); - if (isClosedCurve(geo)) { - startPointRemains = false; - endPointRemains = false; - if (GeoId1 != GeoEnum::GeoUndef && GeoId2 != GeoEnum::GeoUndef) { - paramsOfNewGeos.emplace_back(point2Param, point1Param); + 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()); } + + 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 (GeoId1 != GeoEnum::GeoUndef) { - startPointRemains = true; - paramsOfNewGeos.emplace_back(firstParam, point1Param); + 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(); } - if (GeoId2 != GeoEnum::GeoUndef) { - endPointRemains = true; - paramsOfNewGeos.emplace_back(point2Param, lastParam); + else { + setUpNewGeoLimitsFromNonPeriodic(); } + + ok = createArcGeosForBSplineCurve(bsp); } - for (auto& [u1, u2] : paramsOfNewGeos) { - auto newGeo = geoAsCurve->createArc(u1, u2); - assert(newGeo); - newGeos.push_back(newGeo); - newGeosAsConsts.push_back(newGeo); + if (!ok) { + delGeometries(newIds); + return -1; } switch (newGeos.size()) { @@ -3685,15 +3832,16 @@ 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 (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. + // 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. int nonexistentGeoId = getHighestCurveIndex() + newIds.size(); newIds.front() = nonexistentGeoId; @@ -3731,94 +3879,79 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) const auto& allConstraints = this->Constraints.getValues(); - bool isPoint1ConstrainedOnGeoId1 = false; - bool isPoint2ConstrainedOnGeoId2 = false; + bool isGeoId1CoincidentOnPoint1 = false; + bool isGeoId2CoincidentOnPoint2 = 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::set< int, std::greater<> > geoIdsToBeDeleted; + std::vector 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 constraintWasModified = false; + bool newConstraintCreated = deriveConstraintsForPieces(GeoId, newIds, newGeosAsConsts, con, newConstraints); + // trim-specific changes once general changes are done switch (con->Type) { case PointOnObject: { - // we might want to transform this (and the new point-on-object constraints) into a coincidence - 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; - newConstraints.push_back(newConstr); - isPoint1ConstrainedOnGeoId1 = true; - constraintWasModified = true; + if (!newConstraintCreated) { + break; } - 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(); + 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)) { + // This concerns the start portion of the trim + newConstr->Type = Sketcher::Coincident; + newConstr->SecondPos = PointPos::end; + if (newConstr->First == GeoId1) { + isGeoId1CoincidentOnPoint1 = true; + } + } + if (newConstr->Second == newIds.back() && + isPointAtPosition(newConstr->First, newConstr->FirstPos, point2)) { + // This concerns the end portion of the trim newConstr->Type = Sketcher::Coincident; - newConstr->Second = newIds.back(); newConstr->SecondPos = PointPos::start; - newConstraints.push_back(newConstr); - isPoint2ConstrainedOnGeoId2 = true; - constraintWasModified = true; + if (newConstr->First == GeoId2) { + isGeoId2CoincidentOnPoint2 = true; + } } break; } case Tangent: case Perpendicular: { - // 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; + // TODO Tangent may have to be turned into endpoint-to-endpoint + // we might want to transform this into a coincidence + if (!newConstraintCreated) { + break; } - 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 && !isPoint1ConstrainedOnGeoId1) { + if (GeoId1 != GeoEnum::GeoUndef && !isGeoId1CoincidentOnPoint1) { Constraint* newConstr = new Constraint(); newConstr->First = newIds.front(); newConstr->FirstPos = PointPos::end; @@ -3839,7 +3972,7 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) newConstraints.push_back(newConstr); } - if (GeoId2 != GeoEnum::GeoUndef && !isPoint2ConstrainedOnGeoId2) { + if (GeoId2 != GeoEnum::GeoUndef && !isGeoId2CoincidentOnPoint2) { Constraint* newConstr = new Constraint(); newConstr->First = newIds.back(); newConstr->FirstPos = PointPos::start; @@ -3905,49 +4038,6 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) return 0; } -std::unique_ptr -SketchObject::getConstraintAfterDeletingGeo(const Constraint* constr, - const int deletedGeoId) const -{ - if (constr->First == deletedGeoId || - constr->Second == deletedGeoId || - constr->Third == deletedGeoId) { - return nullptr; - } - - // TODO: While this is not incorrect, it recreates all constraints regardless of whether or not we need to. - auto newConstr = std::unique_ptr(constr->clone()); - - changeConstraintAfterDeletingGeo(newConstr.get(), deletedGeoId); - - return newConstr; -} - -void SketchObject::changeConstraintAfterDeletingGeo(Constraint* constr, - const int deletedGeoId) const -{ - int step = 1; - std::function needsUpdate = [&deletedGeoId](const int& givenId) -> bool { - return givenId > deletedGeoId; - }; - if (deletedGeoId < 0) { - step = -1; - needsUpdate = [&deletedGeoId](const int& givenId) -> bool { - return givenId < deletedGeoId && givenId != GeoEnum::GeoUndef; - }; - } - - if (needsUpdate(constr->First)) { - constr->First -= step; - } - if (needsUpdate(constr->Second)) { - constr->Second -= step; - } - if (needsUpdate(constr->Third)) { - constr->Third -= step; - } -} - bool SketchObject::deriveConstraintsForPieces(const int oldId, const std::vector& newIds, const Constraint* con, @@ -4940,7 +5030,6 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, return Geometry.getSize() - 1; } - std::vector SketchObject::getSymmetric(const std::vector& geoIdList, std::map& geoIdMap, std::map& isStartEndInverted, @@ -5867,7 +5956,6 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 return Geometry.getSize() - 1; } - int SketchObject::removeAxesAlignment(const std::vector& geoIdList) { // no need to check input data validity as this is an sketchobject managed operation. @@ -8308,12 +8396,12 @@ void SketchObject::validateExternalLinks() refSubShape = refShape.getSubShape(SubElement.c_str()); } } - catch ( Base::IndexError& indexError) { + catch (Base::IndexError& indexError) { removeBadLink = true; Base::Console().Warning( this->getFullLabel(), (indexError.getMessage() + "\n").c_str()); } - catch ( Base::ValueError& valueError ) { + catch (Base::ValueError& valueError) { removeBadLink = true; Base::Console().Warning( this->getFullLabel(), (valueError.getMessage() + "\n").c_str()); @@ -8321,7 +8409,7 @@ void SketchObject::validateExternalLinks() catch (Standard_Failure&) { removeBadLink = true; } - if ( removeBadLink ) { + if (removeBadLink) { rebuild = true; Objects.erase(Objects.begin() + i); SubElements.erase(SubElements.begin() + i); @@ -8329,19 +8417,10 @@ void SketchObject::validateExternalLinks() const std::vector& constraints = Constraints.getValues(); std::vector newConstraints(0); int GeoId = GeoEnum::RefExt - i; - for (std::vector::const_iterator it = constraints.begin(); - it != constraints.end(); - ++it) { - if ((*it)->First != GeoId && (*it)->Second != GeoId && (*it)->Third != GeoId) { - Constraint* copiedConstr = (*it)->clone(); - if (copiedConstr->First < GeoId && copiedConstr->First != GeoEnum::GeoUndef) - copiedConstr->First += 1; - if (copiedConstr->Second < GeoId && copiedConstr->Second != GeoEnum::GeoUndef) - copiedConstr->Second += 1; - if (copiedConstr->Third < GeoId && copiedConstr->Third != GeoEnum::GeoUndef) - copiedConstr->Third += 1; - - newConstraints.push_back(copiedConstr); + for (const auto& constr : constraints) { + auto newConstr = getConstraintAfterDeletingGeo(constr, GeoId); + if (newConstr) { + newConstraints.push_back(newConstr.release()); } } From bd522fe189ce5750085349eb5b74d926f19c544f Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 25 Aug 2024 18:11:24 +0530 Subject: [PATCH 06/26] [planegcs] Refactor `GCS::System::initSolution()` --- src/Mod/Sketcher/App/planegcs/GCS.cpp | 79 ++++++++++++--------------- 1 file changed, 34 insertions(+), 45 deletions(-) diff --git a/src/Mod/Sketcher/App/planegcs/GCS.cpp b/src/Mod/Sketcher/App/planegcs/GCS.cpp index 68f4e2b54a..e159e98865 100644 --- a/src/Mod/Sketcher/App/planegcs/GCS.cpp +++ b/src/Mod/Sketcher/App/planegcs/GCS.cpp @@ -1833,12 +1833,9 @@ void System::initSolution(Algorithm alg) } std::vector clistR; if (!redundant.empty()) { - for (std::vector::const_iterator constr = clist.begin(); constr != clist.end(); - ++constr) { - if (redundant.count(*constr) == 0) { - clistR.push_back(*constr); - } - } + std::copy_if(clist.begin(), clist.end(), std::back_inserter(clistR), [this](auto constr) { + return this->redundant.count(constr) == 0; + }); } else { clistR = clist; @@ -1851,15 +1848,15 @@ void System::initSolution(Algorithm alg) } int cvtid = int(plist.size()); - for (std::vector::const_iterator constr = clistR.begin(); constr != clistR.end(); - ++constr, cvtid++) { - VEC_pD& cparams = c2p[*constr]; - for (VEC_pD::const_iterator param = cparams.begin(); param != cparams.end(); ++param) { - MAP_pD_I::const_iterator it = pIndex.find(*param); + for (const auto constr : clistR) { + VEC_pD& cparams = c2p[constr]; + for (const auto param : cparams) { + MAP_pD_I::const_iterator it = pIndex.find(param); if (it != pIndex.end()) { boost::add_edge(cvtid, it->second, g); } } + ++cvtid; } VEC_I components(boost::num_vertices(g)); @@ -1875,26 +1872,21 @@ void System::initSolution(Algorithm alg) { VEC_pD reducedParams = plist; - for (std::vector::const_iterator constr = clistR.begin(); - constr != clistR.end(); - ++constr) { - if ((*constr)->getTag() >= 0 && (*constr)->getTypeId() == Equal) { - MAP_pD_I::const_iterator it1, it2; - it1 = pIndex.find((*constr)->params()[0]); - it2 = pIndex.find((*constr)->params()[1]); - if (it1 != pIndex.end() && it2 != pIndex.end()) { - reducedConstrs.insert(*constr); - double* p_kept = reducedParams[it1->second]; - double* p_replaced = reducedParams[it2->second]; - for (int i = 0; i < int(plist.size()); ++i) { - if (reducedParams[i] == p_replaced) { - reducedParams[i] = p_kept; - } - } - } + for (const auto& constr : clistR) { + if (!(constr->getTag() >= 0 && constr->getTypeId() == Equal)) { + continue; } + const auto it1 = pIndex.find(constr->params()[0]); + const auto it2 = pIndex.find(constr->params()[1]); + if (it1 == pIndex.end() || it2 == pIndex.end()) { + continue; + } + reducedConstrs.insert(constr); + double* p_kept = reducedParams[it1->second]; + double* p_replaced = reducedParams[it2->second]; + std::replace(reducedParams.begin(), reducedParams.end(), p_replaced, p_kept); } - for (int i = 0; i < int(plist.size()); ++i) { + for (size_t i = 0; i < plist.size(); ++i) { if (plist[i] != reducedParams[i]) { int cid = components[i]; reductionmaps[cid][plist[i]] = reducedParams[i]; @@ -1904,9 +1896,9 @@ void System::initSolution(Algorithm alg) clists.clear(); // destroy any lists clists.resize(componentsSize); // create empty lists to be filled in - int i = int(plist.size()); + size_t i = plist.size(); for (std::vector::const_iterator constr = clistR.begin(); constr != clistR.end(); - ++constr, i++) { + ++constr, ++i) { if (reducedConstrs.count(*constr) == 0) { int cid = components[i]; clists[cid].push_back(*constr); @@ -1915,28 +1907,25 @@ void System::initSolution(Algorithm alg) plists.clear(); // destroy any lists plists.resize(componentsSize); // create empty lists to be filled in - for (int i = 0; i < int(plist.size()); ++i) { + for (size_t i = 0; i < plist.size(); ++i) { int cid = components[i]; plists[cid].push_back(plist[i]); } // calculates subSystems and subSystemsAux from clists, plists and reductionmaps clearSubSystems(); - for (std::size_t cid = 0; cid < clists.size(); cid++) { + subSystems.resize(clists.size(), nullptr); + subSystemsAux.resize(clists.size(), nullptr); + for (std::size_t cid = 0; cid < clists.size(); ++cid) { std::vector clist0, clist1; - for (std::vector::const_iterator constr = clists[cid].begin(); - constr != clists[cid].end(); - ++constr) { - if ((*constr)->getTag() >= 0) { - clist0.push_back(*constr); - } - else { // move or distance from reference constraints - clist1.push_back(*constr); - } - } + std::partition_copy(clists[cid].begin(), + clists[cid].end(), + std::back_inserter(clist0), + std::back_inserter(clist1), + [](auto constr) { + return constr->getTag() >= 0; + }); - subSystems.push_back(nullptr); - subSystemsAux.push_back(nullptr); if (!clist0.empty()) { subSystems[cid] = new SubSystem(clist0, plists[cid], reductionmaps[cid]); } From a2521d079859477b0c3b980370d76881c2b0e5e3 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 25 Aug 2024 20:14:29 +0530 Subject: [PATCH 07/26] [planegcs] Refactor `identifyConflictingRedundantConstraints()` --- src/Mod/Sketcher/App/planegcs/GCS.cpp | 230 +++++++++++++------------- 1 file changed, 112 insertions(+), 118 deletions(-) diff --git a/src/Mod/Sketcher/App/planegcs/GCS.cpp b/src/Mod/Sketcher/App/planegcs/GCS.cpp index e159e98865..386c16b52f 100644 --- a/src/Mod/Sketcher/App/planegcs/GCS.cpp +++ b/src/Mod/Sketcher/App/planegcs/GCS.cpp @@ -2088,7 +2088,8 @@ int System::solve_BFGS(SubSystem* subsys, bool /*isFine*/, bool isRedundantsolvi } break; } - if (err > divergingLim || err != err) { // check for diverging and NaN + if (err > divergingLim || err != err) { + // check for diverging and NaN if (debugMode == IterationLevel) { std::stringstream stream; stream << "BFGS Failed: Diverging!!: " @@ -5596,23 +5597,23 @@ void System::identifyConflictingRedundantConstraints( SET_I satisfiedGroups; while (1) { // conflictingMap contains all the eligible constraints of conflict groups not yet - // satisfied. as groups get satisfied, the map created on every iteration is smaller, until + // satisfied. As groups get satisfied, the map created on every iteration is smaller, until // such time it is empty and the infinite loop is exited. The guarantee that the loop will // be exited originates from the fact that in each iteration the algorithm will select one // constraint from the conflict groups, which will satisfy at least one group. std::map conflictingMap; for (std::size_t i = 0; i < conflictGroups.size(); i++) { - if (satisfiedGroups.count(i) == 0) { - for (std::size_t j = 0; j < conflictGroups[i].size(); j++) { - Constraint* constr = conflictGroups[i][j]; - bool isinternalalignment = - (constr->isInternalAlignment() == Constraint::Alignment::InternalAlignment); - bool priorityconstraint = (constr->getTag() == 0); - if (!priorityconstraint && !isinternalalignment) { // exclude constraints - // tagged with zero and - // internal alignment - conflictingMap[constr].insert(i); - } + if (satisfiedGroups.count(i) != 0) { + continue; + } + + for (const auto& constr : conflictGroups[i]) { + bool isinternalalignment = + (constr->isInternalAlignment() == Constraint::Alignment::InternalAlignment); + bool priorityconstraint = (constr->getTag() == 0); + if (!priorityconstraint && !isinternalalignment) { + // exclude constraints tagged with zero and internal alignment + conflictingMap[constr].insert(i); } } } @@ -5621,65 +5622,61 @@ void System::identifyConflictingRedundantConstraints( break; } - int maxPopularity = 0; - Constraint* mostPopular = nullptr; - for (std::map::const_iterator it = conflictingMap.begin(); - it != conflictingMap.end(); - ++it) { + /* This is a heuristic algorithm to propose the user which constraints from a + * redundant/conflicting set should be removed. It is based on the following principles: + * 1. if the current constraint is more popular than previous ones (appears in more + * sets), take it. This prioritises removal of constraints that cause several + * independent groups of constraints to be conflicting/redundant. It is based on the + * observation that the redundancy/conflict is caused by the lesser amount of + * constraints. + * 2. if there is already a constraint ranking in the contest, and the current one is as + * popular, prefer the constraint that removes a lesser amount of DoFs. This prioritises + * removal of sketcher constraints (not solver constraints) that generates a higher + * amount of solver constraints. It is based on the observation that constraints taking + * a higher amount of DoFs (such as symmetry) are preferred by the user, who may not see + * the redundancy of simpler ones. + * 3. if there is already a constraint ranking in the context, the current one is as + * popular, and they remove the same amount of DoFs, prefer removal of the latest + * introduced. + */ + auto iterMostPopular = std::max_element( + conflictingMap.begin(), + conflictingMap.end(), + [&tagmultiplicity](const auto& pair1, const auto& pair2) { + size_t sizeOfSet1 = pair1.second.size(); + size_t sizeOfSet2 = pair2.second.size(); + auto tag1 = pair1.first->getTag(); + auto tag2 = pair2.first->getTag(); - int numberofsets = static_cast( - it->second.size()); // number of sets in which the constraint appears + return (sizeOfSet2 > sizeOfSet1 // (1) + || (sizeOfSet2 == sizeOfSet1 + && tagmultiplicity.at(tag2) < tagmultiplicity.at(tag1)) // (2) + || (sizeOfSet2 == sizeOfSet1 + && tagmultiplicity.at(tag2) == tagmultiplicity.at(tag1) + && tag2 > tag1)); // (3) + }); - /* This is a heuristic algorithm to propose the user which constraints from a - * redundant/conflicting set should be removed. It is based on the following principles: - * 1. if the current constraint is more popular than previous ones (appears in more - * sets), take it. This prioritises removal of constraints that cause several - * independent groups of constraints to be conflicting/redundant. It is based on the - * observation that the redundancy/conflict is caused by the lesser amount of - * constraints. - * 2. if there is already a constraint ranking in the contest, and the current one is as - * popular, prefer the constraint that removes a lesser amount of DoFs. This prioritises - * removal of sketcher constraints (not solver constraints) that generates a higher - * amount of solver constraints. It is based on the observation that constraints taking - * a higher amount of DoFs (such as symmetry) are preferred by the user, who may not see - * the redundancy of simpler ones. - * 3. if there is already a constraint ranking in the context, the current one is as - * popular, and they remove the same amount of DoFs, prefer removal of the latest - * introduced. - */ + Constraint* mostPopular = iterMostPopular->first; + int maxPopularity = iterMostPopular->second.size(); - if ((numberofsets > maxPopularity || // (1) - (numberofsets == maxPopularity && mostPopular - && tagmultiplicity.at(it->first->getTag()) - < tagmultiplicity.at(mostPopular->getTag())) - || // (2) - - (numberofsets == maxPopularity && mostPopular - && tagmultiplicity.at(it->first->getTag()) - == tagmultiplicity.at(mostPopular->getTag()) - && it->first->getTag() > mostPopular->getTag())) // (3) - - ) { - mostPopular = it->first; - maxPopularity = numberofsets; - } + if (!(maxPopularity > 0)) { + continue; } - if (maxPopularity > 0) { - // adding for skipping not only the mostPopular, but also any other constraint in the - // conflicting map associated with the same tag (namely any other solver - // constraint associated with the same sketcher constraint that is also conflicting) - auto maxPopularityTag = mostPopular->getTag(); - for (const auto& c : conflictingMap) { - if (c.first->getTag() == maxPopularityTag) { - skipped.insert(c.first); - for (SET_I::const_iterator it = conflictingMap[c.first].begin(); - it != conflictingMap[c.first].end(); - ++it) { - satisfiedGroups.insert(*it); - } - } + // adding for skipping not only the mostPopular, but also any other constraint in the + // conflicting map associated with the same tag (namely any other solver + // constraint associated with the same sketcher constraint that is also conflicting) + auto maxPopularityTag = mostPopular->getTag(); + + for (const auto& [constr, conflSet] : conflictingMap) { + if (!(constr->getTag() == maxPopularityTag)) { + continue; } + + skipped.insert(constr); + std::copy(conflSet.begin(), + conflSet.end(), + std::inserter(satisfiedGroups, satisfiedGroups.begin())); } } @@ -5690,12 +5687,12 @@ void System::identifyConflictingRedundantConstraints( std::vector clistTmp; clistTmp.reserve(clist.size()); - for (std::vector::iterator constr = clist.begin(); constr != clist.end(); - ++constr) { - if ((*constr)->isDriving() && skipped.count(*constr) == 0) { - clistTmp.push_back(*constr); - } - } + std::copy_if(clist.begin(), + clist.end(), + std::back_inserter(clistTmp), + [&skipped](const auto& constr) { + return (constr->isDriving() && skipped.count(constr) == 0); + }); SubSystem* subSysTmp = new SubSystem(clistTmp, pdiagnoselist); int res = solve(subSysTmp, true, alg, true); @@ -5719,78 +5716,77 @@ void System::identifyConflictingRedundantConstraints( if (res == Success) { subSysTmp->applySolution(); - for (std::set::const_iterator constr = skipped.begin(); - constr != skipped.end(); - ++constr) { - double err = (*constr)->error(); - if (err * err < convergenceRedundant) { - redundant.insert(*constr); - } - } + std::copy_if(skipped.begin(), + skipped.end(), + std::inserter(redundant, redundant.begin()), + [this](const auto& constr) { + double err = constr->error(); + return (err * err < this->convergenceRedundant); + }); resetToReference(); if (debugMode == Minimal || debugMode == IterationLevel) { Base::Console().Log("Sketcher Redundant solving: %d redundants\n", redundant.size()); } + // TODO: Figure out why we need to iterate in reverse order and add explanation here. std::vector> conflictGroupsOrig = conflictGroups; conflictGroups.clear(); for (int i = conflictGroupsOrig.size() - 1; i >= 0; i--) { - bool isRedundant = false; - for (std::size_t j = 0; j < conflictGroupsOrig[i].size(); j++) { - if (redundant.count(conflictGroupsOrig[i][j]) > 0) { - isRedundant = true; - - if (debugMode == IterationLevel) { - Base::Console().Log("(Partially) Redundant, Group %d, index %d, Tag: %d\n", - i, - j, - (conflictGroupsOrig[i][j])->getTag()); - } - - break; - } - } - if (!isRedundant) { + auto iterRedundantEntry = std::find_if(conflictGroups[i].begin(), + conflictGroups[i].end(), + [this](const auto item) { + return (this->redundant.count(item) > 0); + }); + bool hasRedundant = (iterRedundantEntry != conflictGroups[i].end()); + if (!hasRedundant) { conflictGroups.push_back(conflictGroupsOrig[i]); + continue; } - else { - constrNum--; + + if (debugMode == IterationLevel) { + Base::Console().Log("(Partially) Redundant, Group %d, index %d, Tag: %d\n", + i, + iterRedundantEntry - conflictGroupsOrig[i].begin(), + (*iterRedundantEntry)->getTag()); } + + constrNum--; } } delete subSysTmp; // simplified output of conflicting tags SET_I conflictingTagsSet; - for (std::size_t i = 0; i < conflictGroups.size(); i++) { - for (std::size_t j = 0; j < conflictGroups[i].size(); j++) { - bool isinternalalignment = (conflictGroups[i][j]->isInternalAlignment() - == Constraint::Alignment::InternalAlignment); - if (conflictGroups[i][j]->getTag() != 0 - && !isinternalalignment) { // exclude constraints tagged with zero and internal - // alignment - conflictingTagsSet.insert(conflictGroups[i][j]->getTag()); - } - } + for (const auto& cGroup : conflictGroups) { + // exclude internal alignment + std::transform(cGroup.begin(), + cGroup.end(), + std::inserter(conflictingTagsSet, conflictingTagsSet.begin()), + [](const auto& constr) { + bool isinternalalignment = (constr->isInternalAlignment() + == Constraint::Alignment::InternalAlignment); + return (isinternalalignment ? 0 : constr->getTag()); + }); } + // exclude constraints tagged with zero + conflictingTagsSet.erase(0); + conflictingTags.resize(conflictingTagsSet.size()); std::copy(conflictingTagsSet.begin(), conflictingTagsSet.end(), conflictingTags.begin()); // output of redundant tags SET_I redundantTagsSet, partiallyRedundantTagsSet; - for (std::set::iterator constr = redundant.begin(); constr != redundant.end(); - ++constr) { - redundantTagsSet.insert((*constr)->getTag()); - partiallyRedundantTagsSet.insert((*constr)->getTag()); + for (const auto& constr : redundant) { + redundantTagsSet.insert(constr->getTag()); + partiallyRedundantTagsSet.insert(constr->getTag()); } // remove tags represented at least in one non-redundant constraint - for (std::vector::iterator constr = clist.begin(); constr != clist.end(); - ++constr) { - if (redundant.count(*constr) == 0) { - redundantTagsSet.erase((*constr)->getTag()); + for (const auto& constr : clist) { + if (redundant.count(constr) == 0) { + redundantTagsSet.erase(constr->getTag()); } } @@ -5896,7 +5892,6 @@ double lineSearch(SubSystem* subsys, Eigen::VectorXd& xdir) return alphaStar; } - void free(VEC_pD& doublevec) { for (VEC_pD::iterator it = doublevec.begin(); it != doublevec.end(); ++it) { @@ -5961,5 +5956,4 @@ void free(std::vector& subsysvec) } } - } // namespace GCS From 2025c009cdc0a7bf063689ac8dc190113884c3eb Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Tue, 27 Aug 2024 04:42:28 +0530 Subject: [PATCH 08/26] [planegcs] Refactor conditions & loops in `GCS.cpp` for readability --- src/Mod/Sketcher/App/planegcs/GCS.cpp | 432 ++++++++++++++------------ 1 file changed, 225 insertions(+), 207 deletions(-) diff --git a/src/Mod/Sketcher/App/planegcs/GCS.cpp b/src/Mod/Sketcher/App/planegcs/GCS.cpp index 386c16b52f..17cdc21a21 100644 --- a/src/Mod/Sketcher/App/planegcs/GCS.cpp +++ b/src/Mod/Sketcher/App/planegcs/GCS.cpp @@ -1827,10 +1827,13 @@ void System::initSolution(Algorithm alg) // diagnose conflicting or redundant constraints if (!hasDiagnosis) { diagnose(alg); - if (!hasDiagnosis) { - return; - } } + + // if still no diagnosis after explicitly calling `diagnose`, nothing to do here + if (!hasDiagnosis) { + return; + } + std::vector clistR; if (!redundant.empty()) { std::copy_if(clist.begin(), clist.end(), std::back_inserter(clistR), [this](auto constr) { @@ -1894,15 +1897,18 @@ void System::initSolution(Algorithm alg) } } + // TODO: Why are the later (constraint-related) items added first? + // Adding plist-related items first would simplify assignment of `i`, but is not a big expense + // overall. Leaving as is to avoid any unintended consequences. clists.clear(); // destroy any lists clists.resize(componentsSize); // create empty lists to be filled in size_t i = plist.size(); - for (std::vector::const_iterator constr = clistR.begin(); constr != clistR.end(); - ++constr, ++i) { - if (reducedConstrs.count(*constr) == 0) { + for (const auto& constr : clistR) { + if (reducedConstrs.count(constr) == 0) { int cid = components[i]; - clists[cid].push_back(*constr); + clists[cid].push_back(constr); } + ++i; } plists.clear(); // destroy any lists @@ -2058,15 +2064,18 @@ int System::solve_BFGS(SubSystem* subsys, bool /*isFine*/, bool isRedundantsolvi h = x - h; // = x - xold // double convergence = isFine ? convergence : XconvergenceRough; - int maxIterNumber = - (isRedundantsolving - ? (sketchSizeMultiplierRedundant ? maxIterRedundant * xsize : maxIterRedundant) - : (sketchSizeMultiplier ? maxIter * xsize : maxIter)); + int maxIterNumber = (sketchSizeMultiplier ? maxIter * xsize : maxIter); + double convCriterion = convergence; + if (isRedundantsolving) { + maxIterNumber = + (sketchSizeMultiplierRedundant ? maxIterRedundant * xsize : maxIterRedundant); + convCriterion = convergenceRedundant; + } if (debugMode == IterationLevel) { std::stringstream stream; - stream << "BFGS: convergence: " << (isRedundantsolving ? convergenceRedundant : convergence) - << ", xsize: " << xsize << ", maxIter: " << maxIterNumber << "\n"; + stream << "BFGS: convergence: " << convCriterion << ", xsize: " << xsize + << ", maxIter: " << maxIterNumber << "\n"; const std::string tmp = stream.str(); Base::Console().Log(tmp.c_str()); @@ -2075,9 +2084,9 @@ int System::solve_BFGS(SubSystem* subsys, bool /*isFine*/, bool isRedundantsolvi double divergingLim = 1e6 * err + 1e12; double h_norm {}; - for (int iter = 1; iter < maxIterNumber; iter++) { + for (int iter = 1; iter < maxIterNumber; ++iter) { h_norm = h.norm(); - if (h_norm <= (isRedundantsolving ? convergenceRedundant : convergence) || err <= smallF) { + if (h_norm <= convCriterion || err <= smallF) { if (debugMode == IterationLevel) { std::stringstream stream; stream << "BFGS Converged!!: " @@ -2142,7 +2151,7 @@ int System::solve_BFGS(SubSystem* subsys, bool /*isFine*/, bool isRedundantsolvi if (err <= smallF) { return Success; } - if (h.norm() <= (isRedundantsolving ? convergenceRedundant : convergence)) { + if (h.norm() <= convCriterion) { return Converged; } return Failed; @@ -2173,16 +2182,21 @@ int System::solve_LM(SubSystem* subsys, bool isRedundantsolving) subsys->calcResidual(e); e *= -1; - int maxIterNumber = - (isRedundantsolving - ? (sketchSizeMultiplierRedundant ? maxIterRedundant * xsize : maxIterRedundant) - : (sketchSizeMultiplier ? maxIter * xsize : maxIter)); + int maxIterNumber = (sketchSizeMultiplier ? maxIter * xsize : maxIter); double divergingLim = 1e6 * e.squaredNorm() + 1e12; - double eps = (isRedundantsolving ? LM_epsRedundant : LM_eps); - double eps1 = (isRedundantsolving ? LM_eps1Redundant : LM_eps1); - double tau = (isRedundantsolving ? LM_tauRedundant : LM_tau); + double eps = LM_eps; + double eps1 = LM_eps1; + double tau = LM_tau; + + if (isRedundantsolving) { + maxIterNumber = + (sketchSizeMultiplierRedundant ? maxIterRedundant * xsize : maxIterRedundant); + eps = LM_epsRedundant; + eps1 = LM_eps1Redundant; + tau = LM_tauRedundant; + } if (debugMode == IterationLevel) { std::stringstream stream; @@ -2197,21 +2211,21 @@ int System::solve_LM(SubSystem* subsys, bool isRedundantsolving) double nu = 2, mu = 0; int iter = 0, stop = 0; for (iter = 0; iter < maxIterNumber && !stop; ++iter) { - // check error double err = e.squaredNorm(); - if (err <= eps * eps) { // error is small, Success + if (err <= eps * eps) { + // error is small, Success stop = 1; break; } - else if (err > divergingLim || err != err) { // check for diverging and NaN + else if (err > divergingLim || err != err) { + // check for diverging and NaN stop = 6; break; } // J^T J, J^T e subsys->calcJacobi(J); - ; A = J.transpose() * J; g = J.transpose() * e; @@ -2246,7 +2260,6 @@ int System::solve_LM(SubSystem* subsys, bool isRedundantsolving) // check if solving works if (rel_error < 1e-5) { - // restrict h according to maxStep double scale = subsys->maxStep(h); if (scale < 1.) { @@ -2257,12 +2270,13 @@ int System::solve_LM(SubSystem* subsys, bool isRedundantsolving) x_new = x + h; h_norm = h.squaredNorm(); - if (h_norm <= eps1 * eps1 * x.norm()) { // relative change in p is small, stop + if (h_norm <= eps1 * eps1 * x.norm()) { + // relative change in p is small, stop stop = 3; break; } - else if (h_norm - >= (x.norm() + eps1) / (DBL_EPSILON * DBL_EPSILON)) { // almost singular + else if (h_norm >= (x.norm() + eps1) / (DBL_EPSILON * DBL_EPSILON)) { + // almost singular stop = 4; break; } @@ -2322,17 +2336,12 @@ int System::solve_LM(SubSystem* subsys, bool isRedundantsolving) return (stop == 1) ? Success : Failed; } - int System::solve_DL(SubSystem* subsys, bool isRedundantsolving) { #ifdef _GCS_EXTRACT_SOLVER_SUBSYSTEM_ extractSubsystem(subsys, isRedundantsolving); #endif - double tolg = (isRedundantsolving ? DL_tolgRedundant : DL_tolg); - double tolx = (isRedundantsolving ? DL_tolxRedundant : DL_tolx); - double tolf = (isRedundantsolving ? DL_tolfRedundant : DL_tolf); - int xsize = subsys->pSize(); int csize = subsys->cSize(); @@ -2340,10 +2349,19 @@ int System::solve_DL(SubSystem* subsys, bool isRedundantsolving) return Success; } - int maxIterNumber = - (isRedundantsolving - ? (sketchSizeMultiplierRedundant ? maxIterRedundant * xsize : maxIterRedundant) - : (sketchSizeMultiplier ? maxIter * xsize : maxIter)); + double tolg = DL_tolg; + double tolx = DL_tolx; + double tolf = DL_tolf; + + int maxIterNumber = (sketchSizeMultiplier ? maxIter * xsize : maxIter); + if (isRedundantsolving) { + tolg = DL_tolgRedundant; + tolx = DL_tolxRedundant; + tolf = DL_tolfRedundant; + + maxIterNumber = + (sketchSizeMultiplierRedundant ? maxIterRedundant * xsize : maxIterRedundant); + } if (debugMode == IterationLevel) { std::stringstream stream; @@ -2386,84 +2404,83 @@ int System::solve_DL(SubSystem* subsys, bool isRedundantsolving) double nu = 2.; int iter = 0, stop = 0, reduce = 0; while (!stop) { - // check if finished - if (fx_inf <= tolf) { // Success + if (fx_inf <= tolf) { + // Success stop = 1; + break; } else if (g_inf <= tolg) { stop = 2; + break; } else if (delta <= tolx * (tolx + x.norm())) { stop = 2; + break; } else if (iter >= maxIterNumber) { stop = 4; + break; } - else if (err > divergingLim || err != err) { // check for diverging and NaN + else if (err > divergingLim || err != err) { + // check for diverging and NaN stop = 6; - } - else { - // get the steepest descent direction - alpha = g.squaredNorm() / (Jx * g).squaredNorm(); - h_sd = alpha * g; - - // get the gauss-newton step - // https://forum.freecad.org/viewtopic.php?f=10&t=12769&start=50#p106220 - // https://forum.kde.org/viewtopic.php?f=74&t=129439#p346104 - switch (dogLegGaussStep) { - case FullPivLU: - h_gn = Jx.fullPivLu().solve(-fx); - break; - case LeastNormFullPivLU: - h_gn = Jx.adjoint() * (Jx * Jx.adjoint()).fullPivLu().solve(-fx); - break; - case LeastNormLdlt: - h_gn = Jx.adjoint() * (Jx * Jx.adjoint()).ldlt().solve(-fx); - break; - } - - double rel_error = (Jx * h_gn + fx).norm() / fx.norm(); - if (rel_error > 1e15) { - break; - } - - // compute the dogleg step - if (h_gn.norm() < delta) { - h_dl = h_gn; - if (h_dl.norm() <= tolx * (tolx + x.norm())) { - stop = 5; - break; - } - } - else if (alpha * g.norm() >= delta) { - h_dl = (delta / (alpha * g.norm())) * h_sd; - } - else { - // compute beta - double beta = 0; - Eigen::VectorXd b = h_gn - h_sd; - double bb = (b.transpose() * b).norm(); - double gb = (h_sd.transpose() * b).norm(); - double c = (delta + h_sd.norm()) * (delta - h_sd.norm()); - - if (gb > 0) { - beta = c / (gb + sqrt(gb * gb + c * bb)); - } - else { - beta = (sqrt(gb * gb + c * bb) - gb) / bb; - } - - // and update h_dl and dL with beta - h_dl = h_sd + beta * b; - } - } - - // see if we are already finished - if (stop) { break; } + // get the steepest descent direction + alpha = g.squaredNorm() / (Jx * g).squaredNorm(); + h_sd = alpha * g; + + // get the gauss-newton step + // https://forum.freecad.org/viewtopic.php?f=10&t=12769&start=50#p106220 + // https://forum.kde.org/viewtopic.php?f=74&t=129439#p346104 + switch (dogLegGaussStep) { + case FullPivLU: + h_gn = Jx.fullPivLu().solve(-fx); + break; + case LeastNormFullPivLU: + h_gn = Jx.adjoint() * (Jx * Jx.adjoint()).fullPivLu().solve(-fx); + break; + case LeastNormLdlt: + h_gn = Jx.adjoint() * (Jx * Jx.adjoint()).ldlt().solve(-fx); + break; + } + + double rel_error = (Jx * h_gn + fx).norm() / fx.norm(); + if (rel_error > 1e15) { + break; + } + + // compute the dogleg step + if (h_gn.norm() < delta) { + h_dl = h_gn; + if (h_dl.norm() <= tolx * (tolx + x.norm())) { + stop = 5; + break; + } + } + else if (alpha * g.norm() >= delta) { + h_dl = (delta / (alpha * g.norm())) * h_sd; + } + else { + // compute beta + double beta = 0; + Eigen::VectorXd b = h_gn - h_sd; + double bb = (b.transpose() * b).norm(); + double gb = (h_sd.transpose() * b).norm(); + double c = (delta + h_sd.norm()) * (delta - h_sd.norm()); + + if (gb > 0) { + beta = c / (gb + sqrt(gb * gb + c * bb)); + } + else { + beta = (sqrt(gb * gb + c * bb) - gb) / bb; + } + + // and update h_dl and dL with beta + h_dl = h_sd + beta * b; + } // get the new values double err_new; @@ -4974,7 +4991,6 @@ int System::diagnose(Algorithm alg) // like 0 and -1. std::map tagmultiplicity; - makeReducedJacobian(J, jacobianconstraintmap, pdiagnoselist, tagmultiplicity); // this function will exit with a diagnosis and, unless overridden by functions below, with full @@ -4982,10 +4998,6 @@ int System::diagnose(Algorithm alg) hasDiagnosis = true; dofs = pdiagnoselist.size(); - if (J.rows() > 0) { - emptyDiagnoseMatrix = false; - } - // There is a legacy decision to use QR decomposition. I (abdullah) do not know all the // consideration taken in that decisions. I see that: // - QR decomposition is able to provide information about the rank and @@ -5040,65 +5052,73 @@ int System::diagnose(Algorithm alg) } #endif + if (J.rows() == 0) { + return dofs; + } + + // From here on, presuming `J.rows() > 0`. + emptyDiagnoseMatrix = false; + if (qrAlgorithm == EigenDenseQR) { #ifdef PROFILE_DIAGNOSE Base::TimeElapsed DenseQR_start_time; #endif - if (J.rows() > 0) { - int rank = 0; // rank is not cheap to retrieve from qrJT in DenseQR - Eigen::MatrixXd R; - Eigen::FullPivHouseholderQR qrJT; - // Here we give the system the possibility to run the two QR decompositions in parallel, - // depending on the load of the system so we are using the default std::launch::async | - // std::launch::deferred policy, as nobody better than the system nows if it can run the - // task in parallel or is oversubscribed and should deferred it. Care to wait() for the - // future before any prospective detection of conflicting/redundant, because the - // redundant solve modifies pdiagnoselist and it would NOT be thread-safe. Care to call - // the thread with silent=true, unless the present thread does not use Base::Console, or - // the launch policy is set to std::launch::deferred policy, as it is not thread-safe to - // use them in both at the same time. - // - // identifyDependentParametersDenseQR(J, jacobianconstraintmap, pdiagnoselist, true) - // - auto fut = std::async(&System::identifyDependentParametersDenseQR, - this, - J, - jacobianconstraintmap, - pdiagnoselist, - true); - makeDenseQRDecomposition(J, jacobianconstraintmap, qrJT, rank, R); + int rank = 0; // rank is not cheap to retrieve from qrJT in DenseQR + Eigen::MatrixXd R; + Eigen::FullPivHouseholderQR qrJT; + // Here we give the system the possibility to run the two QR decompositions in parallel, + // depending on the load of the system so we are using the default std::launch::async | + // std::launch::deferred policy, as nobody better than the system nows if it can run the + // task in parallel or is oversubscribed and should deferred it. Care to wait() for the + // future before any prospective detection of conflicting/redundant, because the + // redundant solve modifies pdiagnoselist and it would NOT be thread-safe. Care to call + // the thread with silent=true, unless the present thread does not use Base::Console, or + // the launch policy is set to std::launch::deferred policy, as it is not thread-safe to + // use them in both at the same time. + // + // identifyDependentParametersDenseQR(J, jacobianconstraintmap, pdiagnoselist, true) + // + auto fut = std::async(&System::identifyDependentParametersDenseQR, + this, + J, + jacobianconstraintmap, + pdiagnoselist, + true); - int paramsNum = qrJT.rows(); - int constrNum = qrJT.cols(); + makeDenseQRDecomposition(J, jacobianconstraintmap, qrJT, rank, R); - // This function is legacy code that was used to obtain partial geometry dependency - // information from a SINGLE Dense QR decomposition. I am reluctant to remove it from - // here until everything new is well tested. - // identifyDependentGeometryParametersInTransposedJacobianDenseQRDecomposition( qrJT, - // pdiagnoselist, paramsNum, rank); + int paramsNum = qrJT.rows(); + int constrNum = qrJT.cols(); - fut.wait(); // wait for the execution of identifyDependentParametersSparseQR to finish + // This function is legacy code that was used to obtain partial geometry dependency + // information from a SINGLE Dense QR decomposition. I am reluctant to remove it from + // here until everything new is well tested. + // identifyDependentGeometryParametersInTransposedJacobianDenseQRDecomposition( qrJT, + // pdiagnoselist, paramsNum, rank); - dofs = paramsNum - rank; // unless overconstraint, which will be overridden below + fut.wait(); // wait for the execution of identifyDependentParametersSparseQR to finish - // Detecting conflicting or redundant constraints - if (constrNum > rank) { // conflicting or redundant constraints - int nonredundantconstrNum; - identifyConflictingRedundantConstraints(alg, - qrJT, - jacobianconstraintmap, - tagmultiplicity, - pdiagnoselist, - R, - constrNum, - rank, - nonredundantconstrNum); - if (paramsNum == rank && nonredundantconstrNum > rank) { // over-constrained - dofs = paramsNum - nonredundantconstrNum; - } + dofs = paramsNum - rank; // unless overconstraint, which will be overridden below + + // Detecting conflicting or redundant constraints + if (constrNum > rank) { + // conflicting or redundant constraints + int nonredundantconstrNum; + identifyConflictingRedundantConstraints(alg, + qrJT, + jacobianconstraintmap, + tagmultiplicity, + pdiagnoselist, + R, + constrNum, + rank, + nonredundantconstrNum); + if (paramsNum == rank && nonredundantconstrNum > rank) { // over-constrained + dofs = paramsNum - nonredundantconstrNum; } } + #ifdef PROFILE_DIAGNOSE Base::TimeElapsed DenseQR_end_time; @@ -5113,66 +5133,64 @@ int System::diagnose(Algorithm alg) #ifdef PROFILE_DIAGNOSE Base::TimeElapsed SparseQR_start_time; #endif - if (J.rows() > 0) { - int rank = 0; - Eigen::MatrixXd R; - Eigen::SparseQR, Eigen::COLAMDOrdering> SqrJT; - // Here we give the system the possibility to run the two QR decompositions in parallel, - // depending on the load of the system so we are using the default std::launch::async | - // std::launch::deferred policy, as nobody better than the system nows if it can run the - // task in parallel or is oversubscribed and should deferred it. Care to wait() for the - // future before any prospective detection of conflicting/redundant, because the - // redundant solve modifies pdiagnoselist and it would NOT be thread-safe. Care to call - // the thread with silent=true, unless the present thread does not use Base::Console, or - // the launch policy is set to std::launch::deferred policy, as it is not thread-safe to - // use them in both at the same time. - // - // identifyDependentParametersSparseQR(J, jacobianconstraintmap, pdiagnoselist, true) - // - // Debug: - // auto fut = - // std::async(std::launch::deferred,&System::identifyDependentParametersSparseQR, this, - // J, jacobianconstraintmap, pdiagnoselist, false); - auto fut = std::async(&System::identifyDependentParametersSparseQR, - this, - J, + int rank = 0; + Eigen::MatrixXd R; + Eigen::SparseQR, Eigen::COLAMDOrdering> SqrJT; + // Here we give the system the possibility to run the two QR decompositions in parallel, + // depending on the load of the system so we are using the default std::launch::async | + // std::launch::deferred policy, as nobody better than the system nows if it can run the + // task in parallel or is oversubscribed and should deferred it. Care to wait() for the + // future before any prospective detection of conflicting/redundant, because the + // redundant solve modifies pdiagnoselist and it would NOT be thread-safe. Care to call + // the thread with silent=true, unless the present thread does not use Base::Console, or + // the launch policy is set to std::launch::deferred policy, as it is not thread-safe to + // use them in both at the same time. + // + // identifyDependentParametersSparseQR(J, jacobianconstraintmap, pdiagnoselist, true) + // + // Debug: + // auto fut = + // std::async(std::launch::deferred,&System::identifyDependentParametersSparseQR, this, + // J, jacobianconstraintmap, pdiagnoselist, false); + auto fut = std::async(&System::identifyDependentParametersSparseQR, + this, + J, + jacobianconstraintmap, + pdiagnoselist, + /*silent=*/true); + + makeSparseQRDecomposition(J, jacobianconstraintmap, - pdiagnoselist, - /*silent=*/true); + SqrJT, + rank, + R, + /*transposed=*/true, + /*silent=*/false); - makeSparseQRDecomposition(J, - jacobianconstraintmap, - SqrJT, - rank, - R, - /*transposed=*/true, - /*silent=*/false); + int paramsNum = SqrJT.rows(); + int constrNum = SqrJT.cols(); - int paramsNum = SqrJT.rows(); - int constrNum = SqrJT.cols(); + fut.wait(); // wait for the execution of identifyDependentParametersSparseQR to finish - fut.wait(); // wait for the execution of identifyDependentParametersSparseQR to finish + dofs = paramsNum - rank; // unless overconstraint, which will be overridden below - dofs = paramsNum - rank; // unless overconstraint, which will be overridden below + // Detecting conflicting or redundant constraints + if (constrNum > rank) { + int nonredundantconstrNum; - // Detecting conflicting or redundant constraints - if (constrNum > rank) { + identifyConflictingRedundantConstraints(alg, + SqrJT, + jacobianconstraintmap, + tagmultiplicity, + pdiagnoselist, + R, + constrNum, + rank, + nonredundantconstrNum); - int nonredundantconstrNum; - - identifyConflictingRedundantConstraints(alg, - SqrJT, - jacobianconstraintmap, - tagmultiplicity, - pdiagnoselist, - R, - constrNum, - rank, - nonredundantconstrNum); - - if (paramsNum == rank && nonredundantconstrNum > rank) { // over-constrained - dofs = paramsNum - nonredundantconstrNum; - } + if (paramsNum == rank && nonredundantconstrNum > rank) { + // over-constrained + dofs = paramsNum - nonredundantconstrNum; } } From 568f984b0bfc4b22eaa5c44ecb9d809750bc0287 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Tue, 27 Aug 2024 09:00:36 +0530 Subject: [PATCH 09/26] [planegcs] Refactor pointer vector freeing Remove the type-checks in `free(std::vector& constrvec)` as well as checks for `nullptr` before deleting. --- src/Mod/Sketcher/App/planegcs/GCS.cpp | 66 +++++---------------------- src/Mod/Sketcher/App/planegcs/GCS.h | 6 +-- 2 files changed, 15 insertions(+), 57 deletions(-) diff --git a/src/Mod/Sketcher/App/planegcs/GCS.cpp b/src/Mod/Sketcher/App/planegcs/GCS.cpp index 17cdc21a21..6bf522a210 100644 --- a/src/Mod/Sketcher/App/planegcs/GCS.cpp +++ b/src/Mod/Sketcher/App/planegcs/GCS.cpp @@ -525,7 +525,7 @@ void System::clear() reference.clear(); clearSubSystems(); - free(clist); + deleteAllContent(clist); c2p.clear(); p2c.clear(); } @@ -5823,12 +5823,11 @@ void System::identifyConflictingRedundantConstraints( nonredundantconstrNum = constrNum; } - void System::clearSubSystems() { isInit = false; - free(subSystems); - free(subSystemsAux); + deleteAllContent(subSystems); + deleteAllContent(subSystemsAux); subSystems.clear(); subSystemsAux.clear(); } @@ -5910,67 +5909,26 @@ double lineSearch(SubSystem* subsys, Eigen::VectorXd& xdir) return alphaStar; } -void free(VEC_pD& doublevec) +void deleteAllContent(VEC_pD& doublevec) { - for (VEC_pD::iterator it = doublevec.begin(); it != doublevec.end(); ++it) { - if (*it) { - delete *it; - } + for (auto& doubleptr : doublevec) { + delete doubleptr; } doublevec.clear(); } -void free(std::vector& constrvec) +void deleteAllContent(std::vector& constrvec) { - for (std::vector::iterator constr = constrvec.begin(); constr != constrvec.end(); - ++constr) { - if (*constr) { - switch ((*constr)->getTypeId()) { - case Equal: - delete static_cast(*constr); - break; - case Difference: - delete static_cast(*constr); - break; - case P2PDistance: - delete static_cast(*constr); - break; - case P2PAngle: - delete static_cast(*constr); - break; - case P2LDistance: - delete static_cast(*constr); - break; - case PointOnLine: - delete static_cast(*constr); - break; - case Parallel: - delete static_cast(*constr); - break; - case Perpendicular: - delete static_cast(*constr); - break; - case L2LAngle: - delete static_cast(*constr); - break; - case MidpointOnLine: - delete static_cast(*constr); - break; - case None: - default: - delete *constr; - } - } + for (auto& constr : constrvec) { + delete constr; } constrvec.clear(); } -void free(std::vector& subsysvec) +void deleteAllContent(std::vector& subsysvec) { - for (std::vector::iterator it = subsysvec.begin(); it != subsysvec.end(); ++it) { - if (*it) { - delete *it; - } + for (auto& subsys : subsysvec) { + delete subsys; } } diff --git a/src/Mod/Sketcher/App/planegcs/GCS.h b/src/Mod/Sketcher/App/planegcs/GCS.h index 6b0b12da05..440c8d278e 100644 --- a/src/Mod/Sketcher/App/planegcs/GCS.h +++ b/src/Mod/Sketcher/App/planegcs/GCS.h @@ -633,9 +633,9 @@ protected: // Helper elements /////////////////////////////////////// -void free(VEC_pD& doublevec); -void free(std::vector& constrvec); -void free(std::vector& subsysvec); +void deleteAllContent(VEC_pD& doublevec); +void deleteAllContent(std::vector& constrvec); +void deleteAllContent(std::vector& subsysvec); } // namespace GCS From 23cd6cb54e10cbbb4c7476084da983b09d026477 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Thu, 26 Sep 2024 22:25:48 +0530 Subject: [PATCH 10/26] [Sketcher][test] Tests for expose/delete internal geometry --- tests/src/Mod/Sketcher/App/SketchObject.cpp | 201 ++++++++++++++++++++ 1 file changed, 201 insertions(+) diff --git a/tests/src/Mod/Sketcher/App/SketchObject.cpp b/tests/src/Mod/Sketcher/App/SketchObject.cpp index 9010c1749a..d05266ae26 100644 --- a/tests/src/Mod/Sketcher/App/SketchObject.cpp +++ b/tests/src/Mod/Sketcher/App/SketchObject.cpp @@ -48,6 +48,18 @@ void setupEllipse(Part::GeomEllipse& ellipse) ellipse.setMinorRadius(minorRadius); } +void setupArcOfHyperbola(Part::GeomArcOfHyperbola& arcOfHyperbola) +{ + Base::Vector3d coordsCenter(1.0, 2.0, 0.0); + double majorRadius = 4.0; + double minorRadius = 3.0; + double startParam = M_PI / 3, endParam = M_PI * 1.5; + arcOfHyperbola.setCenter(coordsCenter); + arcOfHyperbola.setMajorRadius(majorRadius); + arcOfHyperbola.setMinorRadius(minorRadius); + arcOfHyperbola.setRange(startParam, endParam, true); +} + void setupArcOfParabola(Part::GeomArcOfParabola& aop) { Base::Vector3d coordsCenter(1.0, 2.0, 0.0); @@ -631,6 +643,195 @@ TEST_F(SketchObjectTest, testConstraintAfterDeletingGeo) EXPECT_EQ(constr2.Type, Sketcher::ConstraintType::None); } +TEST_F(SketchObjectTest, testDeleteExposeInternalGeometryOfEllipse) +{ + // Arrange + Part::GeomEllipse ellipse; + setupEllipse(ellipse); + int geoId = getObject()->addGeometry(&ellipse); + + // Act + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(geoId); + + // Assert + // Ensure there's only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); + + // Act + // "Expose" internal geometry + getObject()->exposeInternalGeometry(geoId); + + // Assert + // Ensure all internal geometry is satisfied + // TODO: Also try to ensure types of geometries that have this type + const auto constraints = getObject()->Constraints.getValues(); + for (auto alignmentType : {Sketcher::InternalAlignmentType::EllipseMajorDiameter, + Sketcher::InternalAlignmentType::EllipseMinorDiameter, + Sketcher::InternalAlignmentType::EllipseFocus1, + Sketcher::InternalAlignmentType::EllipseFocus2}) { + // TODO: Ensure there exists one and only one curve with this type + int numConstraintsOfThisType = + std::count_if(constraints.begin(), + constraints.end(), + [&geoId, &alignmentType](const auto* constr) { + return constr->Type == Sketcher::ConstraintType::InternalAlignment + && constr->AlignmentType == alignmentType + && constr->Second == geoId; + }); + EXPECT_EQ(numConstraintsOfThisType, 1); + } + + // Act + // Delete internal geometry (again) + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(geoId); + + // Assert + // Ensure there's only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); +} + +TEST_F(SketchObjectTest, testDeleteExposeInternalGeometryOfHyperbola) +{ + // Arrange + Part::GeomArcOfHyperbola aoh; + setupArcOfHyperbola(aoh); + int geoId = getObject()->addGeometry(&aoh); + + // Act + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(geoId); + + // Assert + // Ensure there's only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); + + // Act + // "Expose" internal geometry + getObject()->exposeInternalGeometry(geoId); + + // Assert + // Ensure all internal geometry is satisfied + // TODO: Also try to ensure types of geometries that have this type + const auto constraints = getObject()->Constraints.getValues(); + for (auto alignmentType : {Sketcher::InternalAlignmentType::HyperbolaMajor, + Sketcher::InternalAlignmentType::HyperbolaMinor, + Sketcher::InternalAlignmentType::HyperbolaFocus}) { + // TODO: Ensure there exists one and only one curve with this type + int numConstraintsOfThisType = + std::count_if(constraints.begin(), + constraints.end(), + [&geoId, &alignmentType](const auto* constr) { + return constr->Type == Sketcher::ConstraintType::InternalAlignment + && constr->AlignmentType == alignmentType + && constr->Second == geoId; + }); + EXPECT_EQ(numConstraintsOfThisType, 1); + } + + // Act + // Delete internal geometry (again) + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(geoId); + + // Assert + // Ensure there's only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); +} + +TEST_F(SketchObjectTest, testDeleteExposeInternalGeometryOfParabola) +{ + // Arrange + Part::GeomArcOfParabola aoh; + setupArcOfParabola(aoh); + int geoId = getObject()->addGeometry(&aoh); + + // Act + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(geoId); + + // Assert + // Ensure there's only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); + + // Act + // "Expose" internal geometry + getObject()->exposeInternalGeometry(geoId); + + // Assert + // Ensure all internal geometry is satisfied + // TODO: Also try to ensure types of geometries that have this type + const auto constraints = getObject()->Constraints.getValues(); + for (auto alignmentType : {Sketcher::InternalAlignmentType::ParabolaFocalAxis, + Sketcher::InternalAlignmentType::ParabolaFocus}) { + // TODO: Ensure there exists one and only one curve with this type + int numConstraintsOfThisType = + std::count_if(constraints.begin(), + constraints.end(), + [&geoId, &alignmentType](const auto* constr) { + return constr->Type == Sketcher::ConstraintType::InternalAlignment + && constr->AlignmentType == alignmentType + && constr->Second == geoId; + }); + EXPECT_EQ(numConstraintsOfThisType, 1); + } + + // Act + // Delete internal geometry (again) + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(geoId); + + // Assert + // Ensure there's only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); +} + +TEST_F(SketchObjectTest, testDeleteExposeInternalGeometryOfBSpline) +{ + // NOTE: We test only non-periodic B-spline here. Periodic B-spline should behave exactly the + // same. + + // Arrange + auto nonPeriodicBSpline = createTypicalNonPeriodicBSpline(); + int geoId = getObject()->addGeometry(nonPeriodicBSpline.get()); + + // Act + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(geoId); + + // Assert + // Ensure there's only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); + + // Act + // "Expose" internal geometry + getObject()->exposeInternalGeometry(geoId); + + // Assert + // Ensure all internal geometry is satisfied + // TODO: Also try to ensure types of geometries that have this type + const auto constraints = getObject()->Constraints.getValues(); + std::map numConstraintsOfThisType; + for (auto alignmentType : {Sketcher::InternalAlignmentType::BSplineControlPoint, + Sketcher::InternalAlignmentType::BSplineKnotPoint}) { + // TODO: Ensure there exists one and only one curve with this type + numConstraintsOfThisType[alignmentType] = + std::count_if(constraints.begin(), + constraints.end(), + [&geoId, &alignmentType](const auto* constr) { + return constr->Type == Sketcher::ConstraintType::InternalAlignment + && constr->AlignmentType == alignmentType + && constr->Second == geoId; + }); + } + EXPECT_EQ(numConstraintsOfThisType[Sketcher::InternalAlignmentType::BSplineControlPoint], + nonPeriodicBSpline->countPoles()); + EXPECT_EQ(numConstraintsOfThisType[Sketcher::InternalAlignmentType::BSplineKnotPoint], + nonPeriodicBSpline->countKnots()); + + // Act + // Delete internal geometry (again) + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(geoId); + + // Assert + // Ensure there's only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); +} + TEST_F(SketchObjectTest, testSplitLineSegment) { // Arrange From 5a01d438f2d8fe73bd50a3ed2977003c2d29c084 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 22 Dec 2024 04:00:53 +0530 Subject: [PATCH 11/26] [Sketcher] Refactor internal geometry operations * Use range-for and rearrange for understandability. * Break down into individual functions by geomtype. * Also refactor `exposeInternalGeometryForType` * Use `Constraint::involves...()` in delete internal Suppress clang-tidy/clazy warnings by using `[[maybe_unused]]`. Co-authored-by: Chris Hennes --- src/Mod/Sketcher/App/SketchObject.cpp | 1691 +++++++++++++------------ src/Mod/Sketcher/App/SketchObject.h | 14 + 2 files changed, 907 insertions(+), 798 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 4cc9f34867..9b2acf87d7 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -6090,6 +6090,643 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) return 0; } +template <> +int SketchObject::exposeInternalGeometryForType(const int GeoId) +{ + const Part::Geometry* geo = getGeometry(GeoId); + // First we search what has to be restored + bool major = false; + bool minor = false; + bool focus1 = false; + bool focus2 = false; + + const std::vector& vals = Constraints.getValues(); + + for (const auto& constr : vals) { + if (constr->Type != Sketcher::InternalAlignment || constr->Second != GeoId) { + continue; + } + + switch (constr->AlignmentType) { + case Sketcher::EllipseMajorDiameter: + major = true; + break; + case Sketcher::EllipseMinorDiameter: + minor = true; + break; + case Sketcher::EllipseFocus1: + focus1 = true; + break; + case Sketcher::EllipseFocus2: + focus2 = true; + break; + default: + return -1; + } + } + + int currentgeoid = getHighestCurveIndex(); + int incrgeo = 0; + + Base::Vector3d center; + double majord; + double minord; + Base::Vector3d majdir; + + std::vector igeo; + std::vector icon; + + const Part::GeomEllipse* ellipse = static_cast(geo); + + center = ellipse->getCenter(); + majord = ellipse->getMajorRadius(); + minord = ellipse->getMinorRadius(); + majdir = ellipse->getMajorAxisDir(); + + Base::Vector3d mindir = Vector3d(-majdir.y, majdir.x); + + Base::Vector3d majorpositiveend = center + majord * majdir; + Base::Vector3d majornegativeend = center - majord * majdir; + Base::Vector3d minorpositiveend = center + minord * mindir; + Base::Vector3d minornegativeend = center - minord * mindir; + + double df = sqrt(majord * majord - minord * minord); + + Base::Vector3d focus1P = center + df * majdir; + Base::Vector3d focus2P = center - df * majdir; + + if (!major) { + Part::GeomLineSegment* lmajor = new Part::GeomLineSegment(); + lmajor->setPoints(majorpositiveend, majornegativeend); + + igeo.push_back(lmajor); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = EllipseMajorDiameter; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + incrgeo++; + } + if (!minor) { + Part::GeomLineSegment* lminor = new Part::GeomLineSegment(); + lminor->setPoints(minorpositiveend, minornegativeend); + + igeo.push_back(lminor); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = EllipseMinorDiameter; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + incrgeo++; + } + if (!focus1) { + Part::GeomPoint* pf1 = new Part::GeomPoint(); + pf1->setPoint(focus1P); + + igeo.push_back(pf1); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = EllipseFocus1; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->FirstPos = Sketcher::PointPos::start; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + incrgeo++; + } + if (!focus2) { + Part::GeomPoint* pf2 = new Part::GeomPoint(); + pf2->setPoint(focus2P); + igeo.push_back(pf2); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = EllipseFocus2; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->FirstPos = Sketcher::PointPos::start; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + } + + this->addGeometry(igeo, true); + this->addConstraints(icon); + + for (auto& geo : igeo) { + delete geo; + } + + for (auto& con : icon) { + delete con; + } + + return incrgeo;// number of added elements +} + +// TODO: This is a repeat of ellipse. Can we do some code reuse? +template <> +int SketchObject::exposeInternalGeometryForType(const int GeoId) +{ + const Part::Geometry* geo = getGeometry(GeoId); + // First we search what has to be restored + bool major = false; + bool minor = false; + bool focus1 = false; + bool focus2 = false; + + const std::vector& vals = Constraints.getValues(); + + for (const auto& constr : vals) { + if (constr->Type != Sketcher::InternalAlignment || constr->Second != GeoId) { + continue; + } + + switch (constr->AlignmentType) { + case Sketcher::EllipseMajorDiameter: + major = true; + break; + case Sketcher::EllipseMinorDiameter: + minor = true; + break; + case Sketcher::EllipseFocus1: + focus1 = true; + break; + case Sketcher::EllipseFocus2: + focus2 = true; + break; + default: + return -1; + } + } + + int currentgeoid = getHighestCurveIndex(); + int incrgeo = 0; + + Base::Vector3d center; + double majord; + double minord; + Base::Vector3d majdir; + + std::vector igeo; + std::vector icon; + + const Part::GeomArcOfEllipse* aoe = static_cast(geo); + + center = aoe->getCenter(); + majord = aoe->getMajorRadius(); + minord = aoe->getMinorRadius(); + majdir = aoe->getMajorAxisDir(); + + Base::Vector3d mindir = Vector3d(-majdir.y, majdir.x); + + Base::Vector3d majorpositiveend = center + majord * majdir; + Base::Vector3d majornegativeend = center - majord * majdir; + Base::Vector3d minorpositiveend = center + minord * mindir; + Base::Vector3d minornegativeend = center - minord * mindir; + + double df = sqrt(majord * majord - minord * minord); + + Base::Vector3d focus1P = center + df * majdir; + Base::Vector3d focus2P = center - df * majdir; + + if (!major) { + Part::GeomLineSegment* lmajor = new Part::GeomLineSegment(); + lmajor->setPoints(majorpositiveend, majornegativeend); + + igeo.push_back(lmajor); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = EllipseMajorDiameter; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + incrgeo++; + } + if (!minor) { + Part::GeomLineSegment* lminor = new Part::GeomLineSegment(); + lminor->setPoints(minorpositiveend, minornegativeend); + + igeo.push_back(lminor); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = EllipseMinorDiameter; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + incrgeo++; + } + if (!focus1) { + Part::GeomPoint* pf1 = new Part::GeomPoint(); + pf1->setPoint(focus1P); + + igeo.push_back(pf1); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = EllipseFocus1; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->FirstPos = Sketcher::PointPos::start; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + incrgeo++; + } + if (!focus2) { + Part::GeomPoint* pf2 = new Part::GeomPoint(); + pf2->setPoint(focus2P); + igeo.push_back(pf2); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = EllipseFocus2; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->FirstPos = Sketcher::PointPos::start; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + } + + this->addGeometry(igeo, true); + this->addConstraints(icon); + + for (auto& geo : igeo) { + delete geo; + } + + for (auto& con : icon) { + delete con; + } + + return incrgeo;// number of added elements + +} + +template <> +int SketchObject::exposeInternalGeometryForType(const int GeoId) +{ + const Part::Geometry* geo = getGeometry(GeoId); + // First we search what has to be restored + bool major = false; + bool minor = false; + bool focus = false; + + const std::vector& vals = Constraints.getValues(); + + for (auto const& constr : vals) { + if (constr->Type != Sketcher::InternalAlignment || constr->Second != GeoId) { + continue; + } + + switch (constr->AlignmentType) { + case Sketcher::HyperbolaMajor: + major = true; + break; + case Sketcher::HyperbolaMinor: + minor = true; + break; + case Sketcher::HyperbolaFocus: + focus = true; + break; + default: + return -1; + } + } + + int currentgeoid = getHighestCurveIndex(); + int incrgeo = 0; + + const Part::GeomArcOfHyperbola* aoh = static_cast(geo); + + Base::Vector3d center = aoh->getCenter(); + double majord = aoh->getMajorRadius(); + double minord = aoh->getMinorRadius(); + Base::Vector3d majdir = aoh->getMajorAxisDir(); + + std::vector igeo; + std::vector icon; + + Base::Vector3d mindir = Vector3d(-majdir.y, majdir.x); + + Base::Vector3d majorpositiveend = center + majord * majdir; + Base::Vector3d majornegativeend = center - majord * majdir; + Base::Vector3d minorpositiveend = majorpositiveend + minord * mindir; + Base::Vector3d minornegativeend = majorpositiveend - minord * mindir; + + double df = sqrt(majord * majord + minord * minord); + + Base::Vector3d focus1P = center + df * majdir; + + if (!major) { + Part::GeomLineSegment* lmajor = new Part::GeomLineSegment(); + lmajor->setPoints(majorpositiveend, majornegativeend); + + igeo.push_back(lmajor); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = Sketcher::HyperbolaMajor; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + incrgeo++; + } + if (!minor) { + Part::GeomLineSegment* lminor = new Part::GeomLineSegment(); + lminor->setPoints(minorpositiveend, minornegativeend); + + igeo.push_back(lminor); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = Sketcher::HyperbolaMinor; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + + incrgeo++; + } + if (!focus) { + Part::GeomPoint* pf1 = new Part::GeomPoint(); + pf1->setPoint(focus1P); + + igeo.push_back(pf1); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = Sketcher::HyperbolaFocus; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->FirstPos = Sketcher::PointPos::start; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + incrgeo++; + } + + this->addGeometry(igeo, true); + this->addConstraints(icon); + + for (auto& geo : igeo) { + delete geo; + } + + for (auto& con : icon) { + delete con; + } + + return incrgeo;// number of added elements +} + +template <> +int SketchObject::exposeInternalGeometryForType(const int GeoId) +{ + const Part::Geometry* geo = getGeometry(GeoId); + // First we search what has to be restored + bool focus = false; + bool focus_to_vertex = false; + + const std::vector& vals = Constraints.getValues(); + + for (auto const& constr : vals) { + if (constr->Type != Sketcher::InternalAlignment || constr->Second != GeoId) { + continue; + } + + switch (constr->AlignmentType) { + case Sketcher::ParabolaFocus: + focus = true; + break; + case Sketcher::ParabolaFocalAxis: + focus_to_vertex = true; + break; + default: + return -1; + } + } + + int currentgeoid = getHighestCurveIndex(); + int incrgeo = 0; + + const Part::GeomArcOfParabola* aop = static_cast(geo); + + Base::Vector3d center = aop->getCenter(); + Base::Vector3d focusp = aop->getFocus(); + + std::vector igeo; + std::vector icon; + + if (!focus) { + Part::GeomPoint* pf1 = new Part::GeomPoint(); + pf1->setPoint(focusp); + + igeo.push_back(pf1); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = Sketcher::ParabolaFocus; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->FirstPos = Sketcher::PointPos::start; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + incrgeo++; + } + + if (!focus_to_vertex) { + Part::GeomLineSegment* paxis = new Part::GeomLineSegment(); + paxis->setPoints(center, focusp); + + igeo.push_back(paxis); + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = Sketcher::ParabolaFocalAxis; + newConstr->First = currentgeoid + incrgeo + 1; + newConstr->FirstPos = Sketcher::PointPos::none; + newConstr->Second = GeoId; + + icon.push_back(newConstr); + + incrgeo++; + } + + this->addGeometry(igeo, true); + this->addConstraints(icon); + + for (auto& geo : igeo) { + delete geo; + } + + for (auto& con : icon) { + delete con; + } + + return incrgeo;// number of added elements +} + +template <> +int SketchObject::exposeInternalGeometryForType(const int GeoId) +{ + const Part::Geometry* geo = getGeometry(GeoId); + + const Part::GeomBSplineCurve* bsp = static_cast(geo); + // First we search what has to be restored + std::vector controlpointgeoids(bsp->countPoles(), GeoEnum::GeoUndef); + + std::vector knotgeoids(bsp->countKnots(), GeoEnum::GeoUndef); + + bool isfirstweightconstrained = false; + + std::vector::iterator it; + + const std::vector& vals = Constraints.getValues(); + + // search for existing poles + for (auto const& constr : vals) { + if (constr->Type != Sketcher::InternalAlignment || constr->Second != GeoId) { + continue; + } + + switch (constr->AlignmentType) { + case Sketcher::BSplineControlPoint: + controlpointgeoids[constr->InternalAlignmentIndex] = constr->First; + break; + case Sketcher::BSplineKnotPoint: + knotgeoids[constr->InternalAlignmentIndex] = constr->First; + break; + default: + return -1; + } + } + + if (controlpointgeoids[0] != GeoEnum::GeoUndef) { + // search for first pole weight constraint + auto it = std::find_if(vals.begin(), + vals.end(), + [&controlpointgeoids](const auto& constr) {return (constr->Type == Sketcher::Weight && constr->First == controlpointgeoids[0]);}); + isfirstweightconstrained = (it != vals.end()); + } + + int currentgeoid = getHighestCurveIndex(); + int incrgeo = 0; + + std::vector igeo; + std::vector icon; + + std::vector poles = bsp->getPoles(); + std::vector weights = bsp->getWeights(); + std::vector knots = bsp->getKnots(); + + double distance_p0_p1 = (poles[1] - poles[0]).Length();// for visual purposes only + + for (size_t index = 0; index < controlpointgeoids.size(); ++index) { + auto& cpGeoId = controlpointgeoids.at(index); + if (cpGeoId != GeoEnum::GeoUndef) { + continue; + } + + // if controlpoint not existing + Part::GeomCircle* pc = new Part::GeomCircle(); + pc->setCenter(poles[index]); + pc->setRadius(distance_p0_p1 / 6); + + igeo.push_back(pc); + incrgeo++; + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = Sketcher::BSplineControlPoint; + newConstr->First = currentgeoid + incrgeo; + newConstr->FirstPos = Sketcher::PointPos::mid; + newConstr->Second = GeoId; + newConstr->InternalAlignmentIndex = index; + + icon.push_back(newConstr); + + if (index == 0) { + controlpointgeoids[0] = currentgeoid + incrgeo; + if (weights[0] == 1.0) { + // if the first weight is 1.0 it's probably going to be non-rational + Sketcher::Constraint* newConstr3 = new Sketcher::Constraint(); + newConstr3->Type = Sketcher::Weight; + newConstr3->First = controlpointgeoids[0]; + newConstr3->setValue(weights[0]); + + icon.push_back(newConstr3); + + isfirstweightconstrained = true; + } + + continue; + } + + if (isfirstweightconstrained && weights[0] == weights[index]) { + // if pole-weight newly created AND first weight is radius-constrained, + // AND these weights are equal, constrain them to be equal + Sketcher::Constraint* newConstr2 = new Sketcher::Constraint(); + newConstr2->Type = Sketcher::Equal; + newConstr2->First = currentgeoid + incrgeo; + newConstr2->FirstPos = Sketcher::PointPos::none; + newConstr2->Second = controlpointgeoids[0]; + newConstr2->SecondPos = Sketcher::PointPos::none; + + icon.push_back(newConstr2); + } + } + + for (size_t index = 0; index < knotgeoids.size(); ++index) { + auto& kGeoId = knotgeoids.at(index); + if (kGeoId != GeoEnum::GeoUndef) { + continue; + } + + // if knot point not existing + Part::GeomPoint* kp = new Part::GeomPoint(); + + kp->setPoint(bsp->pointAtParameter(knots[index])); + + igeo.push_back(kp); + incrgeo++; + + Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = Sketcher::BSplineKnotPoint; + newConstr->First = currentgeoid + incrgeo; + newConstr->FirstPos = Sketcher::PointPos::start; + newConstr->Second = GeoId; + newConstr->InternalAlignmentIndex = index; + + icon.push_back(newConstr); + } + + Q_UNUSED(isfirstweightconstrained); + + this->addGeometry(igeo, true); + this->addConstraints(icon); + + for (auto& geo : igeo) { + delete geo; + } + + for (auto& con : icon) { + delete con; + } + + return incrgeo;// number of added elements +} + int SketchObject::exposeInternalGeometry(int GeoId) { if (GeoId < 0 || GeoId > getHighestCurveIndex()) @@ -6097,533 +6734,20 @@ int SketchObject::exposeInternalGeometry(int GeoId) const Part::Geometry* geo = getGeometry(GeoId); // Only for supported types - if (geo->is() - || geo->is()) { - // First we search what has to be restored - bool major = false; - bool minor = false; - bool focus1 = false; - bool focus2 = false; - - const std::vector& vals = Constraints.getValues(); - - for (std::vector::const_iterator it = vals.begin(); it != vals.end(); - ++it) { - if ((*it)->Type == Sketcher::InternalAlignment && (*it)->Second == GeoId) { - switch ((*it)->AlignmentType) { - case Sketcher::EllipseMajorDiameter: - major = true; - break; - case Sketcher::EllipseMinorDiameter: - minor = true; - break; - case Sketcher::EllipseFocus1: - focus1 = true; - break; - case Sketcher::EllipseFocus2: - focus2 = true; - break; - default: - return -1; - } - } - } - - int currentgeoid = getHighestCurveIndex(); - int incrgeo = 0; - - Base::Vector3d center; - double majord; - double minord; - Base::Vector3d majdir; - - std::vector igeo; - std::vector icon; - - if (geo->is()) { - const Part::GeomEllipse* ellipse = static_cast(geo); - - center = ellipse->getCenter(); - majord = ellipse->getMajorRadius(); - minord = ellipse->getMinorRadius(); - majdir = ellipse->getMajorAxisDir(); - } - else { - const Part::GeomArcOfEllipse* aoe = static_cast(geo); - - center = aoe->getCenter(); - majord = aoe->getMajorRadius(); - minord = aoe->getMinorRadius(); - majdir = aoe->getMajorAxisDir(); - } - - Base::Vector3d mindir = Vector3d(-majdir.y, majdir.x); - - Base::Vector3d majorpositiveend = center + majord * majdir; - Base::Vector3d majornegativeend = center - majord * majdir; - Base::Vector3d minorpositiveend = center + minord * mindir; - Base::Vector3d minornegativeend = center - minord * mindir; - - double df = sqrt(majord * majord - minord * minord); - - Base::Vector3d focus1P = center + df * majdir; - Base::Vector3d focus2P = center - df * majdir; - - if (!major) { - Part::GeomLineSegment* lmajor = new Part::GeomLineSegment(); - lmajor->setPoints(majorpositiveend, majornegativeend); - - igeo.push_back(lmajor); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = EllipseMajorDiameter; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->Second = GeoId; - - icon.push_back(newConstr); - incrgeo++; - } - if (!minor) { - Part::GeomLineSegment* lminor = new Part::GeomLineSegment(); - lminor->setPoints(minorpositiveend, minornegativeend); - - igeo.push_back(lminor); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = EllipseMinorDiameter; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->Second = GeoId; - - icon.push_back(newConstr); - incrgeo++; - } - if (!focus1) { - Part::GeomPoint* pf1 = new Part::GeomPoint(); - pf1->setPoint(focus1P); - - igeo.push_back(pf1); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = EllipseFocus1; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->FirstPos = Sketcher::PointPos::start; - newConstr->Second = GeoId; - - icon.push_back(newConstr); - incrgeo++; - } - if (!focus2) { - Part::GeomPoint* pf2 = new Part::GeomPoint(); - pf2->setPoint(focus2P); - igeo.push_back(pf2); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = EllipseFocus2; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->FirstPos = Sketcher::PointPos::start; - newConstr->Second = GeoId; - - icon.push_back(newConstr); - } - - this->addGeometry(igeo, true); - this->addConstraints(icon); - - for (std::vector::iterator it = igeo.begin(); it != igeo.end(); ++it) { - if (*it) - delete *it; - } - - for (std::vector::iterator it = icon.begin(); it != icon.end(); ++it) { - if (*it) - delete *it; - } - - icon.clear(); - igeo.clear(); - - return incrgeo;// number of added elements + if (geo->is()) { + return exposeInternalGeometryForType(GeoId); + } + else if (geo->is()) { + return exposeInternalGeometryForType(GeoId); } else if (geo->is()) { - // First we search what has to be restored - bool major = false; - bool minor = false; - bool focus = false; - - const std::vector& vals = Constraints.getValues(); - - for (std::vector::const_iterator it = vals.begin(); it != vals.end(); - ++it) { - if ((*it)->Type == Sketcher::InternalAlignment && (*it)->Second == GeoId) { - switch ((*it)->AlignmentType) { - case Sketcher::HyperbolaMajor: - major = true; - break; - case Sketcher::HyperbolaMinor: - minor = true; - break; - case Sketcher::HyperbolaFocus: - focus = true; - break; - default: - return -1; - } - } - } - - int currentgeoid = getHighestCurveIndex(); - int incrgeo = 0; - - const Part::GeomArcOfHyperbola* aoh = static_cast(geo); - - Base::Vector3d center = aoh->getCenter(); - double majord = aoh->getMajorRadius(); - double minord = aoh->getMinorRadius(); - Base::Vector3d majdir = aoh->getMajorAxisDir(); - - std::vector igeo; - std::vector icon; - - Base::Vector3d mindir = Vector3d(-majdir.y, majdir.x); - - Base::Vector3d majorpositiveend = center + majord * majdir; - Base::Vector3d majornegativeend = center - majord * majdir; - Base::Vector3d minorpositiveend = majorpositiveend + minord * mindir; - Base::Vector3d minornegativeend = majorpositiveend - minord * mindir; - - double df = sqrt(majord * majord + minord * minord); - - Base::Vector3d focus1P = center + df * majdir; - - if (!major) { - Part::GeomLineSegment* lmajor = new Part::GeomLineSegment(); - lmajor->setPoints(majorpositiveend, majornegativeend); - - igeo.push_back(lmajor); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = Sketcher::HyperbolaMajor; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->Second = GeoId; - - icon.push_back(newConstr); - incrgeo++; - } - if (!minor) { - Part::GeomLineSegment* lminor = new Part::GeomLineSegment(); - lminor->setPoints(minorpositiveend, minornegativeend); - - igeo.push_back(lminor); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = Sketcher::HyperbolaMinor; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->Second = GeoId; - - icon.push_back(newConstr); - - incrgeo++; - } - if (!focus) { - Part::GeomPoint* pf1 = new Part::GeomPoint(); - pf1->setPoint(focus1P); - - igeo.push_back(pf1); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = Sketcher::HyperbolaFocus; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->FirstPos = Sketcher::PointPos::start; - newConstr->Second = GeoId; - - icon.push_back(newConstr); - incrgeo++; - } - - this->addGeometry(igeo, true); - this->addConstraints(icon); - - for (std::vector::iterator it = igeo.begin(); it != igeo.end(); ++it) - if (*it) - delete *it; - - for (std::vector::iterator it = icon.begin(); it != icon.end(); ++it) - if (*it) - delete *it; - - icon.clear(); - igeo.clear(); - - return incrgeo;// number of added elements + return exposeInternalGeometryForType(GeoId); } else if (geo->is()) { - // First we search what has to be restored - bool focus = false; - bool focus_to_vertex = false; - - const std::vector& vals = Constraints.getValues(); - - for (std::vector::const_iterator it = vals.begin(); it != vals.end(); - ++it) { - if ((*it)->Type == Sketcher::InternalAlignment && (*it)->Second == GeoId) { - switch ((*it)->AlignmentType) { - case Sketcher::ParabolaFocus: - focus = true; - break; - case Sketcher::ParabolaFocalAxis: - focus_to_vertex = true; - break; - default: - return -1; - } - } - } - - int currentgeoid = getHighestCurveIndex(); - int incrgeo = 0; - - const Part::GeomArcOfParabola* aop = static_cast(geo); - - Base::Vector3d center = aop->getCenter(); - Base::Vector3d focusp = aop->getFocus(); - - std::vector igeo; - std::vector icon; - - if (!focus) { - Part::GeomPoint* pf1 = new Part::GeomPoint(); - pf1->setPoint(focusp); - - igeo.push_back(pf1); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = Sketcher::ParabolaFocus; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->FirstPos = Sketcher::PointPos::start; - newConstr->Second = GeoId; - - icon.push_back(newConstr); - incrgeo++; - } - - if (!focus_to_vertex) { - Part::GeomLineSegment* paxis = new Part::GeomLineSegment(); - paxis->setPoints(center, focusp); - - igeo.push_back(paxis); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = Sketcher::ParabolaFocalAxis; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->FirstPos = Sketcher::PointPos::none; - newConstr->Second = GeoId; - - icon.push_back(newConstr); - - incrgeo++; - } - - this->addGeometry(igeo, true); - this->addConstraints(icon); - - for (std::vector::iterator it = igeo.begin(); it != igeo.end(); ++it) { - if (*it) - delete *it; - } - - for (std::vector::iterator it = icon.begin(); it != icon.end(); ++it) { - if (*it) - delete *it; - } - - icon.clear(); - igeo.clear(); - - return incrgeo;// number of added elements + return exposeInternalGeometryForType(GeoId); } else if (geo->is()) { - - const Part::GeomBSplineCurve* bsp = static_cast(geo); - // First we search what has to be restored - std::vector controlpoints(bsp->countPoles()); - std::vector controlpointgeoids(bsp->countPoles()); - - std::vector knotpoints(bsp->countKnots()); - std::vector knotgeoids(bsp->countKnots()); - - bool isfirstweightconstrained = false; - - std::vector::iterator itb; - std::vector::iterator it; - - for (it = controlpointgeoids.begin(), itb = controlpoints.begin(); - it != controlpointgeoids.end() && itb != controlpoints.end(); - ++it, ++itb) { - (*it) = -1; - (*itb) = false; - } - - for (it = knotgeoids.begin(), itb = knotpoints.begin(); - it != knotgeoids.end() && itb != knotpoints.end(); - ++it, ++itb) { - (*it) = -1; - (*itb) = false; - } - - const std::vector& vals = Constraints.getValues(); - - // search for existing poles - for (std::vector::const_iterator it = vals.begin(); it != vals.end(); - ++it) { - if ((*it)->Type == Sketcher::InternalAlignment && (*it)->Second == GeoId) { - switch ((*it)->AlignmentType) { - case Sketcher::BSplineControlPoint: - controlpoints[(*it)->InternalAlignmentIndex] = true; - controlpointgeoids[(*it)->InternalAlignmentIndex] = (*it)->First; - break; - case Sketcher::BSplineKnotPoint: - knotpoints[(*it)->InternalAlignmentIndex] = true; - knotgeoids[(*it)->InternalAlignmentIndex] = (*it)->First; - break; - default: - return -1; - } - } - } - - if (controlpoints[0]) { - // search for first pole weight constraint - for (std::vector::const_iterator it = vals.begin(); - it != vals.end(); - ++it) { - if ((*it)->Type == Sketcher::Weight && (*it)->First == controlpointgeoids[0]) { - isfirstweightconstrained = true; - } - } - } - - int currentgeoid = getHighestCurveIndex(); - int incrgeo = 0; - - std::vector igeo; - std::vector icon; - - std::vector poles = bsp->getPoles(); - std::vector weights = bsp->getWeights(); - std::vector knots = bsp->getKnots(); - - double distance_p0_p1 = (poles[1] - poles[0]).Length();// for visual purposes only - - int index = 0; - - for (it = controlpointgeoids.begin(), itb = controlpoints.begin(); - it != controlpointgeoids.end() && itb != controlpoints.end(); - ++it, ++itb, index++) { - - if (!(*itb))// if controlpoint not existing - { - Part::GeomCircle* pc = new Part::GeomCircle(); - pc->setCenter(poles[index]); - pc->setRadius(distance_p0_p1 / 6); - - igeo.push_back(pc); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = Sketcher::BSplineControlPoint; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->FirstPos = Sketcher::PointPos::mid; - newConstr->Second = GeoId; - newConstr->InternalAlignmentIndex = index; - - icon.push_back(newConstr); - - if (it != controlpointgeoids.begin()) { - if (isfirstweightconstrained && weights[0] == weights[index]) { - // if pole-weight newly created AND first weight is radius-constrained, - // AND these weights are equal, constrain them to be equal - Sketcher::Constraint* newConstr2 = new Sketcher::Constraint(); - newConstr2->Type = Sketcher::Equal; - newConstr2->First = currentgeoid + incrgeo + 1; - newConstr2->FirstPos = Sketcher::PointPos::none; - newConstr2->Second = controlpointgeoids[0]; - newConstr2->SecondPos = Sketcher::PointPos::none; - - icon.push_back(newConstr2); - } - } - else { - controlpointgeoids[0] = currentgeoid + incrgeo + 1; - if (weights[0] == 1.0) { - // if the first weight is 1.0 it's probably going to be non-rational - Sketcher::Constraint* newConstr3 = new Sketcher::Constraint(); - newConstr3->Type = Sketcher::Weight; - newConstr3->First = controlpointgeoids[0]; - newConstr3->setValue(weights[0]); - - icon.push_back(newConstr3); - - isfirstweightconstrained = true; - } - } - incrgeo++; - } - } - - index = 0; - - for (it = knotgeoids.begin(), itb = knotpoints.begin(); - it != knotgeoids.end() && itb != knotpoints.end(); - ++it, ++itb, index++) { - - if (!(*itb))// if knot point not existing - { - Part::GeomPoint* kp = new Part::GeomPoint(); - - kp->setPoint(bsp->pointAtParameter(knots[index])); - - igeo.push_back(kp); - - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = Sketcher::BSplineKnotPoint; - newConstr->First = currentgeoid + incrgeo + 1; - newConstr->FirstPos = Sketcher::PointPos::start; - newConstr->Second = GeoId; - newConstr->InternalAlignmentIndex = index; - - icon.push_back(newConstr); - - incrgeo++; - } - } - - Q_UNUSED(isfirstweightconstrained); - - this->addGeometry(igeo, true); - this->addConstraints(icon); - - for (std::vector::iterator it = igeo.begin(); it != igeo.end(); ++it) - if (*it) - delete *it; - - for (std::vector::iterator it = icon.begin(); it != icon.end(); ++it) - if (*it) - delete *it; - - icon.clear(); - igeo.clear(); - - return incrgeo;// number of added elements + return exposeInternalGeometryForType(GeoId); } else return -1;// not supported type @@ -6639,289 +6763,260 @@ int SketchObject::deleteUnusedInternalGeometry(int GeoId, bool delgeoid) if (geo->is() || geo->is() || geo->is()) { - - int majorelementindex = -1; - int minorelementindex = -1; - int focus1elementindex = -1; - int focus2elementindex = -1; - - const std::vector& vals = Constraints.getValues(); - - for (std::vector::const_iterator it = vals.begin(); it != vals.end(); - ++it) { - if ((*it)->Type == Sketcher::InternalAlignment && (*it)->Second == GeoId) { - switch ((*it)->AlignmentType) { - case Sketcher::EllipseMajorDiameter: - case Sketcher::HyperbolaMajor: - majorelementindex = (*it)->First; - break; - case Sketcher::EllipseMinorDiameter: - case Sketcher::HyperbolaMinor: - minorelementindex = (*it)->First; - break; - case Sketcher::EllipseFocus1: - case Sketcher::HyperbolaFocus: - focus1elementindex = (*it)->First; - break; - case Sketcher::EllipseFocus2: - focus2elementindex = (*it)->First; - break; - default: - return -1; - } - } - } - - // Hide unused geometry here - int majorconstraints = 0;// number of constraints associated to the geoid of the major axis - int minorconstraints = 0; - int focus1constraints = 0; - int focus2constraints = 0; - - for (std::vector::const_iterator it = vals.begin(); it != vals.end(); - ++it) { - - if ((*it)->Second == majorelementindex || (*it)->First == majorelementindex - || (*it)->Third == majorelementindex) - majorconstraints++; - else if ((*it)->Second == minorelementindex || (*it)->First == minorelementindex - || (*it)->Third == minorelementindex) - minorconstraints++; - else if ((*it)->Second == focus1elementindex || (*it)->First == focus1elementindex - || (*it)->Third == focus1elementindex) - focus1constraints++; - else if ((*it)->Second == focus2elementindex || (*it)->First == focus2elementindex - || (*it)->Third == focus2elementindex) - focus2constraints++; - } - - std::vector delgeometries; - - // those with less than 2 constraints must be removed - if (focus2constraints < 2) - delgeometries.push_back(focus2elementindex); - - if (focus1constraints < 2) - delgeometries.push_back(focus1elementindex); - - if (minorconstraints < 2) - delgeometries.push_back(minorelementindex); - - if (majorconstraints < 2) - delgeometries.push_back(majorelementindex); - - if (delgeoid) - delgeometries.push_back(GeoId); - - // indices over an erased element get automatically updated!! - std::sort(delgeometries.begin(), delgeometries.end()); - - if (!delgeometries.empty()) { - for (std::vector::reverse_iterator it = delgeometries.rbegin(); - it != delgeometries.rend(); - ++it) { - delGeometry(*it, false); - } - } - - int ndeleted = delgeometries.size(); - - delgeometries.clear(); - - return ndeleted;// number of deleted elements + return deleteUnusedInternalGeometryWhenTwoFoci(GeoId, delgeoid); } - else if (geo->is()) { - // if the focus-to-vertex line is constrained, then never delete the focus - // if the line is unconstrained, then the line may be deleted, - // in this case the focus may be deleted if unconstrained. - int majorelementindex = -1; - int focus1elementindex = -1; - const std::vector& vals = Constraints.getValues(); - - for (std::vector::const_iterator it = vals.begin(); it != vals.end(); - ++it) { - if ((*it)->Type == Sketcher::InternalAlignment && (*it)->Second == GeoId) { - switch ((*it)->AlignmentType) { - case Sketcher::ParabolaFocus: - focus1elementindex = (*it)->First; - break; - case Sketcher::ParabolaFocalAxis: - majorelementindex = (*it)->First; - break; - default: - return -1; - } - } - } - - // Hide unused geometry here - // number of constraints associated to the geoid of the major axis other than the coincident - // ones - int majorconstraints = 0; - int focus1constraints = 0; - - for (std::vector::const_iterator it = vals.begin(); it != vals.end(); - ++it) { - if ((*it)->Second == majorelementindex || (*it)->First == majorelementindex - || (*it)->Third == majorelementindex) - majorconstraints++; - else if ((*it)->Second == focus1elementindex || (*it)->First == focus1elementindex - || (*it)->Third == focus1elementindex) - focus1constraints++; - } - - std::vector delgeometries; - - // major has minimum one constraint, the specific internal alignment constraint - if (majorelementindex != -1 && majorconstraints < 2) - delgeometries.push_back(majorelementindex); - - // focus has minimum one constraint now, the specific internal alignment constraint - if (focus1elementindex != -1 && focus1constraints < 2) - delgeometries.push_back(focus1elementindex); - - if (delgeoid) - delgeometries.push_back(GeoId); - - // indices over an erased element get automatically updated!! - std::sort(delgeometries.begin(), delgeometries.end()); - - if (!delgeometries.empty()) { - for (std::vector::reverse_iterator it = delgeometries.rbegin(); - it != delgeometries.rend(); - ++it) { - delGeometry(*it, false); - } - } - - int ndeleted = delgeometries.size(); - - delgeometries.clear(); - - return ndeleted;// number of deleted elements + if (geo->is()) { + return deleteUnusedInternalGeometryWhenOneFocus(GeoId, delgeoid); } - else if (geo->is()) { - const Part::GeomBSplineCurve* bsp = static_cast(geo); - - // First we search existing IA - std::vector controlpointgeoids(bsp->countPoles()); - std::vector cpassociatedconstraints(bsp->countPoles()); - - std::vector knotgeoids(bsp->countKnots()); - std::vector kassociatedconstraints(bsp->countKnots()); - - std::vector::iterator it; - std::vector::iterator ita; - - for (it = controlpointgeoids.begin(), ita = cpassociatedconstraints.begin(); - it != controlpointgeoids.end() && ita != cpassociatedconstraints.end(); - ++it, ++ita) { - (*it) = -1; - (*ita) = 0; - } - - for (it = knotgeoids.begin(), ita = kassociatedconstraints.begin(); - it != knotgeoids.end() && ita != kassociatedconstraints.end(); - ++it, ++ita) { - (*it) = -1; - (*ita) = 0; - } - - const std::vector& vals = Constraints.getValues(); - - // search for existing poles - for (std::vector::const_iterator jt = vals.begin(); jt != vals.end(); - ++jt) { - if ((*jt)->Type == Sketcher::InternalAlignment && (*jt)->Second == GeoId) { - switch ((*jt)->AlignmentType) { - case Sketcher::BSplineControlPoint: - controlpointgeoids[(*jt)->InternalAlignmentIndex] = (*jt)->First; - break; - case Sketcher::BSplineKnotPoint: - knotgeoids[(*jt)->InternalAlignmentIndex] = (*jt)->First; - break; - default: - return -1; - } - } - } - - std::vector delgeometries; - - for (it = controlpointgeoids.begin(), ita = cpassociatedconstraints.begin(); - it != controlpointgeoids.end() && ita != cpassociatedconstraints.end(); - ++it, ++ita) { - if ((*it) != -1) { - // look for a circle at geoid index - for (std::vector::const_iterator itc = vals.begin(); - itc != vals.end(); - ++itc) { - - if ((*itc)->Type == Sketcher::Equal) { - bool f = false, s = false; - for (std::vector::iterator its = controlpointgeoids.begin(); - its != controlpointgeoids.end(); - ++its) { - if ((*itc)->First == *its) { - f = true; - } - else if ((*itc)->Second == *its) { - s = true; - } - - if (f && s) {// the equality constraint is not interpole - break; - } - } - - // the equality constraint constraints a pole but it is not interpole - if (f != s) { - (*ita)++; - } - } - // We do not ignore weight constraints as we did with radius constraints, - // because the radius magnitude no longer makes sense without the B-Spline. - } - - if ((*ita) < 2) {// IA - delgeometries.push_back((*it)); - } - } - } - - for (it = knotgeoids.begin(), ita = kassociatedconstraints.begin(); - it != knotgeoids.end() && ita != kassociatedconstraints.end(); - ++it, ++ita) { - if ((*it) != -1) { - // look for a point at geoid index - for (std::vector::const_iterator itc = vals.begin(); - itc != vals.end(); - ++itc) { - if ((*itc)->Second == (*it) || (*itc)->First == (*it) - || (*itc)->Third == (*it)) { - (*ita)++; - } - } - - if ((*ita) < 2) {// IA - delgeometries.push_back((*it)); - } - } - } - - - if (delgeoid) - delgeometries.push_back(GeoId); - - int ndeleted = delGeometriesExclusiveList(delgeometries); - - return ndeleted;// number of deleted elements + if (geo->is()) { + return deleteUnusedInternalGeometryWhenBSpline(GeoId, delgeoid); } - else { - return -1;// not supported type + + // Default case: type not supported + return -1; +} + +int SketchObject::deleteUnusedInternalGeometryWhenTwoFoci(int GeoId, bool delgeoid) +{ + int majorelementindex = -1; + int minorelementindex = -1; + int focus1elementindex = -1; + int focus2elementindex = -1; + + const std::vector& vals = Constraints.getValues(); + + for (auto const& constr : vals) { + if (constr->Type != Sketcher::InternalAlignment || constr->Second != GeoId) { + continue; + } + + switch (constr->AlignmentType) { + case Sketcher::EllipseMajorDiameter: + case Sketcher::HyperbolaMajor: + majorelementindex = constr->First; + break; + case Sketcher::EllipseMinorDiameter: + case Sketcher::HyperbolaMinor: + minorelementindex = constr->First; + break; + case Sketcher::EllipseFocus1: + case Sketcher::HyperbolaFocus: + focus1elementindex = constr->First; + break; + case Sketcher::EllipseFocus2: + focus2elementindex = constr->First; + break; + default: + return -1; + } } + + // Hide unused geometry here + int majorconstraints = 0;// number of constraints associated to the geoid of the major axis + int minorconstraints = 0; + int focus1constraints = 0; + int focus2constraints = 0; + + for (const auto& constr : vals) { + if (constr->involvesGeoId(majorelementindex)) + majorconstraints++; + else if (constr->involvesGeoId(minorelementindex)) + minorconstraints++; + else if (constr->involvesGeoId(focus1elementindex)) + focus1constraints++; + else if (constr->involvesGeoId(focus2elementindex)) + focus2constraints++; + } + + std::vector delgeometries; + + // those with less than 2 constraints must be removed + if (focus2constraints < 2) + delgeometries.push_back(focus2elementindex); + + if (focus1constraints < 2) + delgeometries.push_back(focus1elementindex); + + if (minorconstraints < 2) + delgeometries.push_back(minorelementindex); + + if (majorconstraints < 2) + delgeometries.push_back(majorelementindex); + + if (delgeoid) + delgeometries.push_back(GeoId); + + // indices over an erased element get automatically updated!! + std::sort(delgeometries.begin(), delgeometries.end(), std::greater<>()); + + for (auto& dGeoId : delgeometries) { + delGeometry(dGeoId, false); + } + + int ndeleted = delgeometries.size(); + + return ndeleted;// number of deleted elements +} + +int SketchObject::deleteUnusedInternalGeometryWhenOneFocus(int GeoId, bool delgeoid) +{ + // if the focus-to-vertex line is constrained, then never delete the focus + // if the line is unconstrained, then the line may be deleted, + // in this case the focus may be deleted if unconstrained. + int majorelementindex = -1; + int focus1elementindex = -1; + + const std::vector& vals = Constraints.getValues(); + + for (auto const& constr : vals) { + if (constr->Type != Sketcher::InternalAlignment || constr->Second != GeoId) { + continue; + } + + switch (constr->AlignmentType) { + case Sketcher::ParabolaFocus: + focus1elementindex = constr->First; + break; + case Sketcher::ParabolaFocalAxis: + majorelementindex = constr->First; + break; + default: + return -1; + } + } + + // Hide unused geometry here + // number of constraints associated to the geoid of the major axis other than the coincident + // ones + int majorconstraints = 0; + int focus1constraints = 0; + + for (const auto& constr : vals) { + if (constr->involvesGeoId(majorelementindex)) { + majorconstraints++; + } + else if (constr->involvesGeoId(focus1elementindex)) { + focus1constraints++; + } + } + + std::vector delgeometries; + + // major has minimum one constraint, the specific internal alignment constraint + if (majorelementindex != -1 && majorconstraints < 2) + delgeometries.push_back(majorelementindex); + + // focus has minimum one constraint now, the specific internal alignment constraint + if (focus1elementindex != -1 && focus1constraints < 2) + delgeometries.push_back(focus1elementindex); + + if (delgeoid) + delgeometries.push_back(GeoId); + + // indices over an erased element get automatically updated!! + std::sort(delgeometries.begin(), delgeometries.end(), std::greater<>()); + + for (auto& dGeoId : delgeometries) { + delGeometry(dGeoId, false); + } + + int ndeleted = delgeometries.size(); + + delgeometries.clear(); + + return ndeleted;// number of deleted elements +} + +int SketchObject::deleteUnusedInternalGeometryWhenBSpline(int GeoId, bool delgeoid) +{ + const Part::GeomBSplineCurve* bsp = static_cast(getGeometry(GeoId)); + + // First we search existing IA + std::vector > poleGeoIdsAndConstraints(bsp->countPoles(), {GeoEnum::GeoUndef, 0}); + + std::vector > knotGeoIdsAndConstraints(bsp->countKnots(), {GeoEnum::GeoUndef, 0}); + + const std::vector& vals = Constraints.getValues(); + + // search for existing poles + for (auto const& constr : vals) { + if (constr->Type != Sketcher::InternalAlignment || constr->Second != GeoId) { + continue; + } + + switch (constr->AlignmentType) { + case Sketcher::BSplineControlPoint: + poleGeoIdsAndConstraints[constr->InternalAlignmentIndex].first = constr->First; + break; + case Sketcher::BSplineKnotPoint: + knotGeoIdsAndConstraints[constr->InternalAlignmentIndex].first = constr->First; + break; + default: + return -1; + } + } + + std::vector delgeometries; + + for (auto& [cpGeoId, numConstr] : poleGeoIdsAndConstraints) { + if (cpGeoId == GeoEnum::GeoUndef) { + continue; + } + + // look for a circle at geoid index + for (auto const& constr : vals) { + if (constr->Type != Sketcher::Equal) { + continue; + } + + bool firstIsInCPGeoIds = std::find_if(poleGeoIdsAndConstraints.begin(), + poleGeoIdsAndConstraints.end(), + [&constr](const auto& _pair) { + return _pair.first == constr->First; + }) != poleGeoIdsAndConstraints.end(); + bool secondIsInCPGeoIds = std::find_if(poleGeoIdsAndConstraints.begin(), + poleGeoIdsAndConstraints.end(), + [&constr](const auto& _pair){ + return _pair.first == constr->Second; + }) != poleGeoIdsAndConstraints.end(); + + // the equality constraint constrains a pole but it is not interpole + if (firstIsInCPGeoIds != secondIsInCPGeoIds) { + numConstr++; + } + // We do not ignore weight constraints as we did with radius constraints, + // because the radius magnitude no longer makes sense without the B-Spline. + } + + if (numConstr < 2) { // IA + delgeometries.push_back(cpGeoId); + } + } + + for (auto& [kGeoId, numConstr] : knotGeoIdsAndConstraints) { + if (kGeoId == GeoEnum::GeoUndef) { + continue; + } + + // look for a point at geoid index + numConstr = std::count_if(vals.begin(), vals.end(), [&kGeoId](const auto& constr) { + return constr->involvesGeoId(kGeoId); + }); + + if (numConstr < 2) { // IA + delgeometries.push_back(kGeoId); + } + } + + if (delgeoid) { + delgeometries.push_back(GeoId); + } + + int ndeleted = delGeometriesExclusiveList(delgeometries); + + return ndeleted;// number of deleted elements } int SketchObject::deleteUnusedInternalGeometryAndUpdateGeoId(int& GeoId, bool delgeoid) diff --git a/src/Mod/Sketcher/App/SketchObject.h b/src/Mod/Sketcher/App/SketchObject.h index 62a13821dd..09aca4d269 100644 --- a/src/Mod/Sketcher/App/SketchObject.h +++ b/src/Mod/Sketcher/App/SketchObject.h @@ -482,6 +482,11 @@ public: * \return -1 on error */ int exposeInternalGeometry(int GeoId); + template + int exposeInternalGeometryForType([[maybe_unused]] const int GeoId) + { + return -1; // By default internal geometry is not supported + } /*! \brief Deletes all unused (not further constrained) internal geometry \param GeoId - the geometry having the internal geometry to delete @@ -899,6 +904,15 @@ protected: void buildShape(); /// get called by the container when a property has changed void onChanged(const App::Property* /*prop*/) override; + + /// Helper functions for `deleteUnusedInternalGeometry` by cases + /// two foci for ellipses and arcs of ellipses and hyperbolas + int deleteUnusedInternalGeometryWhenTwoFoci(int GeoId, bool delgeoid = false); + /// one focus for parabolas + int deleteUnusedInternalGeometryWhenOneFocus(int GeoId, bool delgeoid = false); + /// b-splines need their own treatment + int deleteUnusedInternalGeometryWhenBSpline(int GeoId, bool delgeoid = false); + void onDocumentRestored() override; void restoreFinished() override; From 0ded3a4af1c91f3d76b845f832a75d5781c84535 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Fri, 23 Aug 2024 05:52:43 +0530 Subject: [PATCH 12/26] [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; From 8c53b2ded507fb34bc9e08f6d18422c00455061a Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 23 Dec 2024 12:31:37 +0530 Subject: [PATCH 13/26] [Sketcher][test] Add tests for B-spline operations --- tests/src/Mod/Sketcher/App/SketchObject.cpp | 319 ++++++++++++++++++++ 1 file changed, 319 insertions(+) diff --git a/tests/src/Mod/Sketcher/App/SketchObject.cpp b/tests/src/Mod/Sketcher/App/SketchObject.cpp index d05266ae26..5a3f910892 100644 --- a/tests/src/Mod/Sketcher/App/SketchObject.cpp +++ b/tests/src/Mod/Sketcher/App/SketchObject.cpp @@ -1413,6 +1413,325 @@ TEST_F(SketchObjectTest, testTrimNonPeriodicBSplineMid) // TODO: Ensure shape is preserved } +TEST_F(SketchObjectTest, testModifyKnotMultInNonPeriodicBSplineToZero) +{ + // Arrange + auto nonPeriodicBSpline = createTypicalNonPeriodicBSpline(); + assert(nonPeriodicBSpline); + int geoId = getObject()->addGeometry(nonPeriodicBSpline.get()); + + // Act + // Try decreasing mult to zero. + // NOTE: we still use OCCT notation of knot index starting with 1 (not 0). + getObject()->modifyBSplineKnotMultiplicity(geoId, 2, -1); + // Assert + // Knot should disappear. We start with 3 (unique) knots, so expect 2. + auto bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), 2); +} + +TEST_F(SketchObjectTest, testModifyKnotMultInNonPeriodicBSplineToDisallowed) +{ + // Arrange + auto nonPeriodicBSpline = createTypicalNonPeriodicBSpline(); + assert(nonPeriodicBSpline); + int geoId = getObject()->addGeometry(nonPeriodicBSpline.get()); + + // Act and Assert + // TODO: Try modifying such that resultant multiplicity > degree + // TODO: This should immediately throw exception + EXPECT_THROW(getObject()->modifyBSplineKnotMultiplicity(geoId, 2, 3), Base::ValueError); + // TODO: Try modifying such that resultant multiplicity < 0 + // TODO: This should immediately throw exception + EXPECT_THROW(getObject()->modifyBSplineKnotMultiplicity(geoId, 2, -2), Base::ValueError); +} + +TEST_F(SketchObjectTest, testModifyKnotMultInNonPeriodicBSpline) +{ + // Arrange + auto nonPeriodicBSpline = createTypicalNonPeriodicBSpline(); + assert(nonPeriodicBSpline); + int geoId = getObject()->addGeometry(nonPeriodicBSpline.get()); + + auto bsp = static_cast(getObject()->getGeometry(geoId)); + int oldKnotsNum = bsp->countKnots(); + int oldMultiplicityOfTargetKnot = bsp->getMultiplicities()[1]; + + // Act + // TODO: Increase/decrease knot multiplicity normally + getObject()->modifyBSplineKnotMultiplicity(geoId, 2, 1); + // Assert + // This should not alter the sizes of knot and multiplicity vectors. + bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), oldKnotsNum); + // This should increment the multiplicity. + EXPECT_EQ(bsp->getMultiplicities()[1], oldMultiplicityOfTargetKnot + 1); + // This should still be a non-periodic spline + EXPECT_FALSE(bsp->isPeriodic()); + // TODO: Expect shape is preserved + + // Act + // TODO: Increase/decrease knot multiplicity normally + getObject()->modifyBSplineKnotMultiplicity(geoId, 2, -1); + // Assert + // This should not alter the sizes of knot and multiplicity vectors. + bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), oldKnotsNum); + // This should increment the multiplicity. + EXPECT_EQ(bsp->getMultiplicities()[1], oldMultiplicityOfTargetKnot); + // This should still be a non-periodic spline + EXPECT_FALSE(bsp->isPeriodic()); +} + +TEST_F(SketchObjectTest, testModifyKnotMultInPeriodicBSplineToZero) +{ + // Arrange + auto PeriodicBSpline = createTypicalPeriodicBSpline(); + assert(PeriodicBSpline); + int geoId = getObject()->addGeometry(PeriodicBSpline.get()); + + // Act + // Try decreasing mult to zero. + // NOTE: we still use OCCT notation of knot index starting with 1 (not 0). + getObject()->modifyBSplineKnotMultiplicity(geoId, 2, -1); + // Assert + // Knot should disappear. We start with 3 (unique) knots, so expect 2. + auto bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), 5); +} + +TEST_F(SketchObjectTest, testModifyKnotMultInPeriodicBSplineToDisallowed) +{ + // Arrange + auto PeriodicBSpline = createTypicalPeriodicBSpline(); + assert(PeriodicBSpline); + int geoId = getObject()->addGeometry(PeriodicBSpline.get()); + + // Act and Assert + // TODO: Try modifying such that resultant multiplicity > degree + // TODO: This should immediately throw exception + EXPECT_THROW(getObject()->modifyBSplineKnotMultiplicity(geoId, 2, 3), Base::ValueError); + // TODO: Try modifying such that resultant multiplicity < 0 + // TODO: This should immediately throw exception + EXPECT_THROW(getObject()->modifyBSplineKnotMultiplicity(geoId, 2, -2), Base::ValueError); +} + +TEST_F(SketchObjectTest, testModifyKnotMultInPeriodicBSpline) +{ + // Arrange + auto PeriodicBSpline = createTypicalPeriodicBSpline(); + assert(PeriodicBSpline); + int geoId = getObject()->addGeometry(PeriodicBSpline.get()); + + auto bsp = static_cast(getObject()->getGeometry(geoId)); + int oldKnotsNum = bsp->countKnots(); + int oldMultiplicityOfTargetKnot = bsp->getMultiplicities()[1]; + + // Act + // TODO: Increase/decrease knot multiplicity normally + getObject()->modifyBSplineKnotMultiplicity(geoId, 2, 1); + // Assert + // This should not alter the sizes of knot and multiplicity vectors. + bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), oldKnotsNum); + // This should increment the multiplicity. + EXPECT_EQ(bsp->getMultiplicities()[1], oldMultiplicityOfTargetKnot + 1); + // This should still be a periodic spline + EXPECT_TRUE(bsp->isPeriodic()); + // TODO: Expect shape is preserved + + // Act + // TODO: Increase/decrease knot multiplicity normally + getObject()->modifyBSplineKnotMultiplicity(geoId, 2, -1); + // Assert + // This should not alter the sizes of knot and multiplicity vectors. + bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), oldKnotsNum); + // This should decrement the multiplicity. + EXPECT_EQ(bsp->getMultiplicities()[1], oldMultiplicityOfTargetKnot); + // This should still be a non-periodic spline + EXPECT_TRUE(bsp->isPeriodic()); +} + +TEST_F(SketchObjectTest, testInsertKnotInNonPeriodicBSpline) +{ + // Arrange + auto nonPeriodicBSpline = createTypicalNonPeriodicBSpline(); + assert(nonPeriodicBSpline); + int geoId = getObject()->addGeometry(nonPeriodicBSpline.get()); + + // Act and Assert + // Try inserting knot with zero multiplicity + // zero multiplicity knot should immediately throw exception + EXPECT_THROW(getObject()->insertBSplineKnot(geoId, 0.5, 0), Base::ValueError); + + // Act and Assert + // Try inserting knot with multiplicity > degree + // This should immediately throw exception + EXPECT_THROW(getObject()->insertBSplineKnot(geoId, 0.5, 4), Base::ValueError); + + // Act and Assert + // TODO: Try inserting at an existing knot with resultant multiplicity > degree + // TODO: This should immediately throw exception + // FIXME: Not happening. May be ignoring existing values. + // EXPECT_THROW(getObject()->insertBSplineKnot(geoId, 1.0, 3), Base::ValueError); + + auto bsp = static_cast(getObject()->getGeometry(geoId)); + int oldKnotsNum = bsp->countKnots(); + int oldMultiplicityOfTargetKnot = bsp->getMultiplicities()[1]; + + // Act + // Add at a general position (where no knot exists) + getObject()->insertBSplineKnot(geoId, 0.5, 1); + // Assert + // This should add to both the knot and multiplicity "vectors" + bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), oldKnotsNum + 1); + // This should still be a non-periodic spline + EXPECT_FALSE(bsp->isPeriodic()); + + // Act + // Add a knot at an existing knot + getObject()->insertBSplineKnot(geoId, 1.0, 1); + // Assert + // This should not alter the sizes of knot and multiplicity vectors. + // (Since we previously added a knot, this means the total is still one more than original) + bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), oldKnotsNum + 1); + // This should increment the multiplicity. + EXPECT_EQ(bsp->getMultiplicities()[2], oldMultiplicityOfTargetKnot + 1); + // This should still be a non-periodic spline + EXPECT_FALSE(bsp->isPeriodic()); +} + +TEST_F(SketchObjectTest, testInsertKnotInPeriodicBSpline) +{ + // This should also cover as a representative of arc of conic + + // Arrange + auto PeriodicBSpline = createTypicalPeriodicBSpline(); + assert(PeriodicBSpline); + int geoId = getObject()->addGeometry(PeriodicBSpline.get()); + + // Act and Assert + // Try inserting knot with zero multiplicity + // zero multiplicity knot should immediately throw exception + EXPECT_THROW(getObject()->insertBSplineKnot(geoId, 0.5, 0), Base::ValueError); + + // Act and Assert + // Try inserting knot with multiplicity > degree + // This should immediately throw exception + EXPECT_THROW(getObject()->insertBSplineKnot(geoId, 0.5, 4), Base::ValueError); + + // Act and Assert + // TODO: Try inserting at an existing knot with resultant multiplicity > degree + // TODO: This should immediately throw exception + + auto bsp = static_cast(getObject()->getGeometry(geoId)); + int oldKnotsNum = bsp->countKnots(); + int oldMultiplicityOfTargetKnot = bsp->getMultiplicities()[2]; + + // Act + // Add at a general position (where no knot exists) + getObject()->insertBSplineKnot(geoId, 0.5, 1); + // Assert + // This should add to both the knot and multiplicity "vectors" + bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), oldKnotsNum + 1); + // This should still be a periodic spline + EXPECT_TRUE(bsp->isPeriodic()); + + // Act + // Add a knot at an existing knot + getObject()->insertBSplineKnot(geoId, 1.0, 1); + // Assert + // This should not alter the sizes of knot and multiplicity vectors. + bsp = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp->countKnots(), oldKnotsNum + 1); + // This should increment the multiplicity. + EXPECT_EQ(bsp->getMultiplicities()[3], oldMultiplicityOfTargetKnot + 1); + // This should still be a periodic spline + EXPECT_TRUE(bsp->isPeriodic()); +} + +TEST_F(SketchObjectTest, testJoinCurves) +{ + // Arrange + // Make two curves + Base::Vector3d coordsCenter(0.0, 0.0, 0.0); + double radius = 3.0, startParam = M_PI / 2, endParam = M_PI; + Part::GeomArcOfCircle arcOfCircle; + arcOfCircle.setCenter(coordsCenter); + arcOfCircle.setRadius(radius); + arcOfCircle.setRange(startParam, endParam, true); + int geoId1 = getObject()->addGeometry(&arcOfCircle); + + Base::Vector3d coords1(0.1, 0.0, 0.0); + Base::Vector3d coords2(3.0, 4.0, 0.0); + Part::GeomLineSegment lineSeg; + lineSeg.setPoints(coords1, coords2); + int geoId2 = getObject()->addGeometry(&lineSeg); + + // Act + // Join these curves + getObject()->join(geoId1, Sketcher::PointPos::start, geoId2, Sketcher::PointPos::start); + + // Assert + // Check they are replaced (here it means there is only one curve left after internal + // geometries are removed) + for (int iterGeoId = 0; iterGeoId < getObject()->getHighestCurveIndex(); ++iterGeoId) { + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(iterGeoId); + } + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); +} + +TEST_F(SketchObjectTest, testJoinCurvesWhenTangent) +{ + // Arrange + // Make two curves + Base::Vector3d coordsCenter(0.0, 0.0, 0.0); + double radius = 3.0, startParam = M_PI / 2, endParam = M_PI; + Part::GeomArcOfCircle arcOfCircle; + arcOfCircle.setCenter(coordsCenter); + arcOfCircle.setRadius(radius); + arcOfCircle.setRange(startParam, endParam, true); + int geoId1 = getObject()->addGeometry(&arcOfCircle); + + Base::Vector3d coords1(0.0, 0.0, 0.0); + Base::Vector3d coords2(3.0, 0.0, 0.0); + Part::GeomLineSegment lineSeg; + lineSeg.setPoints(coords1, coords2); + int geoId2 = getObject()->addGeometry(&lineSeg); + + // Add end-to-end tangent between these + auto constraint = new Sketcher::Constraint(); // Ownership will be transferred to the sketch + constraint->Type = Sketcher::ConstraintType::Tangent; + constraint->First = geoId1; + constraint->FirstPos = Sketcher::PointPos::start; + constraint->Second = geoId2; + constraint->SecondPos = Sketcher::PointPos::start; + getObject()->addConstraint(constraint); + + // Act + // Join these curves + getObject()->join(geoId1, Sketcher::PointPos::start, geoId2, Sketcher::PointPos::start, 1); + + // Assert + // Check they are replaced (here it means there is only one curve left after internal + // geometries are removed) + for (int iterGeoId = 0; iterGeoId < getObject()->getHighestCurveIndex(); ++iterGeoId) { + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(iterGeoId); + } + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); + // TODO: Check the shape is conserved (how?) + // Check there is no C-0 knot (should be possible for the chosen example) + auto mults = static_cast(getObject()->getGeometry(0)) + ->getMultiplicities(); + EXPECT_TRUE(std::all_of(mults.begin(), mults.end(), [](auto mult) { + return mult >= 1; + })); +} + TEST_F(SketchObjectTest, testReverseAngleConstraintToSupplementaryExpressionNoUnits1) { std::string expr = Sketcher::SketchObject::reverseAngleConstraintExpression("180 - 60"); From 6ba1b0b7a92378b76f05e4d8c6798ce70c6380d4 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sat, 29 Jun 2024 13:50:57 +0530 Subject: [PATCH 14/26] [Sketcher] Refactor `SketchObject::insertBSplineKnot()` --- src/Mod/Sketcher/App/SketchObject.cpp | 123 +++++++++++--------------- 1 file changed, 51 insertions(+), 72 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index eb17c2ae6d..386166ef9e 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -7349,38 +7349,43 @@ bool SketchObject::insertBSplineKnot(int GeoId, double param, int multiplicity) Base::StateLocker lock(managedoperation, true); // handling unacceptable cases - if (GeoId < 0 || GeoId > getHighestCurveIndex()) + if (GeoId < 0 || GeoId > getHighestCurveIndex()) { THROWMT( Base::ValueError, QT_TRANSLATE_NOOP("Exceptions", "B-spline Geometry Index (GeoID) is out of bounds.")); + } - if (multiplicity == 0) + if (multiplicity == 0) { THROWMT(Base::ValueError, QT_TRANSLATE_NOOP("Exceptions", "Knot cannot have zero multiplicity.")); + } const Part::Geometry* geo = getGeometry(GeoId); - if (geo->getTypeId() != Part::GeomBSplineCurve::getClassTypeId()) + if (!geo->is()) { THROWMT(Base::TypeError, QT_TRANSLATE_NOOP("Exceptions", "The Geometry Index (GeoId) provided is not a B-spline.")); + } - const Part::GeomBSplineCurve* bsp = static_cast(geo); + const auto* bsp = static_cast(geo); int degree = bsp->getDegree(); double firstParam = bsp->getFirstParameter(); double lastParam = bsp->getLastParameter(); - if (multiplicity > degree) + if (multiplicity > degree) { THROWMT(Base::ValueError, QT_TRANSLATE_NOOP( "Exceptions", "Knot multiplicity cannot be higher than the degree of the B-spline.")); + } - if (param > lastParam || param < firstParam) + if (param > lastParam || param < firstParam) { THROWMT(Base::ValueError, QT_TRANSLATE_NOOP("Exceptions", "Knot cannot be inserted outside the B-spline parameter range.")); + } std::unique_ptr bspline; @@ -7400,89 +7405,64 @@ bool SketchObject::insertBSplineKnot(int GeoId, double param, int multiplicity) std::vector poles = bsp->getPoles(); std::vector newpoles = bspline->getPoles(); - std::vector prevpole(bsp->countPoles()); + std::vector poleIndexInNew(poles.size(), -1); - for (int i = 0; i < int(poles.size()); i++) - prevpole[i] = -1; - - int taken = 0; - for (int j = 0; j < int(poles.size()); j++) { - for (int i = taken; i < int(newpoles.size()); i++) { - if (newpoles[i] == poles[j]) { - prevpole[j] = i; - taken++; - break; - } - } + for (size_t j = 0; j < poles.size(); j++) { + const auto it = std::find(newpoles.begin(), newpoles.end(), poles[j]); + poleIndexInNew[j] = it - newpoles.begin(); } - // on fully removing a knot the knot geometry changes std::vector knots = bsp->getKnots(); std::vector newknots = bspline->getKnots(); - std::vector prevknot(bsp->countKnots()); + std::vector knotIndexInNew(knots.size(), -1); - for (int i = 0; i < int(knots.size()); i++) - prevknot[i] = -1; - - taken = 0; - for (int j = 0; j < int(knots.size()); j++) { - for (int i = taken; i < int(newknots.size()); i++) { - if (newknots[i] == knots[j]) { - prevknot[j] = i; - taken++; - break; - } - } + for (size_t j = 0; j < knots.size(); j++) { + const auto it = std::find(newknots.begin(), newknots.end(), knots[j]); + knotIndexInNew[j] = it - newknots.begin(); } const std::vector& cvals = Constraints.getValues(); std::vector newcVals(0); - // modify pole constraints - for (std::vector::const_iterator it = cvals.begin(); it != cvals.end(); - ++it) { - if ((*it)->Type == Sketcher::InternalAlignment && (*it)->Second == GeoId) { - if ((*it)->AlignmentType == Sketcher::BSplineControlPoint) { - if (prevpole[(*it)->InternalAlignmentIndex] != -1) { - assert(prevpole[(*it)->InternalAlignmentIndex] < bspline->countPoles()); - Constraint* newConstr = (*it)->clone(); - newConstr->InternalAlignmentIndex = prevpole[(*it)->InternalAlignmentIndex]; - newcVals.push_back(newConstr); - } - else { - // it is an internal alignment geometry that is no longer valid => delete it and - // the pole circle - delGeoId.push_back((*it)->First); - } - } - else if ((*it)->AlignmentType == Sketcher::BSplineKnotPoint) { - if (prevknot[(*it)->InternalAlignmentIndex] != -1) { - assert(prevknot[(*it)->InternalAlignmentIndex] < bspline->countKnots()); - Constraint* newConstr = (*it)->clone(); - newConstr->InternalAlignmentIndex = prevknot[(*it)->InternalAlignmentIndex]; - newcVals.push_back(newConstr); - } - else { - // it is an internal alignment geometry that is no longer valid => delete it and - // the knot point - delGeoId.push_back((*it)->First); - } - } - else { - // it is a bspline geometry, but not a controlpoint or knot - newcVals.push_back(*it); - } + // modify pole and knot constraints + for (const auto& constr : cvals) { + if (!(constr->Type == Sketcher::InternalAlignment && constr->Second == GeoId)) { + newcVals.push_back(constr); + continue; + } + + std::vector* indexInNew = nullptr; + + if (constr->AlignmentType == Sketcher::BSplineControlPoint) { + indexInNew = &poleIndexInNew; + } + else if (constr->AlignmentType == Sketcher::BSplineKnotPoint) { + indexInNew = &knotIndexInNew; } else { - newcVals.push_back(*it); + // it is a bspline geometry, but not a controlpoint or knot + newcVals.push_back(constr); + continue; } + + if (indexInNew && indexInNew->at(constr->InternalAlignmentIndex) == -1) { + // it is an internal alignment geometry that is no longer valid + // => delete it and the pole circle + delGeoId.push_back(constr->First); + continue; + } + + Constraint* newConstr = constr->clone(); + newConstr->InternalAlignmentIndex = indexInNew->at(constr->InternalAlignmentIndex); + newcVals.push_back(newConstr); } const std::vector& vals = getInternalGeometry(); std::vector newVals(vals); + GeometryFacade::copyId(geo, bspline.get()); newVals[GeoId] = bspline.release(); // Block acceptGeometry in OnChanged to avoid unnecessary checks and updates @@ -7506,12 +7486,11 @@ bool SketchObject::insertBSplineKnot(int GeoId, double param, int multiplicity) // See 247a9f0876a00e08c25b07d1f8802479d8623e87 for suggestions. // Geometry.touch(); delGeometriesExclusiveList(delGeoId); - } - else { - Geometry.touch(); + return true; } - // handle this last return + Geometry.touch(); + return true; } From 3832db01ff3c47466dd7df49ae6c630a2249aa19 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 23 Dec 2024 13:48:18 +0530 Subject: [PATCH 15/26] [Sketcher] Refactor `SketchObject::modifyBSplineKnotMultiplicity` --- src/Mod/Sketcher/App/SketchObject.cpp | 138 +++++++++++--------------- 1 file changed, 60 insertions(+), 78 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 386166ef9e..aaf81eb3fe 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -7166,63 +7166,70 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m if (GeoId < 0 || GeoId > getHighestCurveIndex()) { THROWMT( Base::ValueError, - QT_TRANSLATE_NOOP("Exceptions", "B-spline Geometry Index (GeoID) is out of bounds.")) + QT_TRANSLATE_NOOP("Exceptions", "B-spline Geometry Index (GeoID) is out of bounds.")); } - if (multiplicityincr == 0)// no change in multiplicity + if (multiplicityincr == 0) { + // no change in multiplicity THROWMT( Base::ValueError, - QT_TRANSLATE_NOOP("Exceptions", "You are requesting no change in knot multiplicity.")) + QT_TRANSLATE_NOOP("Exceptions", "You are requesting no change in knot multiplicity.")); + } const Part::Geometry* geo = getGeometry(GeoId); - if (geo->getTypeId() != Part::GeomBSplineCurve::getClassTypeId()) + if (geo->getTypeId() != Part::GeomBSplineCurve::getClassTypeId()) { THROWMT(Base::TypeError, QT_TRANSLATE_NOOP("Exceptions", - "The Geometry Index (GeoId) provided is not a B-spline.")) + "The Geometry Index (GeoId) provided is not a B-spline.")); + } const Part::GeomBSplineCurve* bsp = static_cast(geo); int degree = bsp->getDegree(); - if (knotIndex > bsp->countKnots() || knotIndex < 1)// knotindex in OCC 1 -> countKnots + if (knotIndex > bsp->countKnots() || knotIndex < 1) { + // knotindex in OCC 1 -> countKnots THROWMT(Base::ValueError, QT_TRANSLATE_NOOP("Exceptions", "The knot index is out of bounds. Note that in accordance with " - "OCC notation, the first knot has index 1 and not zero.")) + "OCC notation, the first knot has index 1 and not zero.")); + } std::unique_ptr bspline; int curmult = bsp->getMultiplicity(knotIndex); // zero is removing the knot, degree is just positional continuity - if ((curmult + multiplicityincr) > degree) + if ((curmult + multiplicityincr) > degree) { THROWMT(Base::ValueError, QT_TRANSLATE_NOOP( "Exceptions", - "The multiplicity cannot be increased beyond the degree of the B-spline.")) + "The multiplicity cannot be increased beyond the degree of the B-spline.")); + } // zero is removing the knot, degree is just positional continuity - if ((curmult + multiplicityincr) < 0) + if ((curmult + multiplicityincr) < 0) { THROWMT( Base::ValueError, - QT_TRANSLATE_NOOP("Exceptions", "The multiplicity cannot be decreased beyond zero.")) + QT_TRANSLATE_NOOP("Exceptions", "The multiplicity cannot be decreased beyond zero.")); + } try { bspline.reset(static_cast(bsp->clone())); - if (multiplicityincr > 0) {// increase multiplicity + if (multiplicityincr > 0) { // increase multiplicity bspline->increaseMultiplicity(knotIndex, curmult + multiplicityincr); } - else {// decrease multiplicity + else { // decrease multiplicity bool result = bspline->removeKnot(knotIndex, curmult + multiplicityincr, 1E6); - if (!result) - THROWMT( - Base::CADKernelError, - QT_TRANSLATE_NOOP( - "Exceptions", - "OCC is unable to decrease the multiplicity within the maximum tolerance.")) + if (!result) { + THROWMT(Base::CADKernelError, + QT_TRANSLATE_NOOP("Exceptions", + "OCC is unable to decrease the multiplicity within the " + "maximum tolerance.")); + } } } catch (const Base::Exception& e) { @@ -7232,87 +7239,62 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m // we succeeded with the multiplicity modification, so alignment geometry may be // invalid/inconsistent for the new bspline - std::vector delGeoId; std::vector poles = bsp->getPoles(); std::vector newpoles = bspline->getPoles(); - std::vector prevpole(bsp->countPoles()); + std::vector poleIndexInNew(bsp->countPoles(), -1); - for (int i = 0; i < int(poles.size()); i++) - prevpole[i] = -1; - - int taken = 0; for (int j = 0; j < int(poles.size()); j++) { - for (int i = taken; i < int(newpoles.size()); i++) { - if (newpoles[i] == poles[j]) { - prevpole[j] = i; - taken++; - break; - } - } + const auto it = std::find(newpoles.begin(), newpoles.end(), poles[j]); + poleIndexInNew[j] = it - newpoles.begin(); } // on fully removing a knot the knot geometry changes std::vector knots = bsp->getKnots(); std::vector newknots = bspline->getKnots(); - std::vector prevknot(bsp->countKnots()); + std::vector knotIndexInNew(bsp->countKnots(), -1); - for (int i = 0; i < int(knots.size()); i++) - prevknot[i] = -1; - - taken = 0; for (int j = 0; j < int(knots.size()); j++) { - for (int i = taken; i < int(newknots.size()); i++) { - if (newknots[i] == knots[j]) { - prevknot[j] = i; - taken++; - break; - } - } + const auto it = std::find(newknots.begin(), newknots.end(), knots[j]); + knotIndexInNew[j] = it - newknots.begin(); } const std::vector& cvals = Constraints.getValues(); std::vector newcVals(0); - // modify pole constraints - for (std::vector::const_iterator it = cvals.begin(); it != cvals.end(); - ++it) { - if ((*it)->Type == Sketcher::InternalAlignment && (*it)->Second == GeoId) { - if ((*it)->AlignmentType == Sketcher::BSplineControlPoint) { - if (prevpole[(*it)->InternalAlignmentIndex] != -1) { - assert(prevpole[(*it)->InternalAlignmentIndex] < bspline->countPoles()); - Constraint* newConstr = (*it)->clone(); - newConstr->InternalAlignmentIndex = prevpole[(*it)->InternalAlignmentIndex]; - newcVals.push_back(newConstr); - } - else { - // it is an internal alignment geometry that is no longer valid => delete it and - // the pole circle - delGeoId.push_back((*it)->First); - } - } - else if ((*it)->AlignmentType == Sketcher::BSplineKnotPoint) { - if (prevknot[(*it)->InternalAlignmentIndex] != -1) { - assert(prevknot[(*it)->InternalAlignmentIndex] < bspline->countKnots()); - Constraint* newConstr = (*it)->clone(); - newConstr->InternalAlignmentIndex = prevknot[(*it)->InternalAlignmentIndex]; - newcVals.push_back(newConstr); - } - else { - // it is an internal alignment geometry that is no longer valid => delete it and - // the knot point - delGeoId.push_back((*it)->First); - } - } - else {// it is a bspline geometry, but not a controlpoint or knot - newcVals.push_back(*it); - } + // modify pole and knot constraints + for (const auto& constr : cvals) { + if (!(constr->Type == Sketcher::InternalAlignment && constr->Second == GeoId)) { + newcVals.push_back(constr); + continue; + } + + std::vector* indexInNew = nullptr; + + if (constr->AlignmentType == Sketcher::BSplineControlPoint) { + indexInNew = &poleIndexInNew; + } + else if (constr->AlignmentType == Sketcher::BSplineKnotPoint) { + indexInNew = &knotIndexInNew; } else { - newcVals.push_back(*it); + // it is a bspline geometry, but not a controlpoint or knot + newcVals.push_back(constr); + continue; } + + if (indexInNew && indexInNew->at(constr->InternalAlignmentIndex) == -1) { + // it is an internal alignment geometry that is no longer valid + // => delete it and the pole circle + delGeoId.push_back(constr->First); + continue; + } + + Constraint* newConstr = constr->clone(); + newConstr->InternalAlignmentIndex = indexInNew->at(constr->InternalAlignmentIndex); + newcVals.push_back(newConstr); } const std::vector& vals = getInternalGeometry(); From e4f906c23b36571a32d58d95703901838fd8b1e6 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Tue, 24 Dec 2024 20:13:59 +0530 Subject: [PATCH 16/26] [Sketcher] Refactor `SketchObject::trim` Cognitive complexity down to 57. --- src/Mod/Sketcher/App/SketchObject.cpp | 374 ++++++++++++-------------- 1 file changed, 171 insertions(+), 203 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index aaf81eb3fe..2fb3ba3030 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -3540,6 +3540,25 @@ bool SketchObject::seekTrimPoints(int GeoId, const Base::Vector3d& point, int& G return true; } +// given a geometry and a point, returns the corresponding parameter of the geometry point +// closest to the point. Wrapped around a try-catch so the calling operation can fail without +// throwing an exception. +bool getIntersectionParameter(const Part::Geometry* geo, + const Base::Vector3d point, + double& pointParam) { + const auto* curve = static_cast(geo); + + try { + curve->closestParameter(point, pointParam); + } + catch (Base::CADKernelError& e) { + e.ReportException(); + return false; + } + + return true; +} + int SketchObject::trim(int GeoId, const Base::Vector3d& point) { // no need to check input data validity as this is an sketchobject managed operation. @@ -3547,64 +3566,88 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) //******************* Basic checks rejecting the operation //****************************************// - if (GeoId < 0 || GeoId > getHighestCurveIndex()) + if (GeoId < 0 || GeoId > getHighestCurveIndex()) { return -1; + } auto geo = getGeometry(GeoId); - if (!GeometryFacade::isInternalType(geo, InternalType::None)) - return -1;// internal alignment geometry is not trimmable + if (!GeometryFacade::isInternalType(geo, InternalType::None)) { + return -1; // internal alignment geometry is not trimmable + } //******************* Lambdas - common functions for different intersections //****************************************// - // returns true if the point defined by (GeoId1, pos1) can be considered to be coincident with - // point. - auto arePointsWithinPrecision = [](Base::Vector3d point1, Base::Vector3d& point2) { + auto arePointsWithinPrecision = [](Base::Vector3d& point1, Base::Vector3d& point2) { // From testing: 500x (or 0.000050) is needed in order to not falsely distinguish points // calculated with seekTrimPoints - return ((point1 - point2).Length() < 500 * Precision::Confusion()) ; + return ((point1 - point2).Length() < 500 * Precision::Confusion()); }; - auto isPointAtPosition = [this, &arePointsWithinPrecision] - (int GeoId1, PointPos pos1, Base::Vector3d point) { - Base::Vector3d pp = getPoint(GeoId1, pos1); + // returns true if the point defined by (GeoId1, pos1) can be considered to be coincident with + // point. + 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. - auto transformPreexistingConstraint = [this, isPointAtPosition](int GeoId, - int GeoId1, - Base::Vector3d point1, - Constraint* constr) { - /* It is possible that the trimming entity has both a PointOnObject constraint to the + // Preexisting point on object constraints get converted to coincidents. + // Returns: + // - The constraint that should be used to constraint GeoId and cuttingGeoId + auto transformPreexistingConstraint = [this, &isPointAtPosition](const Constraint* constr, + int GeoId, + int cuttingGeoId, + Base::Vector3d& cutPointVec, + int newGeoId, + PointPos newPosId) { + /* TODO: It is possible that the trimming entity has both a PointOnObject constraint to the * trimmed entity, and a simple Tangent constraint to the trimmed entity. In this case we - * want to change to a single end-to-end tangency, i.e we want to ensure that constrType1 is - * set to Sketcher::Tangent, that the secondPos1 is captured from the PointOnObject, and - * also make sure that the PointOnObject constraint is deleted. The below loop ensures this, - * also in case the ordering of the constraints is first Tangent and then PointOnObject. */ - // if (!(constr->Type == Sketcher::Tangent - // || constr->Type == Sketcher::Perpendicular)) { - // return; - // } - - // if (constr->First == GeoId1 && constr->Second == GeoId) { - // constr->Type = constr->Type; - // if (secondPos == Sketcher::PointPos::none) - // secondPos = constr->FirstPos; - // delete_list.push_back(constrId); - // } - // else if (constr->First == GeoId && constr->Second == GeoId1) { - // constr->Type = constr->Type; - // if (secondPos == Sketcher::PointPos::none) - // secondPos = constr->SecondPos; - // delete_list.push_back(constrId); - // } + * want to change to a single end-to-end tangency, i.e we want to ensure that constrType1 + * is set to Sketcher::Tangent, that the secondPos1 is captured from the PointOnObject, + * and also make sure that the PointOnObject constraint is deleted. + */ + std::unique_ptr newConstr; + switch (constr->Type) { + case PointOnObject: { + // we might want to transform this (and the new point-on-object constraints) into a coincidence + if (constr->First == cuttingGeoId + && isPointAtPosition(constr->First, constr->FirstPos, cutPointVec)) { + // This concerns the start portion of the trim + newConstr.reset(constr->copy()); + newConstr->Type = Sketcher::Coincident; + newConstr->Second = newGeoId; + newConstr->SecondPos = newPosId; + } + break; + } + case Tangent: + case Perpendicular: { + // 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 (!constr->involvesGeoId(cuttingGeoId) + || !constr->involvesGeoIdAndPosId(GeoId, PointPos::none)) { + break; + } + newConstr.reset(constr->copy()); + newConstr->substituteIndexAndPos(GeoId, PointPos::none, newGeoId, newPosId); + // make sure the first position is a point + if (newConstr->FirstPos == PointPos::none) { + std::swap(newConstr->First, newConstr->Second); + std::swap(newConstr->FirstPos, newConstr->SecondPos); + } + // there is no need for the third point if it exists + newConstr->Third = GeoEnum::GeoUndef; + newConstr->ThirdPos = PointPos::none; + break; + } + default: + break; + } + return newConstr; }; // makes an equality constraint between GeoId1 and GeoId2 @@ -3620,57 +3663,41 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) addConstraint(std::move(newConstr)); }; - // given a geometry and tree points, returns the corresponding parameters of the geometry points - // closest to them - auto getIntersectionParameters = [](const Part::Geometry* geo, - const Base::Vector3d point, - double& pointParam, - const Base::Vector3d point1, - double& point1Param, - const Base::Vector3d point2, - double& point2Param) { - auto curve = static_cast(geo); - - try { - curve->closestParameter(point, pointParam); - curve->closestParameter(point1, point1Param); - curve->closestParameter(point2, point2Param); - } - catch (Base::CADKernelError& e) { - e.ReportException(); - return false; - } - - return true; - }; - //******************* Step A => Detection of intersection - Common to all Geometries //****************************************// - int GeoId1 = GeoEnum::GeoUndef, GeoId2 = GeoEnum::GeoUndef; - Base::Vector3d point1, point2; + // GeoIds intersecting the curve around `point` + std::vector cuttingGeoIds(2, GeoEnum::GeoUndef); + // Points at the intersection + std::vector cutPoints(2); - // Remove internal geometry beforehand so GeoId1 and GeoId2 do not change + // Remove internal geometry beforehand so cuttingGeoIds[0] and cuttingGeoIds[1] do not change deleteUnusedInternalGeometryAndUpdateGeoId(GeoId); geo = getGeometry(GeoId); // Using SketchObject wrapper, as Part2DObject version returns GeoId = -1 when intersection not // found, which is wrong for a GeoId (axis). seekTrimPoints returns: // - For a parameter associated with "point" between an intersection and the end point - // (non-periodic case) GeoId1 != GeoUndef and GeoId2 == GeoUndef + // (non-periodic case) cuttingGeoIds[0] != GeoUndef and cuttingGeoIds[1] == GeoUndef // - For a parameter associated with "point" between the start point and an intersection - // (non-periodic case) GeoId2 != GeoUndef and GeoId1 == GeoUndef - // - For a parameter associated with "point" between two intersection points, GeoId1 != GeoUndef - // and GeoId2 != GeoUndef + // (non-periodic case) cuttingGeoIds[1] != GeoUndef and cuttingGeoIds[0] == GeoUndef + // - For a parameter associated with "point" between two intersection points, cuttingGeoIds[0] + // != GeoUndef and cuttingGeoIds[1] != GeoUndef // // FirstParam < point1param < point2param < LastParam - if (!SketchObject::seekTrimPoints(GeoId, point, GeoId1, point1, GeoId2, point2)) { + if (!SketchObject::seekTrimPoints(GeoId, + point, + cuttingGeoIds[0], + cutPoints[0], + cuttingGeoIds[1], + cutPoints[1])) { // If no suitable trim points are found, then trim defaults to deleting the geometry delGeometry(GeoId); return 0; } - if (GeoId1 != GeoEnum::GeoUndef && GeoId2 != GeoEnum::GeoUndef - && arePointsWithinPrecision(point1, point2)) { + size_t numUndefs = std::count(cuttingGeoIds.begin(), cuttingGeoIds.end(), GeoEnum::GeoUndef); + + if (numUndefs == 0 && arePointsWithinPrecision(cutPoints[0], cutPoints[1])) { // If both points are detected and are coincident, deletion is the only option. delGeometry(GeoId); return 0; @@ -3679,46 +3706,31 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) const auto* geoAsCurve = static_cast(geo); double firstParam = geoAsCurve->getFirstParameter(); double lastParam = geoAsCurve->getLastParameter(); - double pointParam, point1Param, point2Param; - if (!getIntersectionParameters( - geo, point, pointParam, point1, point1Param, point2, point2Param)) { + double pointParam, cut0Param, cut1Param; + bool allParamsFound = getIntersectionParameter(geo, point, pointParam) + && getIntersectionParameter(geo, cutPoints[0], cut0Param) + && getIntersectionParameter(geo, cutPoints[1], cut1Param); + if (!allParamsFound) { return -1; } - bool startPointRemains = false; - bool endPointRemains = false; - std::vector > paramsOfNewGeos; + bool startPointRemains = (cuttingGeoIds[0] != GeoEnum::GeoUndef); + bool endPointRemains = (cuttingGeoIds[1] != GeoEnum::GeoUndef); + std::vector> paramsOfNewGeos(2 - numUndefs, {firstParam, lastParam}); std::vector newIds; std::vector newGeos; std::vector newGeosAsConsts; - bool oldGeoIsConstruction = GeometryFacade::getConstruction(static_cast(geo)); + // TODO: This should be needed, but for some reason we already get both curves as construction + // when needed bool oldGeoIsConstruction = GeometryFacade::getConstruction(static_cast(geo)); - if (isClosedCurve(geo)) { + if (isClosedCurve(geo) && !paramsOfNewGeos.empty()) { startPointRemains = false; endPointRemains = false; - if (GeoId1 != GeoEnum::GeoUndef && GeoId2 != GeoEnum::GeoUndef) { - paramsOfNewGeos.emplace_back(point2Param, point1Param); - } - } - else { - if (GeoId1 != GeoEnum::GeoUndef) { - startPointRemains = true; - paramsOfNewGeos.emplace_back(firstParam, point1Param); - } - if (GeoId2 != GeoEnum::GeoUndef) { - endPointRemains = true; - paramsOfNewGeos.emplace_back(point2Param, lastParam); - } + paramsOfNewGeos.pop_back(); } - 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()) { + switch (paramsOfNewGeos.size()) { case 0: { delGeometry(GeoId); return 0; @@ -3733,13 +3745,24 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) break; } default: { - for (auto& newGeo : newGeos) { - delete newGeo; - } return -1; } } + if (cuttingGeoIds[0] != GeoEnum::GeoUndef) { + paramsOfNewGeos.front().second = cut0Param; + } + if (cuttingGeoIds[1] != GeoEnum::GeoUndef) { + paramsOfNewGeos.back().first = cut1Param; + } + + for (auto& [u1, u2] : paramsOfNewGeos) { + auto newGeo = static_cast(geo)->createArc(u1, u2); + assert(newGeo); + newGeos.push_back(newGeo); + 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`. @@ -3759,7 +3782,8 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) if (endPointRemains) { transferConstraints(GeoId, PointPos::end, newIds.back(), PointPos::end, true); } - bool geomHasMid = geo->isDerivedFrom() || geo->isDerivedFrom(); + bool geomHasMid = + geo->isDerivedFrom() || geo->isDerivedFrom(); if (geomHasMid) { transferConstraints(GeoId, PointPos::mid, newIds.front(), PointPos::mid, true); // Make centers coincident @@ -3792,80 +3816,32 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) // 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::set< int, std::greater<> > geoIdsToBeDeleted; + std::set> geoIdsToBeDeleted; // geoIdsToBeDeleted.push_back(GeoId); - for (const auto& oldConstrId: idsOfOldConstraints) { + for (const auto& oldConstrId : idsOfOldConstraints) { // trim-specific changes first const Constraint* con = allConstraints[oldConstrId]; - bool constraintWasModified = false; - switch (con->Type) { - case PointOnObject: { - // we might want to transform this (and the new point-on-object constraints) into a coincidence - 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; - newConstraints.push_back(newConstr); - isPoint1ConstrainedOnGeoId1 = true; - constraintWasModified = true; - } - 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; - newConstraints.push_back(newConstr); - isPoint2ConstrainedOnGeoId2 = true; - constraintWasModified = true; - } - break; - } - case Tangent: - case Perpendicular: { - // 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: { + if (con->Type == InternalAlignment) { geoIdsToBeDeleted.insert(con->First); - break; + continue; } - default: - break; + std::unique_ptr newConstr = transformPreexistingConstraint( + con, GeoId, cuttingGeoIds[0], cutPoints[0], newIds.front(), PointPos::end); + if (newConstr != nullptr) { + newConstraints.push_back(newConstr.release()); + isPoint1ConstrainedOnGeoId1 = true; + continue; } - if (!constraintWasModified) { - deriveConstraintsForPieces(GeoId, newIds, newGeosAsConsts, con, newConstraints); + newConstr = transformPreexistingConstraint( + con, GeoId, cuttingGeoIds[1], cutPoints[1], newIds.back(), PointPos::start); + if (newConstr != nullptr) { + newConstraints.push_back(newConstr.release()); + isPoint2ConstrainedOnGeoId2 = true; + continue; } + // constraint has not yet been changed + deriveConstraintsForPieces(GeoId, newIds, newGeosAsConsts, con, newConstraints); } // Add point-on-object/coincidence constraints with the newly exposed points. @@ -3873,16 +3849,19 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) // to coincident or end-to-end tangency/perpendicularity. // TODO: Tangent/perpendicular not yet covered - if (GeoId1 != GeoEnum::GeoUndef && !isPoint1ConstrainedOnGeoId1) { + auto addNewConstraintAtCut = [&isPointAtPosition, &newConstraints](int cuttingGeoId, + int cutGeoId, + PointPos cutPosId, + Base::Vector3d cutPointVec) { Constraint* newConstr = new Constraint(); - newConstr->First = newIds.front(); - newConstr->FirstPos = PointPos::end; - newConstr->Second = GeoId1; - if (isPointAtPosition(GeoId1, Sketcher::PointPos::start, point1)) { + newConstr->First = cutGeoId; + newConstr->FirstPos = cutPosId; + newConstr->Second = cuttingGeoId; + if (isPointAtPosition(cuttingGeoId, PointPos::start, cutPointVec)) { newConstr->Type = Sketcher::Coincident; newConstr->SecondPos = PointPos::start; } - else if (isPointAtPosition(GeoId1, Sketcher::PointPos::end, point1)) { + else if (isPointAtPosition(cuttingGeoId, PointPos::end, cutPointVec)) { newConstr->Type = Sketcher::Coincident; newConstr->SecondPos = PointPos::end; } @@ -3892,27 +3871,14 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) newConstr->SecondPos = PointPos::none; } newConstraints.push_back(newConstr); + }; + + if (cuttingGeoIds[0] != GeoEnum::GeoUndef && !isPoint1ConstrainedOnGeoId1) { + addNewConstraintAtCut(cuttingGeoIds[0], newIds.front(), PointPos::end, cutPoints[0]); } - if (GeoId2 != GeoEnum::GeoUndef && !isPoint2ConstrainedOnGeoId2) { - Constraint* newConstr = new Constraint(); - newConstr->First = newIds.back(); - newConstr->FirstPos = PointPos::start; - newConstr->Second = GeoId2; - if (isPointAtPosition(GeoId2, Sketcher::PointPos::start, point2)) { - newConstr->Type = Sketcher::Coincident; - newConstr->SecondPos = PointPos::start; - } - else if (isPointAtPosition(GeoId2, Sketcher::PointPos::end, point2)) { - newConstr->Type = Sketcher::Coincident; - newConstr->SecondPos = PointPos::end; - } - else { - // Points are sufficiently far apart: use point-on-object - newConstr->Type = Sketcher::PointOnObject; - newConstr->SecondPos = PointPos::none; - } - newConstraints.push_back(newConstr); + if (cuttingGeoIds[1] != GeoEnum::GeoUndef && !isPoint2ConstrainedOnGeoId2) { + addNewConstraintAtCut(cuttingGeoIds[1], newIds.back(), PointPos::start, cutPoints[1]); } // Workaround to avoid https://github.com/FreeCAD/FreeCAD/issues/15484. @@ -3936,7 +3902,8 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) Geometry.setValues(std::move(newVals)); // FIXME: Somewhere here we need to delete the internal geometry (even if in use) - // FIXME: Set the second geoid as construction or not depending on what the original was + // FIXME: Set the second geoid as construction or not depending on what the original was. + // This somehow is still not needed (why?) if (noRecomputes) { solve(); @@ -3949,8 +3916,9 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) } addConstraints(newConstraints); - if (noRecomputes) + if (noRecomputes) { solve(); + } // Since we used regular "non-smart" pointers, we have to handle cleanup for (auto& cons : newConstraints) { @@ -7178,7 +7146,7 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m const Part::Geometry* geo = getGeometry(GeoId); - if (geo->getTypeId() != Part::GeomBSplineCurve::getClassTypeId()) { + if (!geo->is()) { THROWMT(Base::TypeError, QT_TRANSLATE_NOOP("Exceptions", "The Geometry Index (GeoId) provided is not a B-spline.")); From 226d24792d674e79ce40ce66ccd2eb7ddc29a922 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Thu, 26 Dec 2024 15:28:40 +0530 Subject: [PATCH 17/26] [Sketcher] Fix some issues in `trim` Fixes fails at cases where one (or both) of the remaining segments is (are) degenerate. This existed pre-refactor. Fixes cases where there are some constraints on internal geometry that do not get deleted cleanly. Also fixes a memory leak. --- src/Mod/Sketcher/App/SketchObject.cpp | 75 +++++++++++++++++---------- 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 2fb3ba3030..1117ec4f22 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -3585,6 +3585,12 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) return ((point1 - point2).Length() < 500 * Precision::Confusion()); }; + auto areParamsWithinApproximation = [](double param1, double param2) { + // From testing: 500x (or 0.000050) is needed in order to not falsely distinguish points + // calculated with seekTrimPoints + return (std::abs(param1 - param2) < Precision::PApproximation()); + }; + // returns true if the point defined by (GeoId1, pos1) can be considered to be coincident with // point. auto isPointAtPosition = @@ -3611,11 +3617,15 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) * and also make sure that the PointOnObject constraint is deleted. */ std::unique_ptr newConstr; + if (!constr->involvesGeoId(cuttingGeoId) + || !constr->involvesGeoIdAndPosId(GeoId, PointPos::none)) { + return newConstr; + } switch (constr->Type) { case PointOnObject: { // we might want to transform this (and the new point-on-object constraints) into a coincidence - if (constr->First == cuttingGeoId - && isPointAtPosition(constr->First, constr->FirstPos, cutPointVec)) { + // At this stage of the check the point has to be an end of `cuttingGeoId` on the edge of `GeoId`. + if (isPointAtPosition(constr->First, constr->FirstPos, cutPointVec)) { // This concerns the start portion of the trim newConstr.reset(constr->copy()); newConstr->Type = Sketcher::Coincident; @@ -3628,10 +3638,6 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) case Perpendicular: { // 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 (!constr->involvesGeoId(cuttingGeoId) - || !constr->involvesGeoIdAndPosId(GeoId, PointPos::none)) { - break; - } newConstr.reset(constr->copy()); newConstr->substituteIndexAndPos(GeoId, PointPos::none, newGeoId, newPosId); // make sure the first position is a point @@ -3670,10 +3676,6 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) // Points at the intersection std::vector cutPoints(2); - // Remove internal geometry beforehand so cuttingGeoIds[0] and cuttingGeoIds[1] do not change - deleteUnusedInternalGeometryAndUpdateGeoId(GeoId); - geo = getGeometry(GeoId); - // Using SketchObject wrapper, as Part2DObject version returns GeoId = -1 when intersection not // found, which is wrong for a GeoId (axis). seekTrimPoints returns: // - For a parameter associated with "point" between an intersection and the end point @@ -3695,6 +3697,25 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) return 0; } + const auto* geoAsCurve = static_cast(geo); + double firstParam = geoAsCurve->getFirstParameter(); + double lastParam = geoAsCurve->getLastParameter(); + double pointParam, cut0Param{firstParam}, cut1Param{lastParam}; + bool allParamsFound = getIntersectionParameter(geo, point, pointParam) + && getIntersectionParameter(geo, cutPoints[0], cut0Param) + && getIntersectionParameter(geo, cutPoints[1], cut1Param); + if (!allParamsFound) { + return -1; + } + + if (!isClosedCurve(geo) && areParamsWithinApproximation(firstParam, cut0Param)) { + cuttingGeoIds[0] = GeoEnum::GeoUndef; + } + + if (!isClosedCurve(geo) && areParamsWithinApproximation(lastParam, cut1Param)) { + cuttingGeoIds[1] = GeoEnum::GeoUndef; + } + size_t numUndefs = std::count(cuttingGeoIds.begin(), cuttingGeoIds.end(), GeoEnum::GeoUndef); if (numUndefs == 0 && arePointsWithinPrecision(cutPoints[0], cutPoints[1])) { @@ -3703,17 +3724,6 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) return 0; } - const auto* geoAsCurve = static_cast(geo); - double firstParam = geoAsCurve->getFirstParameter(); - double lastParam = geoAsCurve->getLastParameter(); - double pointParam, cut0Param, cut1Param; - bool allParamsFound = getIntersectionParameter(geo, point, pointParam) - && getIntersectionParameter(geo, cutPoints[0], cut0Param) - && getIntersectionParameter(geo, cutPoints[1], cut1Param); - if (!allParamsFound) { - return -1; - } - bool startPointRemains = (cuttingGeoIds[0] != GeoEnum::GeoUndef); bool endPointRemains = (cuttingGeoIds[1] != GeoEnum::GeoUndef); std::vector> paramsOfNewGeos(2 - numUndefs, {firstParam, lastParam}); @@ -3721,8 +3731,10 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) std::vector newGeos; std::vector newGeosAsConsts; // TODO: This should be needed, but for some reason we already get both curves as construction - // when needed bool oldGeoIsConstruction = GeometryFacade::getConstruction(static_cast(geo)); + // when needed. + + // bool oldGeoIsConstruction = + // GeometryFacade::getConstruction(static_cast(geo)); if (isClosedCurve(geo) && !paramsOfNewGeos.empty()) { startPointRemains = false; @@ -3789,7 +3801,6 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) // Make centers coincident if (newIds.size() > 1) { Constraint* joint = new Constraint(); - joint = new Constraint(); joint->Type = Coincident; joint->First = newIds.front(); joint->FirstPos = PointPos::mid; @@ -3840,6 +3851,11 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) isPoint2ConstrainedOnGeoId2 = true; continue; } + // We have already transferred all constraints on endpoints to the new pieces. + // If there is still any left, this means one of the remaining pieces was degenerate. + if (!con->involvesGeoIdAndPosId(GeoId, PointPos::none)) { + continue; + } // constraint has not yet been changed deriveConstraintsForPieces(GeoId, newIds, newGeosAsConsts, con, newConstraints); } @@ -3893,6 +3909,7 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) auto vals = getInternalGeometry(); auto newVals(vals); + GeometryFacade::copyId(geo, newGeos.front()); newVals[GeoId] = newGeos.front(); if (newGeos.size() > 1) { newVals.push_back(newGeos.back()); @@ -3914,6 +3931,13 @@ 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()); + delGeometries(geoIdsToBeDeleted.begin(), geoIdsToBeDeleted.end()); addConstraints(newConstraints); if (noRecomputes) { @@ -4039,8 +4063,7 @@ bool SketchObject::deriveConstraintsForPieces(const int oldId, 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(); + transferToAll = geo->is() || geo->is(); break; } default: From 12102abd6bd2e2727c499e1b03525e61a743ca6a Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 29 Dec 2024 03:26:39 +0530 Subject: [PATCH 18/26] [Sketcher] Incorporate review of #18665 ...by hyarion on December 29. --- src/Mod/Sketcher/App/SketchObject.cpp | 129 ++++++++++++-------------- 1 file changed, 57 insertions(+), 72 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 1117ec4f22..5b0e5ce044 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -1596,7 +1596,7 @@ Base::Axis SketchObject::getAxis(int axId) const if (geo && GeometryFacade::getConstruction(geo) && geo->is()) { if (count == axId) { - Part::GeomLineSegment* lineSeg = static_cast(geo); + auto* lineSeg = static_cast(geo); Base::Vector3d start = lineSeg->getStartPoint(); Base::Vector3d end = lineSeg->getEndPoint(); return Base::Axis(start, end - start); @@ -1771,8 +1771,7 @@ int SketchObject::delGeometry(int GeoId, bool deleteinternalgeo) std::vector newConstraints; newConstraints.reserve(constraints.size()); for (const auto& constr : constraints) { - auto newConstr = getConstraintAfterDeletingGeo(constr, GeoId); - if (newConstr) { + if (auto newConstr = getConstraintAfterDeletingGeo(constr, GeoId)) { newConstraints.push_back(newConstr.release()); } } @@ -3604,12 +3603,12 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) // Preexisting point on object constraints get converted to coincidents. // Returns: // - The constraint that should be used to constraint GeoId and cuttingGeoId - auto transformPreexistingConstraint = [this, &isPointAtPosition](const Constraint* constr, - int GeoId, - int cuttingGeoId, - Base::Vector3d& cutPointVec, - int newGeoId, - PointPos newPosId) { + auto transformPreexistingConstraint = [&isPointAtPosition](const Constraint* constr, + int GeoId, + int cuttingGeoId, + Base::Vector3d& cutPointVec, + int newGeoId, + PointPos newPosId) { /* TODO: It is possible that the trimming entity has both a PointOnObject constraint to the * trimmed entity, and a simple Tangent constraint to the trimmed entity. In this case we * want to change to a single end-to-end tangency, i.e we want to ensure that constrType1 @@ -3672,9 +3671,9 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) //******************* Step A => Detection of intersection - Common to all Geometries //****************************************// // GeoIds intersecting the curve around `point` - std::vector cuttingGeoIds(2, GeoEnum::GeoUndef); + std::array cuttingGeoIds {GeoEnum::GeoUndef, GeoEnum::GeoUndef}; // Points at the intersection - std::vector cutPoints(2); + std::array cutPoints; // Using SketchObject wrapper, as Part2DObject version returns GeoId = -1 when intersection not // found, which is wrong for a GeoId (axis). seekTrimPoints returns: @@ -3837,16 +3836,14 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) geoIdsToBeDeleted.insert(con->First); continue; } - std::unique_ptr newConstr = transformPreexistingConstraint( - con, GeoId, cuttingGeoIds[0], cutPoints[0], newIds.front(), PointPos::end); - if (newConstr != nullptr) { + if (auto newConstr = transformPreexistingConstraint( + con, GeoId, cuttingGeoIds[0], cutPoints[0], newIds.front(), PointPos::end)) { newConstraints.push_back(newConstr.release()); isPoint1ConstrainedOnGeoId1 = true; continue; } - newConstr = transformPreexistingConstraint( - con, GeoId, cuttingGeoIds[1], cutPoints[1], newIds.back(), PointPos::start); - if (newConstr != nullptr) { + if (auto newConstr = transformPreexistingConstraint( + con, GeoId, cuttingGeoIds[1], cutPoints[1], newIds.back(), PointPos::start)) { newConstraints.push_back(newConstr.release()); isPoint2ConstrainedOnGeoId2 = true; continue; @@ -4033,7 +4030,7 @@ bool SketchObject::deriveConstraintsForPieces(const int oldId, Base::Vector3d conPoint(getPoint(conId, conPos)); double conParam; - auto geoAsCurve = static_cast(geo); + auto* geoAsCurve = static_cast(geo); geoAsCurve->closestParameter(conPoint, conParam); // Choose based on where the closest point lies // If it's not there, just leave this constraint out @@ -4287,7 +4284,7 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) }); } else if (geo->is()) { - const Part::GeomBSplineCurve* bsp = static_cast(geo); + const auto* bsp = static_cast(geo); // what to do for periodic b-splines? originalIsPeriodic = bsp->isPeriodic(); @@ -4606,7 +4603,7 @@ bool SketchObject::isCarbonCopyAllowed(App::Document* pDoc, App::DocumentObject* } - SketchObject* psObj = static_cast(pObj); + auto* psObj = static_cast(pObj); // Sketches outside of the Document are NOT allowed if (this->getDocument() != pDoc) { @@ -5485,7 +5482,7 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 iterfirstpoint = ssp; } else if (geocopy->is()) { - Part::GeomCircle* geosymcircle = static_cast(geocopy); + auto* geosymcircle = static_cast(geocopy); Base::Vector3d cp = geosymcircle->getCenter(); Base::Vector3d scp = cp + double(x) * displacement + double(y) * perpendicularDisplacement; @@ -5496,7 +5493,7 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 iterfirstpoint = scp; } else if (geocopy->is()) { - Part::GeomArcOfCircle* geoaoc = static_cast(geocopy); + auto* geoaoc = static_cast(geocopy); Base::Vector3d cp = geoaoc->getCenter(); Base::Vector3d scp = cp + double(x) * displacement + double(y) * perpendicularDisplacement; @@ -5507,7 +5504,7 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 iterfirstpoint = geoaoc->getStartPoint(true); } else if (geocopy->is()) { - Part::GeomEllipse* geosymellipse = static_cast(geocopy); + auto* geosymellipse = static_cast(geocopy); Base::Vector3d cp = geosymellipse->getCenter(); Base::Vector3d scp = cp + double(x) * displacement + double(y) * perpendicularDisplacement; @@ -5518,7 +5515,7 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 iterfirstpoint = scp; } else if (geocopy->is()) { - Part::GeomArcOfEllipse* geoaoe = static_cast(geocopy); + auto* geoaoe = static_cast(geocopy); Base::Vector3d cp = geoaoe->getCenter(); Base::Vector3d scp = cp + double(x) * displacement + double(y) * perpendicularDisplacement; @@ -5553,7 +5550,7 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 iterfirstpoint = geoaoe->getStartPoint(true); } else if (geocopy->is()) { - Part::GeomBSplineCurve* geobsp = static_cast(geocopy); + auto* geobsp = static_cast(geocopy); std::vector poles = geobsp->getPoles(); @@ -5571,7 +5568,7 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 iterfirstpoint = geobsp->getStartPoint(); } else if (geocopy->is()) { - Part::GeomPoint* geopoint = static_cast(geocopy); + auto* geopoint = static_cast(geocopy); Base::Vector3d cp = geopoint->getPoint(); Base::Vector3d scp = cp + double(x) * displacement + double(y) * perpendicularDisplacement; @@ -6041,20 +6038,15 @@ int SketchObject::exposeInternalGeometryForType(const int Geo int currentgeoid = getHighestCurveIndex(); int incrgeo = 0; - Base::Vector3d center; - double majord; - double minord; - Base::Vector3d majdir; - std::vector igeo; std::vector icon; - const Part::GeomEllipse* ellipse = static_cast(geo); + const auto* ellipse = static_cast(geo); - center = ellipse->getCenter(); - majord = ellipse->getMajorRadius(); - minord = ellipse->getMinorRadius(); - majdir = ellipse->getMajorAxisDir(); + Base::Vector3d center {ellipse->getCenter()}; + double majord {ellipse->getMajorRadius()}; + double minord {ellipse->getMinorRadius()}; + Base::Vector3d majdir {ellipse->getMajorAxisDir()}; Base::Vector3d mindir = Vector3d(-majdir.y, majdir.x); @@ -6182,32 +6174,27 @@ int SketchObject::exposeInternalGeometryForType(const in int currentgeoid = getHighestCurveIndex(); int incrgeo = 0; - Base::Vector3d center; - double majord; - double minord; - Base::Vector3d majdir; - std::vector igeo; std::vector icon; - const Part::GeomArcOfEllipse* aoe = static_cast(geo); + const auto* aoe = static_cast(geo); - center = aoe->getCenter(); - majord = aoe->getMajorRadius(); - minord = aoe->getMinorRadius(); - majdir = aoe->getMajorAxisDir(); + Base::Vector3d center {aoe->getCenter()}; + double majord {aoe->getMajorRadius()}; + double minord {aoe->getMinorRadius()}; + Base::Vector3d majdir {aoe->getMajorAxisDir()}; - Base::Vector3d mindir = Vector3d(-majdir.y, majdir.x); + Base::Vector3d mindir {-majdir.y, majdir.x}; - Base::Vector3d majorpositiveend = center + majord * majdir; - Base::Vector3d majornegativeend = center - majord * majdir; - Base::Vector3d minorpositiveend = center + minord * mindir; - Base::Vector3d minornegativeend = center - minord * mindir; + Base::Vector3d majorpositiveend {center + majord * majdir}; + Base::Vector3d majornegativeend {center - majord * majdir}; + Base::Vector3d minorpositiveend {center + minord * mindir}; + Base::Vector3d minornegativeend {center - minord * mindir}; double df = sqrt(majord * majord - minord * minord); - Base::Vector3d focus1P = center + df * majdir; - Base::Vector3d focus2P = center - df * majdir; + Base::Vector3d focus1P {center + df * majdir}; + Base::Vector3d focus2P {center - df * majdir}; if (!major) { Part::GeomLineSegment* lmajor = new Part::GeomLineSegment(); @@ -6319,12 +6306,12 @@ int SketchObject::exposeInternalGeometryForType(const int currentgeoid = getHighestCurveIndex(); int incrgeo = 0; - const Part::GeomArcOfHyperbola* aoh = static_cast(geo); + const auto* aoh = static_cast(geo); - Base::Vector3d center = aoh->getCenter(); - double majord = aoh->getMajorRadius(); - double minord = aoh->getMinorRadius(); - Base::Vector3d majdir = aoh->getMajorAxisDir(); + Base::Vector3d center {aoh->getCenter()}; + double majord {aoh->getMajorRadius()}; + double minord {aoh->getMinorRadius()}; + Base::Vector3d majdir {aoh->getMajorAxisDir()}; std::vector igeo; std::vector icon; @@ -6432,10 +6419,10 @@ int SketchObject::exposeInternalGeometryForType(const i int currentgeoid = getHighestCurveIndex(); int incrgeo = 0; - const Part::GeomArcOfParabola* aop = static_cast(geo); + const auto* aop = static_cast(geo); - Base::Vector3d center = aop->getCenter(); - Base::Vector3d focusp = aop->getFocus(); + Base::Vector3d center {aop->getCenter()}; + Base::Vector3d focusp {aop->getFocus()}; std::vector igeo; std::vector icon; @@ -6494,7 +6481,7 @@ int SketchObject::exposeInternalGeometryForType(const in { const Part::Geometry* geo = getGeometry(GeoId); - const Part::GeomBSplineCurve* bsp = static_cast(geo); + const auto* bsp = static_cast(geo); // First we search what has to be restored std::vector controlpointgeoids(bsp->countPoles(), GeoEnum::GeoUndef); @@ -6502,8 +6489,6 @@ int SketchObject::exposeInternalGeometryForType(const in bool isfirstweightconstrained = false; - std::vector::iterator it; - const std::vector& vals = Constraints.getValues(); // search for existing poles @@ -6844,7 +6829,7 @@ int SketchObject::deleteUnusedInternalGeometryWhenOneFocus(int GeoId, bool delge int SketchObject::deleteUnusedInternalGeometryWhenBSpline(int GeoId, bool delgeoid) { - const Part::GeomBSplineCurve* bsp = static_cast(getGeometry(GeoId)); + const auto* bsp = static_cast(getGeometry(GeoId)); // First we search existing IA std::vector > poleGeoIdsAndConstraints(bsp->countPoles(), {GeoEnum::GeoUndef, 0}); @@ -6980,7 +6965,7 @@ bool SketchObject::convertToNURBS(int GeoId) if (geo->is()) return false; - const Part::GeomCurve* geo1 = static_cast(geo); + const auto* geo1 = static_cast(geo); Part::GeomBSplineCurve* bspline; @@ -6988,7 +6973,7 @@ bool SketchObject::convertToNURBS(int GeoId) bspline = geo1->toNurbs(geo1->getFirstParameter(), geo1->getLastParameter()); if (geo1->isDerivedFrom(Part::GeomArcOfConic::getClassTypeId())) { - const Part::GeomArcOfConic* geoaoc = static_cast(geo1); + const auto* geoaoc = static_cast(geo1); if (geoaoc->isReversed()) bspline->reverse(); @@ -7067,7 +7052,7 @@ bool SketchObject::increaseBSplineDegree(int GeoId, int degreeincrement /*= 1*/) if (geo->getTypeId() != Part::GeomBSplineCurve::getClassTypeId()) return false; - const Part::GeomBSplineCurve* bsp = static_cast(geo); + const auto* bsp = static_cast(geo); const Handle(Geom_BSplineCurve) curve = Handle(Geom_BSplineCurve)::DownCast(bsp->handle()); @@ -7109,7 +7094,7 @@ bool SketchObject::decreaseBSplineDegree(int GeoId, int degreedecrement /*= 1*/) if (geo->getTypeId() != Part::GeomBSplineCurve::getClassTypeId()) return false; - const Part::GeomBSplineCurve* bsp = static_cast(geo); + const auto* bsp = static_cast(geo); const Handle(Geom_BSplineCurve) curve = Handle(Geom_BSplineCurve)::DownCast(bsp->handle()); @@ -7175,7 +7160,7 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m "The Geometry Index (GeoId) provided is not a B-spline.")); } - const Part::GeomBSplineCurve* bsp = static_cast(geo); + const auto* bsp = static_cast(geo); int degree = bsp->getDegree(); @@ -10059,8 +10044,8 @@ double SketchObject::calculateAngleViaPoint(int GeoId1, int GeoId2, double px, d // Temporary sketch based calculation. Slow, but guaranteed consistency with constraints. Sketcher::Sketch sk; - const Part::GeomCurve* p1 = dynamic_cast(this->getGeometry(GeoId1)); - const Part::GeomCurve* p2 = dynamic_cast(this->getGeometry(GeoId2)); + const auto* p1 = dynamic_cast(this->getGeometry(GeoId1)); + const auto* p2 = dynamic_cast(this->getGeometry(GeoId2)); if (p1 && p2) { // TODO: Check if any of these are B-splines From e6becd0b612930948ca2eb918c7599c862c4d2a1 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 29 Dec 2024 05:26:13 +0530 Subject: [PATCH 19/26] [Sketcher] Further refactor `modifyBSplineKnotMultiplicity` --- src/Mod/Sketcher/App/SketchObject.cpp | 35 +++++++++++---------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 5b0e5ce044..ec535c3f41 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -7221,19 +7221,24 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m std::vector newpoles = bspline->getPoles(); std::vector poleIndexInNew(bsp->countPoles(), -1); - for (int j = 0; j < int(poles.size()); j++) { - const auto it = std::find(newpoles.begin(), newpoles.end(), poles[j]); - poleIndexInNew[j] = it - newpoles.begin(); - } // on fully removing a knot the knot geometry changes std::vector knots = bsp->getKnots(); std::vector newknots = bspline->getKnots(); std::vector knotIndexInNew(bsp->countKnots(), -1); + std::map> + indexInNew {{Sketcher::BSplineControlPoint, {bsp->countPoles(), -1}}, + {Sketcher::BSplineKnotPoint, {bsp->countKnots(), -1}}}; + + for (int j = 0; j < int(poles.size()); j++) { + const auto it = std::find(newpoles.begin(), newpoles.end(), poles[j]); + indexInNew[Sketcher::BSplineControlPoint][j] = it - newpoles.begin(); + } + for (int j = 0; j < int(knots.size()); j++) { const auto it = std::find(newknots.begin(), newknots.end(), knots[j]); - knotIndexInNew[j] = it - newknots.begin(); + indexInNew[Sketcher::BSplineKnotPoint][j] = it - newknots.begin(); } const std::vector& cvals = Constraints.getValues(); @@ -7247,29 +7252,17 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m continue; } - std::vector* indexInNew = nullptr; + int index = indexInNew.at(constr->AlignmentType).at(constr->InternalAlignmentIndex); - if (constr->AlignmentType == Sketcher::BSplineControlPoint) { - indexInNew = &poleIndexInNew; - } - else if (constr->AlignmentType == Sketcher::BSplineKnotPoint) { - indexInNew = &knotIndexInNew; - } - else { - // it is a bspline geometry, but not a controlpoint or knot - newcVals.push_back(constr); - continue; - } - - if (indexInNew && indexInNew->at(constr->InternalAlignmentIndex) == -1) { + if (index == -1) { // it is an internal alignment geometry that is no longer valid - // => delete it and the pole circle + // => delete it and the geometry delGeoId.push_back(constr->First); continue; } Constraint* newConstr = constr->clone(); - newConstr->InternalAlignmentIndex = indexInNew->at(constr->InternalAlignmentIndex); + newConstr->InternalAlignmentIndex = index; newcVals.push_back(newConstr); } From c6f5739763dd657b7bf16c139c6193423abe3658 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 29 Dec 2024 21:26:43 +0530 Subject: [PATCH 20/26] [Sketcher] Refactor `SketchObject::split` 1. Now uses `Part::Geometry::createArc`. 2. Removed type-specific codes. It can be generalized now. --- src/Mod/Sketcher/App/SketchObject.cpp | 225 ++++++-------------------- 1 file changed, 48 insertions(+), 177 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index ec535c3f41..f6203dadf8 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -4091,17 +4091,15 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) return -1; } - bool originalIsPeriodic = false; const Part::Geometry* geo = getGeometry(GeoId); + bool originalIsPeriodic = isClosedCurve(geo); std::vector newIds; std::vector newConstraints; Base::Vector3d startPoint, endPoint, splitPoint; double startParam, endParam, splitParam = 0.0; - auto createGeosFromPeriodic = [&](const Part::GeomCurve* curve, - auto getCurveWithLimitParams, - auto createAndTransferConstraints) { + auto createGeosFromPeriodic = [&](const Part::GeomCurve* curve) { // find split point curve->closestParameter(point, splitParam); double period = curve->getLastParameter() - curve->getFirstParameter(); @@ -4109,7 +4107,7 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) endParam = splitParam + period; // create new arc and restrict it - auto newCurve = getCurveWithLimitParams(curve, startParam, endParam); + auto newCurve = curve->createArc(startParam, endParam); int newId(GeoEnum::GeoUndef); newId = addGeometry(std::move(newCurve));// after here newCurve is a shell if (newId < 0) { @@ -4120,14 +4118,10 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) setConstruction(newId, GeometryFacade::getConstruction(curve)); exposeInternalGeometry(newId); - // transfer any constraints - createAndTransferConstraints(GeoId, newId); return true; }; - auto createGeosFromNonPeriodic = [&](const Part::GeomBoundedCurve* curve, - auto getCurveWithLimitParams, - auto createAndTransferConstraints) { + auto createGeosFromNonPeriodic = [&](const Part::GeomBoundedCurve* curve) { startPoint = curve->getStartPoint(); endPoint = curve->getEndPoint(); @@ -4143,7 +4137,7 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) } // create new curves - auto newCurve = getCurveWithLimitParams(curve, startParam, splitParam); + auto newCurve = curve->createArc(startParam, splitParam); int newId(GeoEnum::GeoUndef); newId = addGeometry(std::move(newCurve)); if (newId < 0) { @@ -4154,7 +4148,7 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) exposeInternalGeometry(newId); // the "second" half - newCurve = getCurveWithLimitParams(curve, splitParam, endParam); + newCurve = curve->createArc(splitParam, endParam); newId = addGeometry(std::move(newCurve)); if (newId < 0) { return false; @@ -4163,169 +4157,15 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) setConstruction(newId, GeometryFacade::getConstruction(curve)); exposeInternalGeometry(newId); - // TODO: Certain transfers and new constraint can be directly made here. - // But this may reduce readability. - // apply appropriate constraints on the new points at split point and - // transfer constraints from start and end of original spline - createAndTransferConstraints(GeoId, newIds[0], newIds[1]); return true; }; bool ok = false; - if (geo->is()) { - originalIsPeriodic = false; - ok = createGeosFromNonPeriodic( - static_cast(geo), - [](const Part::GeomCurve* curve, double startParam, double endParam) { - auto newArc = std::unique_ptr( - static_cast(curve->copy())); - newArc->setRange(startParam, endParam); - return newArc; - }, - [this, &newConstraints](int GeoId, int newId0, int newId1) { - Constraint* joint = new Constraint(); - joint->Type = Coincident; - joint->First = newId0; - joint->FirstPos = PointPos::end; - joint->Second = newId1; - joint->SecondPos = PointPos::start; - newConstraints.push_back(joint); - - transferConstraints(GeoId, PointPos::start, newId0, PointPos::start); - transferConstraints(GeoId, PointPos::end, newId1, PointPos::end); - }); + if (originalIsPeriodic) { + ok = createGeosFromPeriodic(static_cast(geo)); } - else if (geo->is()) { - originalIsPeriodic = true; - ok = createGeosFromPeriodic( - static_cast(geo), - [](const Part::GeomCurve* curve, double startParam, double endParam) { - auto newArc = std::make_unique( - Handle(Geom_Circle)::DownCast(curve->handle()->Copy())); - newArc->setRange(startParam, endParam, false); - return newArc; - }, - [this](int GeoId, int newId) { - transferConstraints(GeoId, PointPos::mid, newId, PointPos::mid); - }); - } - else if (geo->is()) { - originalIsPeriodic = true; - ok = createGeosFromPeriodic( - static_cast(geo), - [](const Part::GeomCurve* curve, double startParam, double endParam) { - auto newArc = std::make_unique( - Handle(Geom_Ellipse)::DownCast(curve->handle()->Copy())); - newArc->setRange(startParam, endParam, false); - return newArc; - }, - [this](int GeoId, int newId) { - transferConstraints(GeoId, PointPos::mid, newId, PointPos::mid); - }); - } - else if (geo->is()) { - originalIsPeriodic = false; - ok = createGeosFromNonPeriodic( - static_cast(geo), - [](const Part::GeomCurve* curve, double startParam, double endParam) { - auto newArc = std::unique_ptr( - static_cast(curve->copy())); - newArc->setRange(startParam, endParam, false); - return newArc; - }, - [this, &newConstraints](int GeoId, int newId0, int newId1) { - Constraint* joint = new Constraint(); - joint->Type = Coincident; - joint->First = newId0; - joint->FirstPos = PointPos::end; - joint->Second = newId1; - joint->SecondPos = PointPos::start; - newConstraints.push_back(joint); - - joint = new Constraint(); - joint->Type = Coincident; - joint->First = newId0; - joint->FirstPos = PointPos::mid; - joint->Second = newId1; - joint->SecondPos = PointPos::mid; - newConstraints.push_back(joint); - - transferConstraints(GeoId, PointPos::start, newId0, PointPos::start, true); - transferConstraints(GeoId, PointPos::mid, newId0, PointPos::mid); - transferConstraints(GeoId, PointPos::end, newId1, PointPos::end, true); - }); - } - else if (geo->isDerivedFrom(Part::GeomArcOfConic::getClassTypeId())) { - originalIsPeriodic = false; - ok = createGeosFromNonPeriodic( - static_cast(geo), - [](const Part::GeomCurve* curve, double startParam, double endParam) { - auto newArc = std::unique_ptr( - static_cast(curve->copy())); - newArc->setRange(startParam, endParam); - return newArc; - }, - [this, &newConstraints](int GeoId, int newId0, int newId1) { - // apply appropriate constraints on the new points at split point - Constraint* joint = new Constraint(); - joint->Type = Coincident; - joint->First = newId0; - joint->FirstPos = PointPos::end; - joint->Second = newId1; - joint->SecondPos = PointPos::start; - newConstraints.push_back(joint); - - // TODO: Do we apply constraints on center etc of the conics? - - // transfer constraints from start, mid and end of original - transferConstraints(GeoId, PointPos::start, newId0, PointPos::start, true); - transferConstraints(GeoId, PointPos::mid, newId0, PointPos::mid); - transferConstraints(GeoId, PointPos::end, newId1, PointPos::end, true); - }); - } - else if (geo->is()) { - const auto* bsp = static_cast(geo); - - // what to do for periodic b-splines? - originalIsPeriodic = bsp->isPeriodic(); - if (originalIsPeriodic) { - ok = createGeosFromPeriodic( - static_cast(geo), - [](const Part::GeomCurve* curve, double startParam, double endParam) { - auto newBsp = std::unique_ptr( - static_cast(curve->copy())); - newBsp->Trim(startParam, endParam); - return newBsp; - }, - [](int, int) { - // no constraints to transfer here, and we assume the split is to "break" the - // b-spline - }); - } - else { - ok = createGeosFromNonPeriodic( - static_cast(geo), - [](const Part::GeomCurve* curve, double startParam, double endParam) { - auto newBsp = std::unique_ptr( - static_cast(curve->copy())); - newBsp->Trim(startParam, endParam); - return newBsp; - }, - [this, &newConstraints](int GeoId, int newId0, int newId1) { - // apply appropriate constraints on the new points at split point - Constraint* joint = new Constraint(); - joint->Type = Coincident; - joint->First = newId0; - joint->FirstPos = PointPos::end; - joint->Second = newId1; - joint->SecondPos = PointPos::start; - newConstraints.push_back(joint); - - // transfer constraints from start and end of original spline - transferConstraints(GeoId, PointPos::start, newId0, PointPos::start, true); - transferConstraints(GeoId, PointPos::end, newId1, PointPos::end, true); - }); - } + else { + ok = createGeosFromNonPeriodic(static_cast(geo)); } if (!ok) { @@ -4336,6 +4176,35 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) return -1; } + if (!originalIsPeriodic) { + Constraint* joint = new Constraint(); + joint->Type = Coincident; + joint->First = newIds.front(); + joint->FirstPos = PointPos::end; + joint->Second = newIds.back(); + joint->SecondPos = PointPos::start; + newConstraints.push_back(joint); + + transferConstraints(GeoId, PointPos::start, newIds.front(), PointPos::start); + transferConstraints(GeoId, PointPos::end, newIds.back(), PointPos::end); + } + + // This additional constraint is there to maintain existing behavior. + // TODO: Decide whether to remove it altogether or also apply to other curves with centers. + if (geo->is()) { + Constraint* joint = new Constraint(); + joint->Type = Coincident; + joint->First = newIds.front(); + joint->FirstPos = PointPos::mid; + joint->Second = newIds.back(); + joint->SecondPos = PointPos::mid; + newConstraints.push_back(joint); + } + + if (geo->isDerivedFrom() || geo->isDerivedFrom()) { + transferConstraints(GeoId, PointPos::mid, newIds.front(), PointPos::mid); + } + std::vector idsOfOldConstraints; getConstraintIndices(GeoId, idsOfOldConstraints); @@ -4343,20 +4212,22 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) // keep constraints on internal geometries so they are deleted // when the old curve is deleted - idsOfOldConstraints.erase(std::remove_if(idsOfOldConstraints.begin(), - idsOfOldConstraints.end(), - [=](const auto& i) { - return allConstraints[i]->Type == InternalAlignment; - }), - idsOfOldConstraints.end()); + idsOfOldConstraints.erase( + std::remove_if(idsOfOldConstraints.begin(), + idsOfOldConstraints.end(), + [&allConstraints](const auto& i) { + return allConstraints[i]->Type == InternalAlignment; + }), + idsOfOldConstraints.end()); for (const auto& oldConstrId: idsOfOldConstraints) { Constraint* con = allConstraints[oldConstrId]; deriveConstraintsForPieces(GeoId, newIds, con, newConstraints); } - if (noRecomputes) + if (noRecomputes) { solve(); + } delConstraints(idsOfOldConstraints); addConstraints(newConstraints); From 821bc2ecdf70b806d17e9a74fb104e01c7747b19 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 30 Dec 2024 09:24:18 +0530 Subject: [PATCH 21/26] [Sketcher] Incorporate review of #18665 by kadet 1. Replace `find_if` with `any_of` when the iterator is not used. 2. Use PascalCase for templated class. --- src/Mod/Sketcher/App/SketchObject.cpp | 60 +++++++++++++-------------- src/Mod/Sketcher/App/SketchObject.h | 4 +- 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index f6203dadf8..bb67d49fd3 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -6381,11 +6381,10 @@ int SketchObject::exposeInternalGeometryForType(const in } if (controlpointgeoids[0] != GeoEnum::GeoUndef) { - // search for first pole weight constraint - auto it = std::find_if(vals.begin(), - vals.end(), - [&controlpointgeoids](const auto& constr) {return (constr->Type == Sketcher::Weight && constr->First == controlpointgeoids[0]);}); - isfirstweightconstrained = (it != vals.end()); + isfirstweightconstrained = + std::any_of(vals.begin(), vals.end(), [&controlpointgeoids](const auto& constr) { + return (constr->Type == Sketcher::Weight && constr->First == controlpointgeoids[0]); + }); } int currentgeoid = getHighestCurveIndex(); @@ -6739,21 +6738,19 @@ int SketchObject::deleteUnusedInternalGeometryWhenBSpline(int GeoId, bool delgeo if (constr->Type != Sketcher::Equal) { continue; } - - bool firstIsInCPGeoIds = std::find_if(poleGeoIdsAndConstraints.begin(), + bool firstIsInCPGeoIds = std::any_of(poleGeoIdsAndConstraints.begin(), + poleGeoIdsAndConstraints.end(), + [&constr](const auto& _pair) { + return _pair.first == constr->First; + }); + bool secondIsInCPGeoIds = std::any_of(poleGeoIdsAndConstraints.begin(), poleGeoIdsAndConstraints.end(), [&constr](const auto& _pair) { - return _pair.first == constr->First; - }) != poleGeoIdsAndConstraints.end(); - bool secondIsInCPGeoIds = std::find_if(poleGeoIdsAndConstraints.begin(), - poleGeoIdsAndConstraints.end(), - [&constr](const auto& _pair){ - return _pair.first == constr->Second; - }) != poleGeoIdsAndConstraints.end(); - + return _pair.first == constr->Second; + }); // the equality constraint constrains a pole but it is not interpole if (firstIsInCPGeoIds != secondIsInCPGeoIds) { - numConstr++; + ++numConstr; } // We do not ignore weight constraints as we did with radius constraints, // because the radius magnitude no longer makes sense without the B-Spline. @@ -10707,20 +10704,19 @@ void SketchObject::migrateSketch() auto constraints = Constraints.getValues(); auto geometries = getInternalGeometry(); - auto parabolafound = std::find_if(geometries.begin(), geometries.end(), [](Part::Geometry* g) { + bool parabolaFound = std::any_of(geometries.begin(), geometries.end(), [](Part::Geometry* g) { return g->is(); }); - if (parabolafound != geometries.end()) { + if (parabolaFound) { - auto focalaxisfound = std::find_if(constraints.begin(), constraints.end(), [](auto c) { + bool focalaxisfound = std::any_of(constraints.begin(), constraints.end(), [](auto c) { return c->Type == InternalAlignment && c->AlignmentType == ParabolaFocalAxis; }); // There are parabolas and there isn't an IA axis. (1) there are no axis or (2) there is a // legacy construction line - if (focalaxisfound == constraints.end()) { - + if (!focalaxisfound) { // maps parabola geoid to focusgeoid std::map parabolageoid2focusgeoid; @@ -10771,18 +10767,18 @@ void SketchObject::migrateSketch() } else { auto axismajorcoincidentfound = - std::find_if(axisgeoid2parabolageoid.begin(), - axisgeoid2parabolageoid.end(), - [&](const auto& pair) { - auto parabolageoid = pair.second; - auto axisgeoid = pair.first; - return (c->First == axisgeoid && c->Second == parabolageoid - && c->SecondPos == PointPos::mid) - || (c->Second == axisgeoid && c->First == parabolageoid - && c->FirstPos == PointPos::mid); - }); + std::any_of(axisgeoid2parabolageoid.begin(), + axisgeoid2parabolageoid.end(), + [&](const auto& pair) { + auto parabolageoid = pair.second; + auto axisgeoid = pair.first; + return (c->First == axisgeoid && c->Second == parabolageoid + && c->SecondPos == PointPos::mid) + || (c->Second == axisgeoid && c->First == parabolageoid + && c->FirstPos == PointPos::mid); + }); - if (axismajorcoincidentfound != axisgeoid2parabolageoid.end()) { + if (axismajorcoincidentfound) { // we skip this coincident, the other coincident on axis will be substituted // by internal geometry constraint continue; diff --git a/src/Mod/Sketcher/App/SketchObject.h b/src/Mod/Sketcher/App/SketchObject.h index 09aca4d269..7eaf8b0ba6 100644 --- a/src/Mod/Sketcher/App/SketchObject.h +++ b/src/Mod/Sketcher/App/SketchObject.h @@ -370,8 +370,8 @@ public: /// retrieves the coordinates of a point static Base::Vector3d getPoint(const Part::Geometry* geo, PointPos PosId); Base::Vector3d getPoint(int GeoId, PointPos PosId) const; - template - static Base::Vector3d getPointForGeometry(const geomType* geo, PointPos PosId) + template + static Base::Vector3d getPointForGeometry(const GeomType* geo, PointPos PosId) { (void)geo; (void)PosId; From 8f7a6628ca779d2023aba7a3519dd37e8d2eb101 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 5 Jan 2025 11:39:46 +0530 Subject: [PATCH 22/26] [Sketcher][test] Add new test for internal geometry deletion ...when some are "used" --- tests/src/Mod/Sketcher/App/SketchObject.cpp | 52 +++++++++++++++++++-- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/tests/src/Mod/Sketcher/App/SketchObject.cpp b/tests/src/Mod/Sketcher/App/SketchObject.cpp index 5a3f910892..068d0d4aa8 100644 --- a/tests/src/Mod/Sketcher/App/SketchObject.cpp +++ b/tests/src/Mod/Sketcher/App/SketchObject.cpp @@ -832,6 +832,44 @@ TEST_F(SketchObjectTest, testDeleteExposeInternalGeometryOfBSpline) EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); } +// TODO: Needs to be done for other curves too but currently they are working as intended +TEST_F(SketchObjectTest, testDeleteOnlyUnusedInternalGeometryOfBSpline) +{ + // NOTE: We test only non-periodic B-spline here. Periodic B-spline should behave exactly the + // same. + + // Arrange + auto nonPeriodicBSpline = createTypicalNonPeriodicBSpline(); + int geoIdBsp = getObject()->addGeometry(nonPeriodicBSpline.get()); + // Ensure "exposed" internal geometry + getObject()->exposeInternalGeometry(geoIdBsp); + Base::Vector3d coords(1.0, 1.0, 0.0); + Part::GeomPoint point(coords); + int geoIdPnt = getObject()->addGeometry(&point); + const auto constraints = getObject()->Constraints.getValues(); + auto it = std::find_if(constraints.begin(), constraints.end(), [&geoIdBsp](const auto* constr) { + return constr->Type == Sketcher::ConstraintType::InternalAlignment + && constr->AlignmentType == Sketcher::InternalAlignmentType::BSplineControlPoint + && constr->Second == geoIdBsp && constr->InternalAlignmentIndex == 1; + }); + // One Assert to avoid + EXPECT_NE(it, constraints.end()); + auto constraint = new Sketcher::Constraint(); // Ownership will be transferred to the sketch + constraint->Type = Sketcher::ConstraintType::Coincident; + constraint->First = geoIdPnt; + constraint->FirstPos = Sketcher::PointPos::start; + constraint->Second = (*it)->First; + constraint->SecondPos = Sketcher::PointPos::mid; + getObject()->addConstraint(constraint); + + // Act + getObject()->deleteUnusedInternalGeometryAndUpdateGeoId(geoIdBsp); + + // Assert + // Ensure there are 3 curves: the B-spline, its pole, and the point coincident on the pole + EXPECT_EQ(getObject()->getHighestCurveIndex(), 2); +} + TEST_F(SketchObjectTest, testSplitLineSegment) { // Arrange @@ -1419,6 +1457,8 @@ TEST_F(SketchObjectTest, testModifyKnotMultInNonPeriodicBSplineToZero) auto nonPeriodicBSpline = createTypicalNonPeriodicBSpline(); assert(nonPeriodicBSpline); int geoId = getObject()->addGeometry(nonPeriodicBSpline.get()); + auto bsp1 = static_cast(getObject()->getGeometry(geoId)); + int oldKnotCount = bsp1->countKnots(); // Act // Try decreasing mult to zero. @@ -1426,8 +1466,8 @@ TEST_F(SketchObjectTest, testModifyKnotMultInNonPeriodicBSplineToZero) getObject()->modifyBSplineKnotMultiplicity(geoId, 2, -1); // Assert // Knot should disappear. We start with 3 (unique) knots, so expect 2. - auto bsp = static_cast(getObject()->getGeometry(geoId)); - EXPECT_EQ(bsp->countKnots(), 2); + auto bsp2 = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp2->countKnots(), oldKnotCount - 1); } TEST_F(SketchObjectTest, testModifyKnotMultInNonPeriodicBSplineToDisallowed) @@ -1489,15 +1529,17 @@ TEST_F(SketchObjectTest, testModifyKnotMultInPeriodicBSplineToZero) auto PeriodicBSpline = createTypicalPeriodicBSpline(); assert(PeriodicBSpline); int geoId = getObject()->addGeometry(PeriodicBSpline.get()); + auto bsp1 = static_cast(getObject()->getGeometry(geoId)); + int oldKnotCount = bsp1->countKnots(); // Act // Try decreasing mult to zero. // NOTE: we still use OCCT notation of knot index starting with 1 (not 0). getObject()->modifyBSplineKnotMultiplicity(geoId, 2, -1); // Assert - // Knot should disappear. We start with 3 (unique) knots, so expect 2. - auto bsp = static_cast(getObject()->getGeometry(geoId)); - EXPECT_EQ(bsp->countKnots(), 5); + // Knot should disappear. + auto bsp2 = static_cast(getObject()->getGeometry(geoId)); + EXPECT_EQ(bsp2->countKnots(), oldKnotCount - 1); } TEST_F(SketchObjectTest, testModifyKnotMultInPeriodicBSplineToDisallowed) From aa1122c774aa5143d8a2697ce27bf59f9353e828 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 5 Jan 2025 11:42:37 +0530 Subject: [PATCH 23/26] [Sketcher] Fix "used" B-spline poles being deleted --- src/Mod/Sketcher/App/SketchObject.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index bb67d49fd3..6bc5e6f24f 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -6728,6 +6728,7 @@ int SketchObject::deleteUnusedInternalGeometryWhenBSpline(int GeoId, bool delgeo std::vector delgeometries; + // TODO: This can become significantly costly if there are lots of constraints and poles for (auto& [cpGeoId, numConstr] : poleGeoIdsAndConstraints) { if (cpGeoId == GeoEnum::GeoUndef) { continue; @@ -6735,7 +6736,13 @@ int SketchObject::deleteUnusedInternalGeometryWhenBSpline(int GeoId, bool delgeo // look for a circle at geoid index for (auto const& constr : vals) { + if (constr->Type == Sketcher::InternalAlignment + || constr->Type == Sketcher::Weight + || !constr->involvesGeoId(cpGeoId)) { + continue; + } if (constr->Type != Sketcher::Equal) { + ++numConstr; continue; } bool firstIsInCPGeoIds = std::any_of(poleGeoIdsAndConstraints.begin(), @@ -6756,7 +6763,7 @@ int SketchObject::deleteUnusedInternalGeometryWhenBSpline(int GeoId, bool delgeo // because the radius magnitude no longer makes sense without the B-Spline. } - if (numConstr < 2) { // IA + if (numConstr < 1) { // IA delgeometries.push_back(cpGeoId); } } From b2e414c40792ba899b33369e8e9e7c338c5b8fd4 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 5 Jan 2025 11:43:54 +0530 Subject: [PATCH 24/26] [Sketcher] Make `replaceGeometries` protected This should be a `protected` or `private` operation since it doesn't handle constraints. However, this is intended to be used by external classes that modify a `SketchObject` (instead of these modifications like `split`, `trim`, `join` etc. being methods of `SketchObject`). In that case, it may be best to use the `friend` keyword. --- src/Mod/Sketcher/App/SketchObject.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.h b/src/Mod/Sketcher/App/SketchObject.h index 7eaf8b0ba6..e586837318 100644 --- a/src/Mod/Sketcher/App/SketchObject.h +++ b/src/Mod/Sketcher/App/SketchObject.h @@ -173,10 +173,6 @@ public: int addConstraint(std::unique_ptr constraint); /// delete constraint int delConstraint(int ConstrId); - /// Replaces geometries at `oldGeoIds` with `newGeos`, lower Ids first. - /// If `oldGeoIds` is bigger, deletes the remaining. - /// If `newGeos` is bigger, adds the remaining geometries at the end. - void replaceGeometries(std::vector oldGeoIds, std::vector& newGeos); /** deletes a group of constraints at once, if norecomputes is active, the default behaviour is * that it will solve the sketch. * @@ -905,6 +901,12 @@ protected: /// get called by the container when a property has changed void onChanged(const App::Property* /*prop*/) override; + /// Replaces geometries at `oldGeoIds` with `newGeos`, lower Ids first. + /// If `oldGeoIds` is bigger, deletes the remaining. + /// If `newGeos` is bigger, adds the remaining geometries at the end. + /// NOTE: Does NOT move any constraints + void replaceGeometries(std::vector oldGeoIds, std::vector& newGeos); + /// Helper functions for `deleteUnusedInternalGeometry` by cases /// two foci for ellipses and arcs of ellipses and hyperbolas int deleteUnusedInternalGeometryWhenTwoFoci(int GeoId, bool delgeoid = false); From 73253bd2d4855cb2f302ab8f4fccb5b9303dfeac Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 5 Jan 2025 13:11:23 +0530 Subject: [PATCH 25/26] [Sketcher] Fix issues in knot operations For some reason this is not caught in the tests. --- src/Mod/Sketcher/App/SketchObject.cpp | 47 ++++++++++++++++----------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 6bc5e6f24f..b7c432a025 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -7093,28 +7093,35 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m std::vector delGeoId; std::vector poles = bsp->getPoles(); - std::vector newpoles = bspline->getPoles(); - std::vector poleIndexInNew(bsp->countPoles(), -1); - + std::vector newPoles = bspline->getPoles(); // on fully removing a knot the knot geometry changes std::vector knots = bsp->getKnots(); - std::vector newknots = bspline->getKnots(); - std::vector knotIndexInNew(bsp->countKnots(), -1); + std::vector newKnots = bspline->getKnots(); std::map> - indexInNew {{Sketcher::BSplineControlPoint, {bsp->countPoles(), -1}}, - {Sketcher::BSplineKnotPoint, {bsp->countKnots(), -1}}}; + indexInNew {{Sketcher::BSplineControlPoint, {}}, + {Sketcher::BSplineKnotPoint, {}}}; + indexInNew[Sketcher::BSplineControlPoint].reserve(poles.size()); + indexInNew[Sketcher::BSplineKnotPoint].reserve(knots.size()); - for (int j = 0; j < int(poles.size()); j++) { - const auto it = std::find(newpoles.begin(), newpoles.end(), poles[j]); - indexInNew[Sketcher::BSplineControlPoint][j] = it - newpoles.begin(); + for (const auto& pole : poles) { + const auto it = std::find(newPoles.begin(), newPoles.end(), pole); + indexInNew[Sketcher::BSplineControlPoint].emplace_back(it - newPoles.begin()); } + std::replace(indexInNew[Sketcher::BSplineControlPoint].begin(), + indexInNew[Sketcher::BSplineControlPoint].end(), + int(newPoles.size()), + -1); - for (int j = 0; j < int(knots.size()); j++) { - const auto it = std::find(newknots.begin(), newknots.end(), knots[j]); - indexInNew[Sketcher::BSplineKnotPoint][j] = it - newknots.begin(); + for (const auto& knot : knots) { + const auto it = std::find(newKnots.begin(), newKnots.end(), knot); + indexInNew[Sketcher::BSplineKnotPoint].emplace_back(it - newKnots.begin()); } + std::replace(indexInNew[Sketcher::BSplineKnotPoint].begin(), + indexInNew[Sketcher::BSplineKnotPoint].end(), + int(newKnots.size()), + -1); const std::vector& cvals = Constraints.getValues(); @@ -7230,22 +7237,24 @@ bool SketchObject::insertBSplineKnot(int GeoId, double param, int multiplicity) std::vector delGeoId; std::vector poles = bsp->getPoles(); - std::vector newpoles = bspline->getPoles(); + std::vector newPoles = bspline->getPoles(); std::vector poleIndexInNew(poles.size(), -1); for (size_t j = 0; j < poles.size(); j++) { - const auto it = std::find(newpoles.begin(), newpoles.end(), poles[j]); - poleIndexInNew[j] = it - newpoles.begin(); + const auto it = std::find(newPoles.begin(), newPoles.end(), poles[j]); + poleIndexInNew[j] = it - newPoles.begin(); } + std::replace(poleIndexInNew.begin(), poleIndexInNew.end(), int(newPoles.size()), -1); std::vector knots = bsp->getKnots(); - std::vector newknots = bspline->getKnots(); + std::vector newKnots = bspline->getKnots(); std::vector knotIndexInNew(knots.size(), -1); for (size_t j = 0; j < knots.size(); j++) { - const auto it = std::find(newknots.begin(), newknots.end(), knots[j]); - knotIndexInNew[j] = it - newknots.begin(); + const auto it = std::find(newKnots.begin(), newKnots.end(), knots[j]); + knotIndexInNew[j] = it - newKnots.begin(); } + std::replace(knotIndexInNew.begin(), knotIndexInNew.end(), int(newKnots.size()), -1); const std::vector& cvals = Constraints.getValues(); From 51cf34a5967f8bb0536b8d513e52d8e59ceead96 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 6 Jan 2025 19:08:55 +0530 Subject: [PATCH 26/26] [Sketcher] Refactor and fix an issue in `transferConstraints` --- src/Mod/Sketcher/App/SketchObject.cpp | 91 +++++++++++++-------------- 1 file changed, 42 insertions(+), 49 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index b7c432a025..746767bb0e 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -2627,8 +2627,12 @@ void SketchObject::transferFilletConstraints(int geoId1, PointPos posId1, int ge this->Constraints.setValues(std::move(newConstraints)); } -int SketchObject::transferConstraints(int fromGeoId, PointPos fromPosId, int toGeoId, - PointPos toPosId, bool doNotTransformTangencies) +// clang-format on +int SketchObject::transferConstraints(int fromGeoId, + PointPos fromPosId, + int toGeoId, + PointPos toPosId, + bool doNotTransformTangencies) { // no need to check input data validity as this is an sketchobject managed operation. Base::StateLocker lock(managedoperation, true); @@ -2637,58 +2641,46 @@ int SketchObject::transferConstraints(int fromGeoId, PointPos fromPosId, int toG std::vector newVals(vals); bool changed = false; for (int i = 0; i < int(newVals.size()); i++) { - if (vals[i]->First == fromGeoId && vals[i]->FirstPos == fromPosId - && !(vals[i]->Second == toGeoId && vals[i]->SecondPos == toPosId) - && !(toGeoId < 0 && vals[i]->Second < 0)) { - - std::unique_ptr constNew(newVals[i]->clone()); - constNew->First = toGeoId; - constNew->FirstPos = toPosId; - - // If not explicitly confirmed, nothing guarantees that a tangent can be freely - // transferred to another coincident point, as the transfer destination edge most likely - // won't be intended to be tangent. However, if it is an end to end point tangency, the - // user expects it to be substituted by a coincidence constraint. - if (vals[i]->Type == Sketcher::Tangent || vals[i]->Type == Sketcher::Perpendicular) { - if (!doNotTransformTangencies) { - constNew->Type = Sketcher::Coincident; - } - } - // With respect to angle constraints, if it is a DeepSOIC style angle constraint - // (segment+segment+point), then no problem arises as the segments are PosId=none. In - // this case there is no call to this function. - // - // However, other angle constraints are problematic because they are created on - // segments, but internally operate on vertices, PosId=start Such constraint may not be - // successfully transferred on deletion of the segments. - else if (vals[i]->Type == Sketcher::Angle) { - continue; - } - - Constraint* constPtr = constNew.release(); - newVals[i] = constPtr; - changed = true; + if (vals[i]->Type == Sketcher::InternalAlignment) { + // Transferring internal alignment constraint can cause malformed constraints. + // For example a B-spline pole being a point instead of a circle. + continue; } - else if (vals[i]->Second == fromGeoId && vals[i]->SecondPos == fromPosId - && !(vals[i]->First == toGeoId && vals[i]->FirstPos == toPosId) - && !(toGeoId < 0 && vals[i]->First < 0)) { - + else if (vals[i]->involvesGeoIdAndPosId(fromGeoId, fromPosId) + && !vals[i]->involvesGeoIdAndPosId(toGeoId, toPosId)) { std::unique_ptr constNew(newVals[i]->clone()); - constNew->Second = toGeoId; - constNew->SecondPos = toPosId; - // If not explicitly confirmed, nothing guarantees that a tangent can be freely - // transferred to another coincident point, as the transfer destination edge most likely - // won't be intended to be tangent. However, if it is an end to end point tangency, the - // user expects it to be substituted by a coincidence constraint. - if (vals[i]->Type == Sketcher::Tangent || vals[i]->Type == Sketcher::Perpendicular) { - if (!doNotTransformTangencies) { - constNew->Type = Sketcher::Coincident; - } - } - else if (vals[i]->Type == Sketcher::Angle) { + constNew->substituteIndexAndPos(fromGeoId, fromPosId, toGeoId, toPosId); + if (vals[i]->First < 0 && vals[i]->Second < 0) { + // TODO: Can `vals[i]->Third` be involved as well? + // If it is, we need to be sure at most ONE of these is external continue; } + switch (vals[i]->Type) { + case Sketcher::Tangent: + case Sketcher::Perpendicular: { + // If not explicitly confirmed, nothing guarantees that a tangent can be freely + // transferred to another coincident point, as the transfer destination edge + // most likely won't be intended to be tangent. However, if it is an end to end + // point tangency, the user expects it to be substituted by a coincidence + // constraint. + if (!doNotTransformTangencies) { + constNew->Type = Sketcher::Coincident; + } + } + case Sketcher::Angle: + // With respect to angle constraints, if it is a DeepSOIC style angle constraint + // (segment+segment+point), then no problem arises as the segments are + // PosId=none. In this case there is no call to this function. + // + // However, other angle constraints are problematic because they are created on + // segments, but internally operate on vertices, PosId=start Such constraint may + // not be successfully transferred on deletion of the segments. + continue; + default: + break; + } + Constraint* constPtr = constNew.release(); newVals[i] = constPtr; changed = true; @@ -2701,6 +2693,7 @@ int SketchObject::transferConstraints(int fromGeoId, PointPos fromPosId, int toG } return 0; } +// clang-format off int SketchObject::fillet(int GeoId, PointPos PosId, double radius, bool trim, bool createCorner, bool chamfer) {