From 0b812405a0e35e561ca29cded6710e44bc0fad66 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 7 Apr 2025 02:56:05 +0530 Subject: [PATCH 01/23] [Sketcher] Fix issue in `replaceGeometries` when more old than new --- src/Mod/Sketcher/App/SketchObject.cpp | 16 ++++++++-------- src/Mod/Sketcher/App/SketchObject.h | 12 ++++++------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 7d57db6bbf..3c8f4359b3 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -2021,6 +2021,7 @@ int SketchObject::delGeometriesExclusiveList(const std::vector& GeoIds, Del return 0; } +// clang-format on void SketchObject::replaceGeometries(std::vector oldGeoIds, std::vector& newGeos) { @@ -2042,18 +2043,17 @@ void SketchObject::replaceGeometries(std::vector oldGeoIds, newVals[*oldGeoIdIter] = *newGeoIter; } - if (newGeoIter != newGeos.end()) { - for (; newGeoIter != newGeos.end(); ++newGeoIter) { - generateId(*newGeoIter); - newVals.push_back(*newGeoIter); - } - } - else { - delGeometries(oldGeoIdIter, oldGeoIds.end()); + for (; newGeoIter != newGeos.end(); ++newGeoIter) { + generateId(*newGeoIter); + newVals.push_back(*newGeoIter); } + // Set geometries first, then delete the old ones. This allows to use `delGeometries`. Geometry.setValues(std::move(newVals)); + + delGeometries(oldGeoIdIter, oldGeoIds.end()); } +// clang-format off int SketchObject::deleteAllGeometry(DeleteOptions options) { diff --git a/src/Mod/Sketcher/App/SketchObject.h b/src/Mod/Sketcher/App/SketchObject.h index c318643c91..f42e398a38 100644 --- a/src/Mod/Sketcher/App/SketchObject.h +++ b/src/Mod/Sketcher/App/SketchObject.h @@ -984,6 +984,12 @@ public: // geometry extension functionalities for single element sketch object int setGeometryIds(std::vector> GeoIdsToIds); int getGeometryId(int GeoId, long& id) const; + /// 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); + protected: // Only the first flag is toggled, the rest of the flags is set or cleared following the first // flag. @@ -996,12 +1002,6 @@ 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 472cf2932a1533d013d71537c402dc221ebaaa1b Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 7 Apr 2025 02:57:47 +0530 Subject: [PATCH 02/23] [Sketcher][test] Add tests for `replaceGeometries()` --- .../Mod/Sketcher/App/SketchObjectChanges.cpp | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp b/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp index caef113ab2..7d777b1ba1 100644 --- a/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp +++ b/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp @@ -14,6 +14,73 @@ using namespace SketcherTestHelpers; +TEST_F(SketchObjectTest, testReplaceGeometriesOneToOne) +{ + // Arrange + Part::GeomLineSegment lineSeg; + setupLineSegment(lineSeg); + int geoId = getObject()->addGeometry(&lineSeg); + std::vector newCurves {createTypicalNonPeriodicBSpline().release()}; + + // Act + getObject()->replaceGeometries({geoId}, newCurves); + + // Assert + // Ensure geoId1 is now a B-Spline + auto* geo = getObject()->getGeometry(geoId); + EXPECT_TRUE(geo->is()); +} + +TEST_F(SketchObjectTest, testReplaceGeometriesTwoToOne) +{ + // Arrange + Part::GeomLineSegment lineSeg1, lineSeg2; + setupLineSegment(lineSeg1); + int geoId1 = getObject()->addGeometry(&lineSeg1); + setupLineSegment(lineSeg2); + int geoId2 = getObject()->addGeometry(&lineSeg2); + std::vector newCurves {createTypicalNonPeriodicBSpline().release()}; + + // Act + getObject()->replaceGeometries({geoId1, geoId2}, newCurves); + + // Assert + // Ensure only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 0); + // Ensure geoId1 is now a B-Spline + auto* geo = getObject()->getGeometry(geoId1); + EXPECT_TRUE(geo->is()); +} + +TEST_F(SketchObjectTest, testReplaceGeometriesOneToTwo) +{ + // Arrange + Part::GeomLineSegment lineSeg1; + setupLineSegment(lineSeg1); + int geoId1 = getObject()->addGeometry(&lineSeg1); + std::vector newCurves {createTypicalNonPeriodicBSpline().release(), + createTypicalNonPeriodicBSpline().release()}; + + // Act + getObject()->replaceGeometries({geoId1}, newCurves); + + // Assert + // Ensure only one curve + EXPECT_EQ(getObject()->getHighestCurveIndex(), 1); + // Ensure geoId1 is now a B-Spline + auto* geo = getObject()->getGeometry(geoId1); + EXPECT_TRUE(geo->is()); + geo = getObject()->getGeometry(geoId1 + 1); + EXPECT_TRUE(geo->is()); +} + +// TODO: formulate and add any constraint related changes when replacing geometries +// Currently, `replageGeometries` is a very "low level" operation that directly replaces the +// elements of the vector of geometries. Constraint handling is intended to be done by the +// operations that call this method. We may want to add tests that ensure constraints aren't +// touched. Alternatively, we may want to change `replaceGeometries` such that it modifies +// constraints, though that will limit our control in individual operations. + TEST_F(SketchObjectTest, testSplitLineSegment) { // Arrange From 287aeab832e6c43390e19b751e7bf6ee899e495f Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sat, 29 Jun 2024 15:52:21 +0530 Subject: [PATCH 03/23] [Sketcher] Refactor `SketchObject:validateExternalLinks()` --- src/Mod/Sketcher/App/SketchObject.cpp | 35 ++++++++++++--------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 3c8f4359b3..d9dcd09d57 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -8112,7 +8112,6 @@ void SketchObject::validateExternalLinks() const std::string SubElement = SubElements[i]; TopoDS_Shape refSubShape; - bool removeBadLink = false; try { if (Obj->isDerivedFrom()) { const Part::Datum* datum = static_cast(Obj); @@ -8126,38 +8125,36 @@ void SketchObject::validateExternalLinks() const Part::TopoShape& refShape = refObj->Shape.getShape(); refSubShape = refShape.getSubShape(SubElement.c_str()); } + continue; // no bad link needs to be removed } catch (Base::IndexError& indexError) { - removeBadLink = true; Base::Console().warning( this->getFullLabel(), (indexError.getMessage() + "\n").c_str()); } catch (Base::ValueError& valueError) { - removeBadLink = true; Base::Console().warning( this->getFullLabel(), (valueError.getMessage() + "\n").c_str()); } catch (Standard_Failure&) { - removeBadLink = true; } - if (removeBadLink) { - rebuild = true; - Objects.erase(Objects.begin() + i); - SubElements.erase(SubElements.begin() + i); - const std::vector& constraints = Constraints.getValues(); - std::vector newConstraints(0); - int GeoId = GeoEnum::RefExt - i; - for (const auto& constr : constraints) { - auto newConstr = getConstraintAfterDeletingGeo(constr, GeoId); - if (newConstr) { - newConstraints.push_back(newConstr.release()); - } + rebuild = true; + Objects.erase(Objects.begin() + i); + SubElements.erase(SubElements.begin() + i); + + const std::vector& constraints = Constraints.getValues(); + std::vector newConstraints; + newConstraints.reserve(constraints.size()); + int GeoId = GeoEnum::RefExt - i; + for (const auto& constr : constraints) { + auto newConstr = getConstraintAfterDeletingGeo(constr, GeoId); + if (newConstr) { + newConstraints.push_back(newConstr.release()); } - - Constraints.setValues(std::move(newConstraints)); - i--;// we deleted an item, so the next one took its place } + + Constraints.setValues(std::move(newConstraints)); + i--;// we deleted an item, so the next one took its place } if (rebuild) { From a23197c3e917fa920670caf306dd5f6d384d1ab6 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 1 Jul 2024 20:49:12 +0530 Subject: [PATCH 04/23] [Sketcher] Refactor some loops for understandability Changes include: 1. Modernize `for` loops with range whenever possible. 2. Flip `if` statements for a "flatter" flow. 3. Use internal functions for repeated identical tasks. --- src/Mod/Sketcher/App/SketchObject.cpp | 276 +++++++++++++------------- 1 file changed, 134 insertions(+), 142 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index d9dcd09d57..8b7cfd1cdd 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -2856,18 +2856,21 @@ int SketchObject::fillet(int GeoId, PointPos PosId, double radius, bool trim, bo GeoIdList = chooseFilletsEdges(GeoIdList); // only coincident points between two (non-external) edges can be filleted - if (GeoIdList.size() == 2 && GeoIdList[0] >= 0 && GeoIdList[1] >= 0) { - const Part::Geometry* geo1 = getGeometry(GeoIdList[0]); - const Part::Geometry* geo2 = getGeometry(GeoIdList[1]); - if (geo1->is() - && geo2->is()) { - auto* lineSeg1 = static_cast(geo1); - auto* lineSeg2 = static_cast(geo2); + if (GeoIdList.size() != 2 || GeoIdList[0] < 0 || GeoIdList[1] < 0) { + return -1; + } - Base::Vector3d midPnt1 = (lineSeg1->getStartPoint() + lineSeg1->getEndPoint()) / 2; - Base::Vector3d midPnt2 = (lineSeg2->getStartPoint() + lineSeg2->getEndPoint()) / 2; - return fillet(GeoIdList[0], GeoIdList[1], midPnt1, midPnt2, radius, trim, createCorner, chamfer); - } + const Part::Geometry* geo1 = getGeometry(GeoIdList[0]); + const Part::Geometry* geo2 = getGeometry(GeoIdList[1]); + + if (geo1->is() + && geo2->is()) { + auto* lineSeg1 = static_cast(geo1); + auto* lineSeg2 = static_cast(geo2); + + Base::Vector3d midPnt1 = (lineSeg1->getStartPoint() + lineSeg1->getEndPoint()) / 2; + Base::Vector3d midPnt2 = (lineSeg2->getStartPoint() + lineSeg2->getEndPoint()) / 2; + return fillet(GeoIdList[0], GeoIdList[1], midPnt1, midPnt2, radius, trim, createCorner, chamfer); } return -1; @@ -3880,17 +3883,17 @@ bool SketchObject::deriveConstraintsForPieces( break; } - if (transferToAll) { - for (auto& newId : newIds) { - Constraint* trans = con->copy(); - trans->substituteIndex(oldId, newId); - newConstraints.push_back(trans); - } - - return true; + if (!transferToAll) { + return false; } - return false; + for (auto& newId : newIds) { + Constraint* trans = con->copy(); + trans->substituteIndex(oldId, newId); + newConstraints.push_back(trans); + } + + return true; } int SketchObject::split(int GeoId, const Base::Vector3d& point) @@ -4227,18 +4230,19 @@ bool SketchObject::isExternalAllowed(App::Document* pDoc, App::DocumentObject* p bool SketchObject::isCarbonCopyAllowed(App::Document* pDoc, App::DocumentObject* pObj, bool& xinv, bool& yinv, eReasonList* rsn) const { - if (rsn) { - *rsn = rlAllowed; - } + auto setReason = [&rsn](eReasonList reasonFromList) { + if (rsn) + *rsn = reasonFromList; + }; + + setReason(rlAllowed); std::string sketchArchType ("Sketcher::SketchObjectPython"); // Only applicable to sketches if (!pObj->is() && sketchArchType != pObj->getTypeId().getName()) { - if (rsn) { - *rsn = rlNotASketch; - } + setReason(rlNotASketch); return false; } @@ -4247,18 +4251,14 @@ bool SketchObject::isCarbonCopyAllowed(App::Document* pDoc, App::DocumentObject* // Sketches outside of the Document are NOT allowed if (this->getDocument() != pDoc) { - if (rsn) { - *rsn = rlOtherDoc; - } + setReason(rlOtherDoc); return false; } // circular reference prevention try { if (!(this->testIfLinkDAGCompatible(pObj))) { - if (rsn) { - *rsn = rlCircularReference; - } + setReason(rlCircularReference); return false; } } @@ -4275,31 +4275,25 @@ bool SketchObject::isCarbonCopyAllowed(App::Document* pDoc, App::DocumentObject* Part::BodyBase* body_obj = Part::BodyBase::findBodyOf(pObj); App::Part* part_this = App::Part::getPartOfObject(this); App::Part* part_obj = App::Part::getPartOfObject(pObj); - if (part_this == part_obj) {// either in the same part, or in the root of document - if (body_this) { - if (body_this != body_obj) { - if (!this->allowOtherBody) { - if (rsn) - *rsn = rlOtherBody; - return false; - } - // if the original sketch has external geometry AND it is not in this body prevent - // link - else if (psObj->getExternalGeometryCount() > 2) { - if (rsn) - *rsn = rlOtherBodyWithLinks; - return false; - } - } - } - } - else { + if (part_this != part_obj) { // cross-part relation. Disallow, should be done via shapebinders only - if (rsn) - *rsn = rlOtherPart; + setReason(rlOtherPart); return false; } + // Hereafter assuming: either in the same part, or in the root of document + if (body_this && body_this != body_obj) { + if (!this->allowOtherBody) { + setReason(rlOtherBody); + return false; + } + // if the original sketch has external geometry AND it is not in this body prevent + // link + else if (psObj->getExternalGeometryCount() > 2) { + setReason(rlOtherBodyWithLinks); + return false; + } + } const Rotation& srot = psObj->Placement.getValue().getRotation(); const Rotation& lrot = this->Placement.getValue().getRotation(); @@ -4324,8 +4318,7 @@ bool SketchObject::isCarbonCopyAllowed(App::Document* pDoc, App::DocumentObject* // the planes of the sketches must be parallel if (!allowUnaligned && fabs(fabs(dot) - 1) > Precision::Confusion()) { - if (rsn) - *rsn = rlNonParallel; + setReason(rlNonParallel); return false; } @@ -4333,8 +4326,7 @@ bool SketchObject::isCarbonCopyAllowed(App::Document* pDoc, App::DocumentObject* if (!allowUnaligned && ((fabs(fabs(dotx) - 1) > Precision::Confusion()) || (fabs(fabs(doty) - 1) > Precision::Confusion()))) { - if (rsn) - *rsn = rlAxesMisaligned; + setReason(rlAxesMisaligned); return false; } @@ -4349,8 +4341,7 @@ bool SketchObject::isCarbonCopyAllowed(App::Document* pDoc, App::DocumentObject* if (!allowUnaligned && (fabs(fabs(alignment) - 1) > Precision::Confusion()) && (psObj->Placement.getValue().getPosition() != this->Placement.getValue().getPosition())) { - if (rsn) - *rsn = rlOriginsMisaligned; + setReason(rlOriginsMisaligned); return false; } @@ -5517,43 +5508,45 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) std::vector> changeConstraintIndices; for (size_t i = 0; i < constrvals.size(); i++) { - for (auto geoid : geoIdList) { - if (constrvals[i]->First == geoid || constrvals[i]->Second == geoid - || constrvals[i]->Third == geoid) { - switch (constrvals[i]->Type) { - case Sketcher::Horizontal: - if (constrvals[i]->FirstPos == Sketcher::PointPos::none - && constrvals[i]->SecondPos == Sketcher::PointPos::none) { - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - nhoriz++; - } - break; - case Sketcher::Vertical: - if (constrvals[i]->FirstPos == Sketcher::PointPos::none - && constrvals[i]->SecondPos == Sketcher::PointPos::none) { - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - nvert++; - } - break; - case Sketcher::Symmetric:// only remove symmetric to axes - if ((constrvals[i]->Third == GeoEnum::HAxis - || constrvals[i]->Third == GeoEnum::VAxis) - && constrvals[i]->ThirdPos == Sketcher::PointPos::none) - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - break; - case Sketcher::PointOnObject: - if ((constrvals[i]->Second == GeoEnum::HAxis - || constrvals[i]->Second == GeoEnum::VAxis) - && constrvals[i]->SecondPos == Sketcher::PointPos::none) - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - break; - case Sketcher::DistanceX: - case Sketcher::DistanceY: - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - break; - default: - break; + for (const auto& geoid : geoIdList) { + if (constrvals[i]->First != geoid + && constrvals[i]->Second != geoid + && constrvals[i]->Third != geoid) { + continue; + } + switch (constrvals[i]->Type) { + case Sketcher::Horizontal: + if (constrvals[i]->FirstPos == Sketcher::PointPos::none + && constrvals[i]->SecondPos == Sketcher::PointPos::none) { + changeConstraintIndices.emplace_back(i, constrvals[i]->Type); + nhoriz++; } + break; + case Sketcher::Vertical: + if (constrvals[i]->FirstPos == Sketcher::PointPos::none + && constrvals[i]->SecondPos == Sketcher::PointPos::none) { + changeConstraintIndices.emplace_back(i, constrvals[i]->Type); + nvert++; + } + break; + case Sketcher::Symmetric:// only remove symmetric to axes + if ((constrvals[i]->Third == GeoEnum::HAxis + || constrvals[i]->Third == GeoEnum::VAxis) + && constrvals[i]->ThirdPos == Sketcher::PointPos::none) + changeConstraintIndices.emplace_back(i, constrvals[i]->Type); + break; + case Sketcher::PointOnObject: + if ((constrvals[i]->Second == GeoEnum::HAxis + || constrvals[i]->Second == GeoEnum::VAxis) + && constrvals[i]->SecondPos == Sketcher::PointPos::none) + changeConstraintIndices.emplace_back(i, constrvals[i]->Type); + break; + case Sketcher::DistanceX: + case Sketcher::DistanceY: + changeConstraintIndices.emplace_back(i, constrvals[i]->Type); + break; + default: + break; } } } @@ -5569,55 +5562,54 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) int cindex = 0; for (size_t i = 0; i < constrvals.size(); i++) { - if (i == changeConstraintIndices[cindex].first) { - if (changeConstraintIndices[cindex].second == Sketcher::Horizontal && nhoriz > 0) { - changed = true; - if (referenceHorizontal == GeoEnum::GeoUndef) { - referenceHorizontal = constrvals[i]->First; - } - else { - - auto newConstr = new Constraint(); - - newConstr->Type = Sketcher::Parallel; - newConstr->First = referenceHorizontal; - newConstr->Second = constrvals[i]->First; - - newconstrVals.push_back(newConstr); - } - } - else if (changeConstraintIndices[cindex].second == Sketcher::Vertical && nvert > 0) { - changed = true; - if (referenceVertical == GeoEnum::GeoUndef) { - referenceVertical = constrvals[i]->First; - ; - } - else { - auto newConstr = new Constraint(); - - newConstr->Type = Sketcher::Parallel; - newConstr->First = referenceVertical; - newConstr->Second = constrvals[i]->First; - - newconstrVals.push_back(newConstr); - } - } - else if (changeConstraintIndices[cindex].second == Sketcher::Symmetric - || changeConstraintIndices[cindex].second == Sketcher::PointOnObject) { - changed = true;// We remove symmetric on axes - } - else if (changeConstraintIndices[cindex].second == Sketcher::DistanceX - || changeConstraintIndices[cindex].second == Sketcher::DistanceY) { - changed = true;// We remove symmetric on axes - newconstrVals.push_back(constrvals[i]->clone()); - newconstrVals.back()->Type = Sketcher::Distance; - } - - cindex++; - } - else { + if (i != changeConstraintIndices[cindex].first) { newconstrVals.push_back(constrvals[i]); + continue; } + + if (changeConstraintIndices[cindex].second == Sketcher::Horizontal && nhoriz > 0) { + changed = true; + if (referenceHorizontal == GeoEnum::GeoUndef) { + referenceHorizontal = constrvals[i]->First; + } + else { + + auto newConstr = new Constraint(); + + newConstr->Type = Sketcher::Parallel; + newConstr->First = referenceHorizontal; + newConstr->Second = constrvals[i]->First; + + newconstrVals.push_back(newConstr); + } + } + else if (changeConstraintIndices[cindex].second == Sketcher::Vertical && nvert > 0) { + changed = true; + if (referenceVertical == GeoEnum::GeoUndef) { + referenceVertical = constrvals[i]->First; + } + else { + auto newConstr = new Constraint(); + + newConstr->Type = Sketcher::Parallel; + newConstr->First = referenceVertical; + newConstr->Second = constrvals[i]->First; + + newconstrVals.push_back(newConstr); + } + } + else if (changeConstraintIndices[cindex].second == Sketcher::Symmetric + || changeConstraintIndices[cindex].second == Sketcher::PointOnObject) { + changed = true;// We remove symmetric on axes + } + else if (changeConstraintIndices[cindex].second == Sketcher::DistanceX + || changeConstraintIndices[cindex].second == Sketcher::DistanceY) { + changed = true;// We remove symmetric on axes + newconstrVals.push_back(constrvals[i]->clone()); + newconstrVals.back()->Type = Sketcher::Distance; + } + + cindex++; } if (nhoriz > 0 && nvert > 0) { From 45021f204a39e57afac4f0adc430978ff828c84f Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sat, 27 Jul 2024 22:06:10 +0530 Subject: [PATCH 05/23] [Sketcher] Refactor based on loops and extraction into functions May contain some untested changes --- src/Mod/Sketcher/App/SketchObject.cpp | 76 +++++++++------------------ 1 file changed, 24 insertions(+), 52 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 8b7cfd1cdd..a5a28bc8c3 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -2591,12 +2591,13 @@ void SketchObject::transferFilletConstraints(int geoId1, PointPos posId1, int ge std::vector deleteme; for (int i = 0; i < int(constraints.size()); i++) { const Constraint* c = constraints[i]; - if (c->Type == Sketcher::Distance || c->Type == Sketcher::Equal) { - bool line1 = c->First == geoId1 && c->FirstPos == PointPos::none; - bool line2 = c->First == geoId2 && c->FirstPos == PointPos::none; - if (line1 || line2) { - deleteme.push_back(i); - } + if (c->Type != Sketcher::Distance && c->Type != Sketcher::Equal) { + continue; + } + bool line1 = c->First == geoId1 && c->FirstPos == PointPos::none; + bool line2 = c->First == geoId2 && c->FirstPos == PointPos::none; + if (line1 || line2) { + deleteme.push_back(i); } } delConstraints(std::move(deleteme), DeleteOption::NoFlag); @@ -2607,8 +2608,7 @@ void SketchObject::transferFilletConstraints(int geoId1, PointPos posId1, int ge // TODO: Add support for curved lines. const Part::Geometry* geo1 = getGeometry(geoId1); const Part::Geometry* geo2 = getGeometry(geoId2); - if (!geo1->is() - || !geo2->is()) { + if (!geo1->is() || !geo2->is()) { delConstraintOnPoint(geoId1, posId1, false); delConstraintOnPoint(geoId2, posId2, false); return; @@ -2661,39 +2661,13 @@ void SketchObject::transferFilletConstraints(int geoId1, PointPos posId1, int ge // instead. continue; } - if (point1First || point2First) { - // Move the coincident constraint to the new corner point - c->First = originalCornerId; - c->FirstPos = PointPos::start; - } - if (point1Second || point2Second) { - // Move the coincident constraint to the new corner point - c->Second = originalCornerId; - c->SecondPos = PointPos::start; - } } else if (c->Type == Sketcher::Horizontal || c->Type == Sketcher::Vertical) { - // Point-to-point horizontal or vertical constraint, move to new corner point - if (point1First || point2First) { - c->First = originalCornerId; - c->FirstPos = PointPos::start; - } - if (point1Second || point2Second) { - c->Second = originalCornerId; - c->SecondPos = PointPos::start; - } + // Point-to-point horizontal or vertical constraint, move to new corner point (done towards end of present loop) } else if (c->Type == Sketcher::Distance || c->Type == Sketcher::DistanceX || c->Type == Sketcher::DistanceY) { - // Point-to-point distance constraint. Move it to the new corner point - if (point1First || point2First) { - c->First = originalCornerId; - c->FirstPos = PointPos::start; - } - if (point1Second || point2Second) { - c->Second = originalCornerId; - c->SecondPos = PointPos::start; - } + // Point-to-point distance constraint. Move it to the new corner point (done towards end of present loop) // Distance constraint on the line itself. Change it to point-point between the far end // of the line and the new corner @@ -2710,10 +2684,6 @@ void SketchObject::transferFilletConstraints(int geoId1, PointPos posId1, int ge } else if (c->Type == Sketcher::PointOnObject) { // The corner to be filleted was touching some other object. - if (point1First || point2First) { - c->First = originalCornerId; - c->FirstPos = PointPos::start; - } } else if (c->Type == Sketcher::Equal) { // Equal length constraints are dicey because the lines are getting shorter. Safer to @@ -2724,18 +2694,6 @@ void SketchObject::transferFilletConstraints(int geoId1, PointPos posId1, int ge } else if (c->Type == Sketcher::Symmetric) { // Symmetries should probably be preserved relative to the original corner - if (point1First || point2First) { - c->First = originalCornerId; - c->FirstPos = PointPos::start; - } - else if (point1Second || point2Second) { - c->Second = originalCornerId; - c->SecondPos = PointPos::start; - } - else if (point1Third || point2Third) { - c->Third = originalCornerId; - c->ThirdPos = PointPos::start; - } } else if (c->Type == Sketcher::SnellsLaw) { // Can't imagine any cases where you'd fillet a vertex going through a lens, so let's @@ -2748,6 +2706,20 @@ void SketchObject::transferFilletConstraints(int geoId1, PointPos posId1, int ge continue; } + // For any constraint not passing previous conditions, transfer to the new point if relevant + if (point1First || point2First) { + c->First = originalCornerId; + c->FirstPos = PointPos::start; + } + else if (point1Second || point2Second) { + c->Second = originalCornerId; + c->SecondPos = PointPos::start; + } + else if (point1Third || point2Third) { + c->Third = originalCornerId; + c->ThirdPos = PointPos::start; + } + // Default: keep all other constraints newConstraints.push_back(c->clone()); } From 6a3bbc195db60e162395cdfa028a8bb73c416aaf Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 28 Jul 2024 14:31:05 +0530 Subject: [PATCH 06/23] [Sketcher] Refactor `SketchObject` some more --- src/Mod/Sketcher/App/SketchObject.cpp | 282 ++++++++++++++------------ 1 file changed, 147 insertions(+), 135 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index a5a28bc8c3..9dd8434739 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -4345,7 +4345,7 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, else { for (auto* constr : constrvals) { if (constr->First == refGeoId - && (constr->Type == Sketcher::Vertical || constr->Type == Sketcher::Horizontal)){ + && (constr->Type == Sketcher::Vertical || constr->Type == Sketcher::Horizontal)) { refIsAxisAligned = true; } } @@ -4358,117 +4358,127 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, for (auto* constr : constrvals) { // we look in the map, because we might have skipped internal alignment geometry - auto fit = geoIdMap.find(constr->First); + auto firstIt = geoIdMap.find(constr->First); - if (fit != geoIdMap.end()) {// if First of constraint is in geoIdList - if (addSymmetryConstraints && constr->Type != Sketcher::InternalAlignment) { - // if we are making symmetric constraints, then we don't want to copy all constraints + if (firstIt == geoIdMap.end()) { + continue; + } + + // if First of constraint is in geoIdList + if (addSymmetryConstraints && constr->Type != Sketcher::InternalAlignment) { + // if we are making symmetric constraints, then we don't want to copy all constraints + continue; + } + + if (constr->Second == GeoEnum::GeoUndef /*&& constr->Third == GeoEnum::GeoUndef*/) { + if (refIsAxisAligned + && (constr->Type == Sketcher::DistanceX + || constr->Type == Sketcher::DistanceY)) { + // In this case we want to keep the Vertical, Horizontal constraints. + // DistanceX and DistanceY constraints should also be possible to keep in + // this case, but keeping them causes segfault, not sure why. + + continue; + } + if (!refIsAxisAligned + && (constr->Type == Sketcher::DistanceX + || constr->Type == Sketcher::DistanceY + || constr->Type == Sketcher::Vertical + || constr->Type == Sketcher::Horizontal)) { + // this includes all non-directional single GeoId constraints, as radius, + // diameter, weight,... + continue; + } + Constraint* constNew = constr->copy(); + constNew->Name = ""; + constNew->First = firstIt->second; + newconstrVals.push_back(constNew); + + continue; + } + + // other geoids intervene in this constraint + auto secondIt = geoIdMap.find(constr->Second); + + if (secondIt == geoIdMap.end()) { + continue; + } + + // Second is also in the list + if (constr->Third == GeoEnum::GeoUndef) { + if (!(constr->Type == Sketcher::Coincident + || constr->Type == Sketcher::Perpendicular + || constr->Type == Sketcher::Parallel + || constr->Type == Sketcher::Tangent + || constr->Type == Sketcher::Distance + || constr->Type == Sketcher::Equal + || constr->Type == Sketcher::Angle + || constr->Type == Sketcher::PointOnObject + || constr->Type == Sketcher::InternalAlignment)) { continue; } - if (constr->Second == GeoEnum::GeoUndef ){ - if (refIsAxisAligned) { - // in this case we want to keep the Vertical, Horizontal constraints - // DistanceX ,and DistanceY constraints should also be possible to keep in - // this case, but keeping them causes segfault, not sure why. - - if (constr->Type != Sketcher::DistanceX - && constr->Type != Sketcher::DistanceY) { - Constraint* constNew = constr->copy(); - constNew->Name = ""; // Make sure we don't have 2 constraint with same name. - constNew->First = fit->second; - newconstrVals.push_back(constNew); - } - } - else if (constr->Type != Sketcher::DistanceX - && constr->Type != Sketcher::DistanceY - && constr->Type != Sketcher::Vertical - && constr->Type != Sketcher::Horizontal) { - // this includes all non-directional single GeoId constraints, as radius, - // diameter, weight,... - - Constraint* constNew = constr->copy(); - constNew->Name = ""; - constNew->First = fit->second; - newconstrVals.push_back(constNew); - } + Constraint* constNew = constr->copy(); + constNew->Name = ""; + constNew->First = firstIt->second; + constNew->Second = secondIt->second; + if (isStartEndInverted[constr->First]) { + if (constr->FirstPos == Sketcher::PointPos::start) + constNew->FirstPos = Sketcher::PointPos::end; + else if (constr->FirstPos == Sketcher::PointPos::end) + constNew->FirstPos = Sketcher::PointPos::start; } - else {// other geoids intervene in this constraint - - auto sit = geoIdMap.find(constr->Second); - - if (sit != geoIdMap.end()) {// Second is also in the list - - if (constr->Third == GeoEnum::GeoUndef) { - if (constr->Type == Sketcher::Coincident - || constr->Type == Sketcher::Perpendicular - || constr->Type == Sketcher::Parallel - || constr->Type == Sketcher::Tangent - || constr->Type == Sketcher::Distance - || constr->Type == Sketcher::Equal || constr->Type == Sketcher::Angle - || constr->Type == Sketcher::PointOnObject - || constr->Type == Sketcher::InternalAlignment) { - Constraint* constNew = constr->copy(); - constNew->Name = ""; - constNew->First = fit->second; - constNew->Second = sit->second; - if (isStartEndInverted[constr->First]) { - if (constr->FirstPos == Sketcher::PointPos::start) - constNew->FirstPos = Sketcher::PointPos::end; - else if (constr->FirstPos == Sketcher::PointPos::end) - constNew->FirstPos = Sketcher::PointPos::start; - } - if (isStartEndInverted[constr->Second]) { - if (constr->SecondPos == Sketcher::PointPos::start) - constNew->SecondPos = Sketcher::PointPos::end; - else if (constr->SecondPos == Sketcher::PointPos::end) - constNew->SecondPos = Sketcher::PointPos::start; - } - - if (constNew->Type == Tangent || constNew->Type == Perpendicular) - AutoLockTangencyAndPerpty(constNew, true); - - if ((constr->Type == Sketcher::Angle) - && (refPosId == Sketcher::PointPos::none)) { - constNew->setValue(-constr->getValue()); - } - - newconstrVals.push_back(constNew); - } - } - else {// three GeoIds intervene in constraint - auto tit = geoIdMap.find(constr->Third); - - if (tit != geoIdMap.end()) {// Third is also in the list - Constraint* constNew = constr->copy(); - constNew->Name = ""; - constNew->First = fit->second; - constNew->Second = sit->second; - constNew->Third = tit->second; - if (isStartEndInverted[constr->First]) { - if (constr->FirstPos == Sketcher::PointPos::start) - constNew->FirstPos = Sketcher::PointPos::end; - else if (constr->FirstPos == Sketcher::PointPos::end) - constNew->FirstPos = Sketcher::PointPos::start; - } - if (isStartEndInverted[constr->Second]) { - if (constr->SecondPos == Sketcher::PointPos::start) - constNew->SecondPos = Sketcher::PointPos::end; - else if (constr->SecondPos == Sketcher::PointPos::end) - constNew->SecondPos = Sketcher::PointPos::start; - } - if (isStartEndInverted[constr->Third]) { - if (constr->ThirdPos == Sketcher::PointPos::start) - constNew->ThirdPos = Sketcher::PointPos::end; - else if (constr->ThirdPos == Sketcher::PointPos::end) - constNew->ThirdPos = Sketcher::PointPos::start; - } - newconstrVals.push_back(constNew); - } - } - } + if (isStartEndInverted[constr->Second]) { + if (constr->SecondPos == Sketcher::PointPos::start) + constNew->SecondPos = Sketcher::PointPos::end; + else if (constr->SecondPos == Sketcher::PointPos::end) + constNew->SecondPos = Sketcher::PointPos::start; } + + if (constNew->Type == Tangent || constNew->Type == Perpendicular) + AutoLockTangencyAndPerpty(constNew, true); + + if ((constr->Type == Sketcher::Angle) + && (refPosId == Sketcher::PointPos::none)) { + constNew->setValue(-constr->getValue()); + } + + newconstrVals.push_back(constNew); + continue; } + + // three GeoIds intervene in constraint + auto thirdIt = geoIdMap.find(constr->Third); + + if (thirdIt == geoIdMap.end()) { + continue; + } + + // Third is also in the list + Constraint* constNew = constr->copy(); + constNew->Name = ""; + constNew->First = firstIt->second; + constNew->Second = secondIt->second; + constNew->Third = thirdIt->second; + if (isStartEndInverted[constr->First]) { + if (constr->FirstPos == Sketcher::PointPos::start) + constNew->FirstPos = Sketcher::PointPos::end; + else if (constr->FirstPos == Sketcher::PointPos::end) + constNew->FirstPos = Sketcher::PointPos::start; + } + if (isStartEndInverted[constr->Second]) { + if (constr->SecondPos == Sketcher::PointPos::start) + constNew->SecondPos = Sketcher::PointPos::end; + else if (constr->SecondPos == Sketcher::PointPos::end) + constNew->SecondPos = Sketcher::PointPos::start; + } + if (isStartEndInverted[constr->Third]) { + if (constr->ThirdPos == Sketcher::PointPos::start) + constNew->ThirdPos = Sketcher::PointPos::end; + else if (constr->ThirdPos == Sketcher::PointPos::end) + constNew->ThirdPos = Sketcher::PointPos::start; + } + newconstrVals.push_back(constNew); } if (addSymmetryConstraints) { @@ -4555,20 +4565,21 @@ std::vector SketchObject::getSymmetric(const std::vector& auto shouldCopyGeometry = [&](auto* geo, int geoId) -> bool { auto gf = GeometryFacade::getFacade(geo); - if (gf->isInternalAligned()) { - // only add if the corresponding geometry it defines is also in the list. - int definedGeo = GeoEnum::GeoUndef; - for (auto c : Constraints.getValues()) { - if (c->Type == Sketcher::InternalAlignment && c->First == geoId) { - definedGeo = c->Second; - break; - } - } - // Return true if definedGeo is in geoIdList, false otherwise - return std::ranges::find(geoIdList, definedGeo) != geoIdList.end(); + if (!gf->isInternalAligned()) { + // Return true if not internal aligned, indicating it should always be copied + return true; } - // Return true if not internal aligned, indicating it should always be copied - return true; + + // only add if the corresponding geometry it defines is also in the list. + int definedGeo = GeoEnum::GeoUndef; + for (auto c : Constraints.getValues()) { + if (c->Type == Sketcher::InternalAlignment && c->First == geoId) { + definedGeo = c->Second; + break; + } + } + // Return true if definedGeo is in geoIdList, false otherwise + return std::ranges::find(geoIdList, definedGeo) != geoIdList.end(); }; if (refIsLine) { @@ -5543,32 +5554,31 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) changed = true; if (referenceHorizontal == GeoEnum::GeoUndef) { referenceHorizontal = constrvals[i]->First; + ++cindex; + continue; } - else { + auto newConstr = new Constraint(); - auto newConstr = new Constraint(); + newConstr->Type = Sketcher::Parallel; + newConstr->First = referenceHorizontal; + newConstr->Second = constrvals[i]->First; - newConstr->Type = Sketcher::Parallel; - newConstr->First = referenceHorizontal; - newConstr->Second = constrvals[i]->First; - - newconstrVals.push_back(newConstr); - } + newconstrVals.push_back(newConstr); } else if (changeConstraintIndices[cindex].second == Sketcher::Vertical && nvert > 0) { changed = true; if (referenceVertical == GeoEnum::GeoUndef) { referenceVertical = constrvals[i]->First; + ++cindex; + continue; } - else { - auto newConstr = new Constraint(); + auto newConstr = new Constraint(); - newConstr->Type = Sketcher::Parallel; - newConstr->First = referenceVertical; - newConstr->Second = constrvals[i]->First; + newConstr->Type = Sketcher::Parallel; + newConstr->First = referenceVertical; + newConstr->Second = constrvals[i]->First; - newconstrVals.push_back(newConstr); - } + newconstrVals.push_back(newConstr); } else if (changeConstraintIndices[cindex].second == Sketcher::Symmetric || changeConstraintIndices[cindex].second == Sketcher::PointOnObject) { @@ -5577,11 +5587,12 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) else if (changeConstraintIndices[cindex].second == Sketcher::DistanceX || changeConstraintIndices[cindex].second == Sketcher::DistanceY) { changed = true;// We remove symmetric on axes + // TODO: Handle pathological cases like DistanceY on horizontal constraint newconstrVals.push_back(constrvals[i]->clone()); newconstrVals.back()->Type = Sketcher::Distance; } - cindex++; + ++cindex; } if (nhoriz > 0 && nvert > 0) { @@ -5594,8 +5605,9 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) newconstrVals.push_back(newConstr); } - if (changed) + if (changed) { Constraints.setValues(std::move(newconstrVals)); + } return 0; } From f008424ef5d6b176bdf2b1c6247f22509e00d4e1 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Fri, 2 Aug 2024 23:46:36 +0530 Subject: [PATCH 07/23] [Sketcher] Refactor `SketchObject::addCopy()` if and for rearrangement for readability. --- src/Mod/Sketcher/App/SketchObject.cpp | 935 +++++++++++++------------- 1 file changed, 480 insertions(+), 455 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 9dd8434739..7bd12caf30 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -4939,9 +4939,15 @@ std::vector SketchObject::getSymmetric(const std::vector& return symmetricVals; } -int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3d& displacement, - bool moveonly , bool clone , int csize , int rsize , bool constraindisplacement , - double perpscale ) +// clang-format on +int SketchObject::addCopy(const std::vector& geoIdList, + const Base::Vector3d& displacement, + bool moveonly, + bool clone, + int csize, + int rsize, + bool constraindisplacement, + double perpscale) { // no need to check input data validity as this is an sketchobject managed operation. Base::StateLocker lock(managedoperation, true); @@ -4958,9 +4964,10 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 std::vector newgeoIdList(geoIdList); - if (newgeoIdList.empty()) {// default option to operate on all the geometry - for (int i = 0; i < int(geovals.size()); i++) + if (newgeoIdList.empty()) { // default option to operate on all the geometry + for (int i = 0; i < int(geovals.size()); i++) { newgeoIdList.push_back(i); + } } int cgeoid = getHighestCurveIndex() + 1; @@ -4984,478 +4991,494 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 int x, y; - for (y = 0; y < rsize; y++) { - for (x = 0; x < csize; x++) { - // the reference for constraining array elements is the first valid point of the first - // element - if (x == 0 && y == 0) { - const Part::Geometry* geo = getGeometry(*(newgeoIdList.begin())); + auto makeCopyAtRowColumn = [&](int x, int y) { + // the reference for constraining array elements is the first valid point of the first + // element + if (x == 0 && y == 0) { + const Part::Geometry* geo = getGeometry(*(newgeoIdList.begin())); - auto gf = GeometryFacade::getFacade(geo); + auto gf = GeometryFacade::getFacade(geo); - if (gf->isInternalAligned() && !moveonly) { - // only add this geometry if the corresponding geometry it defines is also in - // the list. - int definedGeo = GeoEnum::GeoUndef; + if (gf->isInternalAligned() && !moveonly) { + // only add this geometry if the corresponding geometry it defines is also in + // the list. - for (auto c : Constraints.getValues()) { - if (c->Type == Sketcher::InternalAlignment - && c->First == *(newgeoIdList.begin())) { - definedGeo = c->Second; - break; - } - } + auto constrIt = std::ranges::find_if(constrvals, [&newgeoIdList](auto c) { + return (c->Type == Sketcher::InternalAlignment + && c->First == *(newgeoIdList.begin())); + }); - if (std::ranges::find(newgeoIdList, definedGeo) == newgeoIdList.end()) { - // the first element setting the reference is an internal alignment - // geometry, wherein the geometry it defines is not part of the copy - // operation. - THROWM(Base::ValueError, - "A move/copy/array operation on an internal alignment geometry is " - "only possible together with the geometry it defines.") - } + int definedGeo = + (constrIt != constrvals.end()) ? (*constrIt)->Second : GeoEnum::GeoUndef; + + if (std::ranges::find(newgeoIdList, definedGeo) == newgeoIdList.end()) { + // the first element setting the reference is an internal alignment + // geometry, wherein the geometry it defines is not part of the copy + // operation. + THROWM(Base::ValueError, + "A move/copy/array operation on an internal alignment geometry is " + "only possible together with the geometry it defines."); } + } - refgeoid = *(newgeoIdList.begin()); - currentrowfirstgeoid = refgeoid; - iterfirstgeoid = refgeoid; - if (geo->is() - || geo->is()) { - refposId = Sketcher::PointPos::mid; - } - else - refposId = Sketcher::PointPos::start; - - continue;// the first element is already in place + refgeoid = *(newgeoIdList.begin()); + currentrowfirstgeoid = refgeoid; + iterfirstgeoid = refgeoid; + if (geo->is() || geo->is()) { + refposId = Sketcher::PointPos::mid; } else { - prevfirstgeoid = iterfirstgeoid; - - iterfirstgeoid = cgeoid; - - if (x == 0) {// if first element of second row - prevrowstartfirstgeoid = currentrowfirstgeoid; - currentrowfirstgeoid = cgeoid; - } + refposId = Sketcher::PointPos::start; } - int index = 0; - for (std::vector::const_iterator it = newgeoIdList.begin(); - it != newgeoIdList.end(); - ++it, index++) { - const Part::Geometry* geo = getGeometry(*it); + return; // the first element is already in place + } - Part::Geometry* geocopy; + prevfirstgeoid = iterfirstgeoid; - auto gf = GeometryFacade::getFacade(geo); + iterfirstgeoid = cgeoid; - if (gf->isInternalAligned() && !moveonly) { - // only add this geometry if the corresponding geometry it defines is also in - // the list. - int definedGeo = GeoEnum::GeoUndef; + if (x == 0) { // if first element of second row + prevrowstartfirstgeoid = currentrowfirstgeoid; + currentrowfirstgeoid = cgeoid; + } - for (auto c : Constraints.getValues()) { - if (c->Type == Sketcher::InternalAlignment && c->First == *it) { - definedGeo = c->Second; - break; - } - } + int index = 0; + for (auto it = newgeoIdList.cbegin(); it != newgeoIdList.cend(); ++it, ++index) { + const Part::Geometry* geo = getGeometry(*it); - if (std::ranges::find(newgeoIdList, definedGeo) - == newgeoIdList.end()) { - // we should not copy internal alignment geometry, unless the element they - // define is also mirrored - continue; - } + Part::Geometry* geocopy; + + auto gf = GeometryFacade::getFacade(geo); + + if (gf->isInternalAligned() && !moveonly) { + // only add this geometry if the corresponding geometry it defines is also in + // the list. + int definedGeo = GeoEnum::GeoUndef; + + auto constrIt = std::ranges::find_if(constrvals, [&it](auto c) { + return (c->Type == Sketcher::InternalAlignment && c->First == *it); + }); + if (constrIt != constrvals.end()) { + definedGeo = (*constrIt)->Second; } - // We have already cloned all geometry and constraints, we only need a copy if not - // moving - if (!moveonly) { - geocopy = geo->copy(); - generateId(geocopy); - } else - geocopy = newgeoVals[*it]; - - // Handle Geometry - if (geocopy->is()) { - Part::GeomLineSegment* geosymline = - static_cast(geocopy); - Base::Vector3d ep = geosymline->getEndPoint(); - Base::Vector3d ssp = geosymline->getStartPoint() + double(x) * displacement - + double(y) * perpendicularDisplacement; - - geosymline->setPoints( - ssp, ep + double(x) * displacement + double(y) * perpendicularDisplacement); - - if (it == newgeoIdList.begin()) - iterfirstpoint = ssp; - } - else if (geocopy->is()) { - auto* geosymcircle = static_cast(geocopy); - Base::Vector3d cp = geosymcircle->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; - - geosymcircle->setCenter(scp); - - if (it == newgeoIdList.begin()) - iterfirstpoint = scp; - } - else if (geocopy->is()) { - auto* geoaoc = static_cast(geocopy); - Base::Vector3d cp = geoaoc->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; - - geoaoc->setCenter(scp); - - if (it == newgeoIdList.begin()) - iterfirstpoint = geoaoc->getStartPoint(true); - } - else if (geocopy->is()) { - auto* geosymellipse = static_cast(geocopy); - Base::Vector3d cp = geosymellipse->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; - - geosymellipse->setCenter(scp); - - if (it == newgeoIdList.begin()) - iterfirstpoint = scp; - } - else if (geocopy->is()) { - auto* geoaoe = static_cast(geocopy); - Base::Vector3d cp = geoaoe->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; - - geoaoe->setCenter(scp); - - if (it == newgeoIdList.begin()) - iterfirstpoint = geoaoe->getStartPoint(true); - } - else if (geocopy->is()) { - Part::GeomArcOfHyperbola* geoaoe = - static_cast(geocopy); - Base::Vector3d cp = geoaoe->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; - - geoaoe->setCenter(scp); - - if (it == newgeoIdList.begin()) - iterfirstpoint = geoaoe->getStartPoint(true); - } - else if (geocopy->is()) { - Part::GeomArcOfParabola* geoaoe = - static_cast(geocopy); - Base::Vector3d cp = geoaoe->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; - - geoaoe->setCenter(scp); - - if (it == newgeoIdList.begin()) - iterfirstpoint = geoaoe->getStartPoint(true); - } - else if (geocopy->is()) { - auto* geobsp = static_cast(geocopy); - - std::vector poles = geobsp->getPoles(); - - for (std::vector::iterator jt = poles.begin(); - jt != poles.end(); - ++jt) { - - (*jt) = (*jt) + double(x) * displacement - + double(y) * perpendicularDisplacement; - } - - geobsp->setPoles(poles); - - if (it == newgeoIdList.begin()) - iterfirstpoint = geobsp->getStartPoint(); - } - else if (geocopy->is()) { - auto* geopoint = static_cast(geocopy); - Base::Vector3d cp = geopoint->getPoint(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; - geopoint->setPoint(scp); - - if (it == newgeoIdList.begin()) - iterfirstpoint = scp; - } - else { - Base::Console().error("Unsupported Geometry!! Just skipping it.\n"); + if (std::ranges::find(newgeoIdList, definedGeo) == newgeoIdList.end()) { + // we should not copy internal alignment geometry, unless the element they + // define is also mirrored continue; } - - if (!moveonly) {// we are copying - newgeoVals.push_back(geocopy); - geoIdMap.insert(std::make_pair(*it, cgeoid)); - cgeoid++; - } } + // We have already cloned all geometry and constraints, we only need a copy if not + // moving if (!moveonly) { - // handle geometry constraints - for (std::vector::const_iterator it = constrvals.begin(); - it != constrvals.end(); - ++it) { - - auto fit = geoIdMap.find((*it)->First); - - if (fit != geoIdMap.end()) {// if First of constraint is in geoIdList - - if ((*it)->Second - == GeoEnum::GeoUndef /*&& (*it)->Third == GeoEnum::GeoUndef*/) { - if (((*it)->Type != Sketcher::DistanceX - && (*it)->Type != Sketcher::DistanceY) - || (*it)->FirstPos == Sketcher::PointPos::none) { - // if it is not a point locking DistanceX/Y - if (((*it)->Type == Sketcher::DistanceX - || (*it)->Type == Sketcher::DistanceY - || (*it)->Type == Sketcher::Distance - || (*it)->Type == Sketcher::Diameter - || (*it)->Type == Sketcher::Weight - || (*it)->Type == Sketcher::Radius) - && clone) { - // Distances on a single Element are mapped to equality - // constraints in clone mode - Constraint* constNew = (*it)->copy(); - constNew->Type = Sketcher::Equal; - constNew->isDriving = true; - // first is already (*it->First) - constNew->Second = fit->second; - newconstrVals.push_back(constNew); - } - else if ((*it)->Type == Sketcher::Angle && clone) { - if (getGeometry((*it)->First)->is()) { - // Angles on a single Element are mapped to parallel - // constraints in clone mode - Constraint* constNew = (*it)->copy(); - constNew->Type = Sketcher::Parallel; - constNew->isDriving = true; - // first is already (*it->First) - constNew->Second = fit->second; - newconstrVals.push_back(constNew); - } - } - else { - Constraint* constNew = (*it)->copy(); - constNew->First = fit->second; - newconstrVals.push_back(constNew); - } - } - } - else {// other geoids intervene in this constraint - - auto sit = geoIdMap.find((*it)->Second); - - if (sit != geoIdMap.end()) {// Second is also in the list - if ((*it)->Third == GeoEnum::GeoUndef) { - if (((*it)->Type == Sketcher::DistanceX - || (*it)->Type == Sketcher::DistanceY - || (*it)->Type == Sketcher::Distance) - && ((*it)->First == (*it)->Second) && clone) { - // Distances on a two Elements, which must be points of the - // same line are mapped to equality constraints in clone - // mode - Constraint* constNew = (*it)->copy(); - constNew->Type = Sketcher::Equal; - constNew->isDriving = true; - constNew->FirstPos = Sketcher::PointPos::none; - // first is already (*it->First) - constNew->Second = fit->second; - constNew->SecondPos = Sketcher::PointPos::none; - newconstrVals.push_back(constNew); - } - else {// this includes InternalAlignment constraints - Constraint* constNew = (*it)->copy(); - constNew->First = fit->second; - constNew->Second = sit->second; - newconstrVals.push_back(constNew); - } - } - else { - auto tit = geoIdMap.find((*it)->Third); - - if (tit != geoIdMap.end()) {// Third is also in the list - Constraint* constNew = (*it)->copy(); - constNew->First = fit->second; - constNew->Second = sit->second; - constNew->Third = tit->second; - - newconstrVals.push_back(constNew); - } - } - } - } - } - } - - // handle inter-geometry constraints - if (constraindisplacement) { - - // add a construction line - Part::GeomLineSegment* constrline = new Part::GeomLineSegment(); - - // position of the reference point - Base::Vector3d sp = getPoint(refgeoid, refposId) - + ((x == 0) ? (double(x) * displacement - + double(y - 1) * perpendicularDisplacement) - : (double(x - 1) * displacement - + double(y) * perpendicularDisplacement)); - - // position of the current instance corresponding point - Base::Vector3d ep = iterfirstpoint; - constrline->setPoints(sp, ep); - GeometryFacade::setConstruction(constrline, true); - - generateId(constrline); - newgeoVals.push_back(constrline); - - Constraint* constNew; - - if (x == 0) {// first element of a row - - // add coincidents for construction line - constNew = new Constraint(); - constNew->Type = Sketcher::Coincident; - constNew->First = prevrowstartfirstgeoid; - constNew->FirstPos = refposId; - constNew->Second = cgeoid; - constNew->SecondPos = Sketcher::PointPos::start; - newconstrVals.push_back(constNew); - - constNew = new Constraint(); - constNew->Type = Sketcher::Coincident; - constNew->First = iterfirstgeoid; - constNew->FirstPos = refposId; - constNew->Second = cgeoid; - constNew->SecondPos = Sketcher::PointPos::end; - newconstrVals.push_back(constNew); - - // it is the first added element of this row in the perpendicular to - // displacementvector direction - if (y == 1) { - rowrefgeoid = cgeoid; - cgeoid++; - - // add length (or equal if perpscale==1) and perpendicular - if (perpscale == 1.0) { - constNew = new Constraint(); - constNew->Type = Sketcher::Equal; - constNew->First = rowrefgeoid; - constNew->FirstPos = Sketcher::PointPos::none; - constNew->Second = colrefgeoid; - constNew->SecondPos = Sketcher::PointPos::none; - newconstrVals.push_back(constNew); - } - else { - constNew = new Constraint(); - constNew->Type = Sketcher::Distance; - constNew->First = rowrefgeoid; - constNew->FirstPos = Sketcher::PointPos::none; - constNew->setValue(perpendicularDisplacement.Length()); - newconstrVals.push_back(constNew); - } - - constNew = new Constraint(); - constNew->Type = Sketcher::Perpendicular; - constNew->First = rowrefgeoid; - constNew->FirstPos = Sketcher::PointPos::none; - constNew->Second = colrefgeoid; - constNew->SecondPos = Sketcher::PointPos::none; - newconstrVals.push_back(constNew); - } - else {// it is just one more element in the col direction - cgeoid++; - - // all other first rowers get an equality and perpendicular constraint - constNew = new Constraint(); - constNew->Type = Sketcher::Equal; - constNew->First = rowrefgeoid; - constNew->FirstPos = Sketcher::PointPos::none; - constNew->Second = cgeoid - 1; - constNew->SecondPos = Sketcher::PointPos::none; - newconstrVals.push_back(constNew); - - constNew = new Constraint(); - constNew->Type = Sketcher::Perpendicular; - constNew->First = cgeoid - 1; - constNew->FirstPos = Sketcher::PointPos::none; - constNew->Second = colrefgeoid; - constNew->SecondPos = Sketcher::PointPos::none; - newconstrVals.push_back(constNew); - } - } - else {// any element not being the first element of a row - - // add coincidents for construction line - constNew = new Constraint(); - constNew->Type = Sketcher::Coincident; - constNew->First = prevfirstgeoid; - constNew->FirstPos = refposId; - constNew->Second = cgeoid; - constNew->SecondPos = Sketcher::PointPos::start; - newconstrVals.push_back(constNew); - - constNew = new Constraint(); - constNew->Type = Sketcher::Coincident; - constNew->First = iterfirstgeoid; - constNew->FirstPos = refposId; - constNew->Second = cgeoid; - constNew->SecondPos = Sketcher::PointPos::end; - newconstrVals.push_back(constNew); - - if (y == 0 && x == 1) {// first element of the first row - colrefgeoid = cgeoid; - cgeoid++; - - // add length and Angle - constNew = new Constraint(); - constNew->Type = Sketcher::Distance; - constNew->First = colrefgeoid; - constNew->FirstPos = Sketcher::PointPos::none; - constNew->setValue(displacement.Length()); - newconstrVals.push_back(constNew); - - constNew = new Constraint(); - constNew->Type = Sketcher::Angle; - constNew->First = colrefgeoid; - constNew->FirstPos = Sketcher::PointPos::none; - constNew->setValue(atan2(displacement.y, displacement.x)); - newconstrVals.push_back(constNew); - } - else {// any other element - cgeoid++; - - // all other elements get an equality and parallel constraint - constNew = new Constraint(); - constNew->Type = Sketcher::Equal; - constNew->First = colrefgeoid; - constNew->FirstPos = Sketcher::PointPos::none; - constNew->Second = cgeoid - 1; - constNew->SecondPos = Sketcher::PointPos::none; - newconstrVals.push_back(constNew); - - constNew = new Constraint(); - constNew->Type = Sketcher::Parallel; - constNew->First = cgeoid - 1; - constNew->FirstPos = Sketcher::PointPos::none; - constNew->Second = colrefgeoid; - constNew->SecondPos = Sketcher::PointPos::none; - newconstrVals.push_back(constNew); - } - } - } - // after each creation reset map so that the key-value is univoque (only for - // operations other than move) - geoIdMap.clear(); + geocopy = geo->copy(); + generateId(geocopy); } + else { + geocopy = newgeoVals[*it]; + } + + // Handle Geometry + if (geocopy->is()) { + auto* geosymline = static_cast(geocopy); + Base::Vector3d ep = geosymline->getEndPoint(); + Base::Vector3d ssp = geosymline->getStartPoint() + double(x) * displacement + + double(y) * perpendicularDisplacement; + + geosymline->setPoints(ssp, + ep + double(x) * displacement + + double(y) * perpendicularDisplacement); + + if (it == newgeoIdList.begin()) { + iterfirstpoint = ssp; + } + } + else if (geocopy->is()) { + auto* geosymcircle = static_cast(geocopy); + Base::Vector3d cp = geosymcircle->getCenter(); + Base::Vector3d scp = + cp + double(x) * displacement + double(y) * perpendicularDisplacement; + + geosymcircle->setCenter(scp); + + if (it == newgeoIdList.begin()) { + iterfirstpoint = scp; + } + } + else if (geocopy->is()) { + auto* geoaoc = static_cast(geocopy); + Base::Vector3d cp = geoaoc->getCenter(); + Base::Vector3d scp = + cp + double(x) * displacement + double(y) * perpendicularDisplacement; + + geoaoc->setCenter(scp); + + if (it == newgeoIdList.begin()) { + iterfirstpoint = geoaoc->getStartPoint(true); + } + } + else if (geocopy->is()) { + auto* geosymellipse = static_cast(geocopy); + Base::Vector3d cp = geosymellipse->getCenter(); + Base::Vector3d scp = + cp + double(x) * displacement + double(y) * perpendicularDisplacement; + + geosymellipse->setCenter(scp); + + if (it == newgeoIdList.begin()) { + iterfirstpoint = scp; + } + } + else if (geocopy->is()) { + auto* geoaoe = static_cast(geocopy); + Base::Vector3d cp = geoaoe->getCenter(); + Base::Vector3d scp = + cp + double(x) * displacement + double(y) * perpendicularDisplacement; + + geoaoe->setCenter(scp); + + if (it == newgeoIdList.begin()) { + iterfirstpoint = geoaoe->getStartPoint(true); + } + } + else if (geocopy->is()) { + auto* geoaoe = static_cast(geocopy); + Base::Vector3d cp = geoaoe->getCenter(); + Base::Vector3d scp = + cp + double(x) * displacement + double(y) * perpendicularDisplacement; + + geoaoe->setCenter(scp); + + if (it == newgeoIdList.begin()) { + iterfirstpoint = geoaoe->getStartPoint(true); + } + } + else if (geocopy->is()) { + auto* geoaoe = static_cast(geocopy); + Base::Vector3d cp = geoaoe->getCenter(); + Base::Vector3d scp = + cp + double(x) * displacement + double(y) * perpendicularDisplacement; + + geoaoe->setCenter(scp); + + if (it == newgeoIdList.begin()) { + iterfirstpoint = geoaoe->getStartPoint(true); + } + } + else if (geocopy->is()) { + auto* geobsp = static_cast(geocopy); + + std::vector poles = geobsp->getPoles(); + + for (std::vector::iterator jt = poles.begin(); jt != poles.end(); + ++jt) { + + (*jt) = + (*jt) + double(x) * displacement + double(y) * perpendicularDisplacement; + } + + geobsp->setPoles(poles); + + if (it == newgeoIdList.begin()) { + iterfirstpoint = geobsp->getStartPoint(); + } + } + else if (geocopy->is()) { + Part::GeomPoint* geopoint = static_cast(geocopy); + Base::Vector3d cp = geopoint->getPoint(); + Base::Vector3d scp = + cp + double(x) * displacement + double(y) * perpendicularDisplacement; + geopoint->setPoint(scp); + + if (it == newgeoIdList.begin()) { + iterfirstpoint = scp; + } + } + else { + Base::Console().error("Unsupported Geometry!! Just skipping it.\n"); + continue; + } + + if (!moveonly) { // we are copying + newgeoVals.push_back(geocopy); + geoIdMap.insert(std::make_pair(*it, cgeoid)); + cgeoid++; + } + } + + if (moveonly) { + return; + } + + // handle geometry constraints + for (const auto& constr : constrvals) { + auto fit = geoIdMap.find(constr->First); + + if (fit == geoIdMap.end()) { + continue; + } + + // First of constraint is in geoIdList + if (constr->Second == GeoEnum::GeoUndef /*&& constr->Third == GeoEnum::GeoUndef*/) { + if ((constr->Type == Sketcher::DistanceX || constr->Type == Sketcher::DistanceY) + && constr->FirstPos != Sketcher::PointPos::none) { + continue; + } + // if it is not a point locking DistanceX/Y + if ((constr->Type == Sketcher::DistanceX || constr->Type == Sketcher::DistanceY + || constr->Type == Sketcher::Distance || constr->Type == Sketcher::Diameter + || constr->Type == Sketcher::Weight || constr->Type == Sketcher::Radius) + && clone) { + // Distances on a single Element are mapped to equality + // constraints in clone mode + Constraint* constNew = constr->copy(); + constNew->Type = Sketcher::Equal; + constNew->isDriving = true; + // first is already (constr->First) + constNew->Second = fit->second; + newconstrVals.push_back(constNew); + continue; + } + if (!(constr->Type == Sketcher::Angle && clone)) { + Constraint* constNew = constr->copy(); + constNew->First = fit->second; + newconstrVals.push_back(constNew); + continue; + } + if (getGeometry(constr->First)->is()) { + // Angles on a single Element are mapped to parallel + // constraints in clone mode + Constraint* constNew = constr->copy(); + constNew->Type = Sketcher::Parallel; + constNew->isDriving = true; + // first is already (constr->First) + constNew->Second = fit->second; + newconstrVals.push_back(constNew); + } + continue; + } + + // other geoids intervene in this constraint + auto sit = geoIdMap.find(constr->Second); + + if (sit == geoIdMap.end()) { + continue; + } + + // Second is also in the list + if (constr->Third == GeoEnum::GeoUndef) { + if ((constr->Type == Sketcher::DistanceX || constr->Type == Sketcher::DistanceY + || constr->Type == Sketcher::Distance) + && (constr->First == constr->Second) && clone) { + // Distances on a two Elements, which must be points of the + // same line are mapped to equality constraints in clone + // mode + Constraint* constNew = constr->copy(); + constNew->Type = Sketcher::Equal; + constNew->isDriving = true; + constNew->FirstPos = Sketcher::PointPos::none; + // first is already (constr->First) + constNew->Second = fit->second; + constNew->SecondPos = Sketcher::PointPos::none; + newconstrVals.push_back(constNew); + continue; + } + // remaining, this includes InternalAlignment constraints + Constraint* constNew = constr->copy(); + constNew->First = fit->second; + constNew->Second = sit->second; + newconstrVals.push_back(constNew); + continue; + } + + auto tit = geoIdMap.find(constr->Third); + + if (tit != geoIdMap.end()) { + continue; + } + + // Third is also in the list + Constraint* constNew = constr->copy(); + constNew->First = fit->second; + constNew->Second = sit->second; + constNew->Third = tit->second; + + newconstrVals.push_back(constNew); + } + + // handle inter-geometry constraints + if (!constraindisplacement) { + // after each creation reset map so that the key-value is univoque (only for + // operations other than move) + geoIdMap.clear(); + } + + // add a construction line + Part::GeomLineSegment* constrline = new Part::GeomLineSegment(); + + // position of the reference point + Base::Vector3d sp = getPoint(refgeoid, refposId) + + ((x == 0) ? (double(x) * displacement + double(y - 1) * perpendicularDisplacement) + : (double(x - 1) * displacement + double(y) * perpendicularDisplacement)); + + // position of the current instance corresponding point + Base::Vector3d ep = iterfirstpoint; + constrline->setPoints(sp, ep); + GeometryFacade::setConstruction(constrline, true); + + generateId(constrline); + newgeoVals.push_back(constrline); + + Constraint* constNew; + + if (x == 0) { + // first element of a row + + // add coincidents for construction line + constNew = new Constraint(); + constNew->Type = Sketcher::Coincident; + constNew->First = prevrowstartfirstgeoid; + constNew->FirstPos = refposId; + constNew->Second = cgeoid; + constNew->SecondPos = Sketcher::PointPos::start; + newconstrVals.push_back(constNew); + + constNew = new Constraint(); + constNew->Type = Sketcher::Coincident; + constNew->First = iterfirstgeoid; + constNew->FirstPos = refposId; + constNew->Second = cgeoid; + constNew->SecondPos = Sketcher::PointPos::end; + newconstrVals.push_back(constNew); + + // it is the first added element of this row in the perpendicular to + // displacementvector direction + if (y == 1) { + rowrefgeoid = cgeoid; + cgeoid++; + + // add length (or equal if perpscale==1) and perpendicular + if (perpscale == 1.0) { + constNew = new Constraint(); + constNew->Type = Sketcher::Equal; + constNew->First = rowrefgeoid; + constNew->FirstPos = Sketcher::PointPos::none; + constNew->Second = colrefgeoid; + constNew->SecondPos = Sketcher::PointPos::none; + newconstrVals.push_back(constNew); + } + else { + constNew = new Constraint(); + constNew->Type = Sketcher::Distance; + constNew->First = rowrefgeoid; + constNew->FirstPos = Sketcher::PointPos::none; + constNew->setValue(perpendicularDisplacement.Length()); + newconstrVals.push_back(constNew); + } + + constNew = new Constraint(); + constNew->Type = Sketcher::Perpendicular; + constNew->First = rowrefgeoid; + constNew->FirstPos = Sketcher::PointPos::none; + constNew->Second = colrefgeoid; + constNew->SecondPos = Sketcher::PointPos::none; + newconstrVals.push_back(constNew); + } + else { + // it is just one more element in the col direction + cgeoid++; + + // all other first rowers get an equality and perpendicular constraint + constNew = new Constraint(); + constNew->Type = Sketcher::Equal; + constNew->First = rowrefgeoid; + constNew->FirstPos = Sketcher::PointPos::none; + constNew->Second = cgeoid - 1; + constNew->SecondPos = Sketcher::PointPos::none; + newconstrVals.push_back(constNew); + + constNew = new Constraint(); + constNew->Type = Sketcher::Perpendicular; + constNew->First = cgeoid - 1; + constNew->FirstPos = Sketcher::PointPos::none; + constNew->Second = colrefgeoid; + constNew->SecondPos = Sketcher::PointPos::none; + newconstrVals.push_back(constNew); + } + } + else { + // any element not being the first element of a row + + // add coincidents for construction line + constNew = new Constraint(); + constNew->Type = Sketcher::Coincident; + constNew->First = prevfirstgeoid; + constNew->FirstPos = refposId; + constNew->Second = cgeoid; + constNew->SecondPos = Sketcher::PointPos::start; + newconstrVals.push_back(constNew); + + constNew = new Constraint(); + constNew->Type = Sketcher::Coincident; + constNew->First = iterfirstgeoid; + constNew->FirstPos = refposId; + constNew->Second = cgeoid; + constNew->SecondPos = Sketcher::PointPos::end; + newconstrVals.push_back(constNew); + + if (y == 0 && x == 1) { + // first element of the first row + colrefgeoid = cgeoid; + cgeoid++; + + // add length and Angle + constNew = new Constraint(); + constNew->Type = Sketcher::Distance; + constNew->First = colrefgeoid; + constNew->FirstPos = Sketcher::PointPos::none; + constNew->setValue(displacement.Length()); + newconstrVals.push_back(constNew); + + constNew = new Constraint(); + constNew->Type = Sketcher::Angle; + constNew->First = colrefgeoid; + constNew->FirstPos = Sketcher::PointPos::none; + constNew->setValue(atan2(displacement.y, displacement.x)); + newconstrVals.push_back(constNew); + } + else { + // any other element + cgeoid++; + + // all other elements get an equality and parallel constraint + constNew = new Constraint(); + constNew->Type = Sketcher::Equal; + constNew->First = colrefgeoid; + constNew->FirstPos = Sketcher::PointPos::none; + constNew->Second = cgeoid - 1; + constNew->SecondPos = Sketcher::PointPos::none; + newconstrVals.push_back(constNew); + + constNew = new Constraint(); + constNew->Type = Sketcher::Parallel; + constNew->First = cgeoid - 1; + constNew->FirstPos = Sketcher::PointPos::none; + constNew->Second = colrefgeoid; + constNew->SecondPos = Sketcher::PointPos::none; + newconstrVals.push_back(constNew); + } + } + + // after each creation reset map so that the key-value is univoque (only for + // operations other than move) + geoIdMap.clear(); + }; + + for (y = 0; y < rsize; y++) { + for (x = 0; x < csize; x++) { + makeCopyAtRowColumn(x, y); } } @@ -5464,8 +5487,9 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 Base::StateLocker preventUpdate(internaltransaction, true); Geometry.setValues(std::move(newgeoVals)); - if (newconstrVals.size() > constrvals.size()) + if (newconstrVals.size() > constrvals.size()) { Constraints.setValues(std::move(newconstrVals)); + } } // we inhibited update, so we trigger it now @@ -5475,6 +5499,7 @@ int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3 return Geometry.getSize() - 1; } +// clang-format off int SketchObject::removeAxesAlignment(const std::vector& geoIdList) { From ef8fe4d4d2479ee759019d80c16e23d7b7dc0fb1 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 12 Aug 2024 21:02:16 +0530 Subject: [PATCH 08/23] [Sketcher] Use range-`for` loop where straightforward --- src/Mod/Sketcher/App/SketchObject.cpp | 288 +++++++++++++------------- 1 file changed, 147 insertions(+), 141 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 7bd12caf30..ee2a26c819 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -1477,9 +1477,7 @@ int SketchObject::moveGeometries(const std::vector& geoEltIds, con std::vector geomlist = solvedSketch.extractGeometry(); Geometry.setValues(geomlist); for (auto* geo : geomlist) { - if (geo){ - delete geo; - } + delete geo; } } @@ -4986,8 +4984,8 @@ int SketchObject::addCopy(const std::vector& geoIdList, std::map geoIdMap; - Base::Vector3d perpendicularDisplacement = - Base::Vector3d(perpscale * displacement.y, perpscale * -displacement.x, 0); + Base::Vector3d perpendicularDisplacement + = Base::Vector3d(perpscale * displacement.y, perpscale * -displacement.x, 0); int x, y; @@ -5004,20 +5002,23 @@ int SketchObject::addCopy(const std::vector& geoIdList, // the list. auto constrIt = std::ranges::find_if(constrvals, [&newgeoIdList](auto c) { - return (c->Type == Sketcher::InternalAlignment - && c->First == *(newgeoIdList.begin())); + return ( + c->Type == Sketcher::InternalAlignment && c->First == *(newgeoIdList.begin()) + ); }); - int definedGeo = - (constrIt != constrvals.end()) ? (*constrIt)->Second : GeoEnum::GeoUndef; + int definedGeo = (constrIt != constrvals.end()) ? (*constrIt)->Second + : GeoEnum::GeoUndef; if (std::ranges::find(newgeoIdList, definedGeo) == newgeoIdList.end()) { // the first element setting the reference is an internal alignment // geometry, wherein the geometry it defines is not part of the copy // operation. - THROWM(Base::ValueError, - "A move/copy/array operation on an internal alignment geometry is " - "only possible together with the geometry it defines."); + THROWM( + Base::ValueError, + "A move/copy/array operation on an internal alignment geometry is " + "only possible together with the geometry it defines." + ); } } @@ -5087,9 +5088,10 @@ int SketchObject::addCopy(const std::vector& geoIdList, Base::Vector3d ssp = geosymline->getStartPoint() + double(x) * displacement + double(y) * perpendicularDisplacement; - geosymline->setPoints(ssp, - ep + double(x) * displacement - + double(y) * perpendicularDisplacement); + geosymline->setPoints( + ssp, + ep + double(x) * displacement + double(y) * perpendicularDisplacement + ); if (it == newgeoIdList.begin()) { iterfirstpoint = ssp; @@ -5098,8 +5100,8 @@ int SketchObject::addCopy(const std::vector& geoIdList, else if (geocopy->is()) { auto* geosymcircle = static_cast(geocopy); Base::Vector3d cp = geosymcircle->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; + Base::Vector3d scp = cp + double(x) * displacement + + double(y) * perpendicularDisplacement; geosymcircle->setCenter(scp); @@ -5110,8 +5112,8 @@ int SketchObject::addCopy(const std::vector& geoIdList, else if (geocopy->is()) { auto* geoaoc = static_cast(geocopy); Base::Vector3d cp = geoaoc->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; + Base::Vector3d scp = cp + double(x) * displacement + + double(y) * perpendicularDisplacement; geoaoc->setCenter(scp); @@ -5122,8 +5124,8 @@ int SketchObject::addCopy(const std::vector& geoIdList, else if (geocopy->is()) { auto* geosymellipse = static_cast(geocopy); Base::Vector3d cp = geosymellipse->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; + Base::Vector3d scp = cp + double(x) * displacement + + double(y) * perpendicularDisplacement; geosymellipse->setCenter(scp); @@ -5134,8 +5136,8 @@ int SketchObject::addCopy(const std::vector& geoIdList, else if (geocopy->is()) { auto* geoaoe = static_cast(geocopy); Base::Vector3d cp = geoaoe->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; + Base::Vector3d scp = cp + double(x) * displacement + + double(y) * perpendicularDisplacement; geoaoe->setCenter(scp); @@ -5146,8 +5148,8 @@ int SketchObject::addCopy(const std::vector& geoIdList, else if (geocopy->is()) { auto* geoaoe = static_cast(geocopy); Base::Vector3d cp = geoaoe->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; + Base::Vector3d scp = cp + double(x) * displacement + + double(y) * perpendicularDisplacement; geoaoe->setCenter(scp); @@ -5158,8 +5160,8 @@ int SketchObject::addCopy(const std::vector& geoIdList, else if (geocopy->is()) { auto* geoaoe = static_cast(geocopy); Base::Vector3d cp = geoaoe->getCenter(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; + Base::Vector3d scp = cp + double(x) * displacement + + double(y) * perpendicularDisplacement; geoaoe->setCenter(scp); @@ -5172,11 +5174,8 @@ int SketchObject::addCopy(const std::vector& geoIdList, std::vector poles = geobsp->getPoles(); - for (std::vector::iterator jt = poles.begin(); jt != poles.end(); - ++jt) { - - (*jt) = - (*jt) + double(x) * displacement + double(y) * perpendicularDisplacement; + for (auto& pole : poles) { + pole = pole + double(x) * displacement + double(y) * perpendicularDisplacement; } geobsp->setPoles(poles); @@ -5188,8 +5187,8 @@ int SketchObject::addCopy(const std::vector& geoIdList, else if (geocopy->is()) { Part::GeomPoint* geopoint = static_cast(geocopy); Base::Vector3d cp = geopoint->getPoint(); - Base::Vector3d scp = - cp + double(x) * displacement + double(y) * perpendicularDisplacement; + Base::Vector3d scp = cp + double(x) * displacement + + double(y) * perpendicularDisplacement; geopoint->setPoint(scp); if (it == newgeoIdList.begin()) { @@ -7062,7 +7061,6 @@ bool SketchObject::insertBSplineKnot(int GeoId, double param, int multiplicity) return true; } -// clang-format off int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) { @@ -7074,8 +7072,9 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) // so far only externals to the support of the sketch and datum features bool xinv = false, yinv = false; - if (!isCarbonCopyAllowed(pObj->getDocument(), pObj, xinv, yinv)) + if (!isCarbonCopyAllowed(pObj->getDocument(), pObj, xinv, yinv)) { return -1; + } SketchObject* psObj = static_cast(pObj); @@ -7111,17 +7110,18 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) auto geos = this->ExternalGeo.getValues(); std::string myName(this->getNameInDocument()); myName += "."; - for (const auto &geo : psObj->ExternalGeo.getValues()) { - if (++i < 2) // skip h/v axes + for (const auto& geo : psObj->ExternalGeo.getValues()) { + if (++i < 2) { // skip h/v axes continue; + } else { auto egf = ExternalGeometryFacade::getFacade(geo); - const auto &ref = egf->getRef(); + const auto& ref = egf->getRef(); if (boost::starts_with(ref, myName)) { int geoId; PointPos posId; - if (this->geoIdFromShapeType(ref.c_str()+myName.size(), geoId, posId)) { - extMap[-i-1] = geoId; + if (this->geoIdFromShapeType(ref.c_str() + myName.size(), geoId, posId)) { + extMap[-i - 1] = geoId; continue; } } @@ -7130,15 +7130,17 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) auto egf = ExternalGeometryFacade::getFacade(copy); egf->setId(++geoLastId); if (!egf->getRef().empty()) { - auto &refs = this->externalGeoRefMap[egf->getRef()]; + auto& refs = this->externalGeoRefMap[egf->getRef()]; refs.push_back(geoLastId); } this->externalGeoMap[geoLastId] = (int)geos.size(); geos.push_back(copy); - extMap[-i-1] = -(int)geos.size(); + extMap[-i - 1] = -(int)geos.size(); } - Base::ObjectStatusLocker - guard(App::Property::User3, &this->ExternalGeo); + Base::ObjectStatusLocker guard( + App::Property::User3, + &this->ExternalGeo + ); this->ExternalGeo.setValues(std::move(geos)); } @@ -7154,8 +7156,10 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) if (Objects.size() != SubElements.size() || sObjects.size() != sSubElements.size()) { assert(0 /*counts of objects and subelements in external geometry links do not match*/); - Base::Console().error("Internal error: counts of objects and subelements in external " - "geometry links do not match\n"); + Base::Console().error( + "Internal error: counts of objects and subelements in external " + "geometry links do not match\n" + ); return -1; } @@ -7166,7 +7170,8 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) if (obj == sobj && SubElements[i] == sSubElements[si]) { Base::Console().error( "Link to %s already exists in this sketch. Delete the link and try again\n", - sSubElements[si].c_str()); + sSubElements[si].c_str() + ); return -1; } @@ -7194,8 +7199,7 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) solverNeedsUpdate = true; } - auto applyGeometryFlipCorrection = [xinv, yinv, origin, axisV, axisH] - (Part::Geometry* geoNew) { + auto applyGeometryFlipCorrection = [xinv, yinv, origin, axisV, axisH](Part::Geometry* geoNew) { if (!xinv && !yinv) { return; } @@ -7208,8 +7212,8 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) } }; - for (std::vector::const_iterator it = svals.begin(); it != svals.end(); ++it) { - Part::Geometry* geoNew = (*it)->copy(); + for (const auto& geoOld : svals) { + Part::Geometry* geoNew = geoOld->copy(); if (xinv || yinv) { // corrections for flipped geometry applyGeometryFlipCorrection(geoNew); @@ -7221,18 +7225,18 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) newVals.push_back(geoNew); } - auto applyConstraintFlipCorrection = [xinv, yinv] - (Sketcher::Constraint* newConstr) { + auto applyConstraintFlipCorrection = [xinv, yinv](Sketcher::Constraint* newConstr) { if (!xinv && !yinv) { return; } // DistanceX, DistanceY - if ((xinv && newConstr->Type == Sketcher::DistanceX) || - (yinv && newConstr->Type == Sketcher::DistanceY)) { + if ((xinv && newConstr->Type == Sketcher::DistanceX) + || (yinv && newConstr->Type == Sketcher::DistanceY)) { if (newConstr->First == newConstr->Second) { std::swap(newConstr->FirstPos, newConstr->SecondPos); - } else{ + } + else { newConstr->setValue(-newConstr->getValue()); } } @@ -7240,15 +7244,18 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) // Angle if (newConstr->Type == Sketcher::Angle) { auto normalizeAngle = [](double angleDeg) { - while (angleDeg > pi) angleDeg -= pi * 2.0; - while (angleDeg <= -pi) angleDeg += pi * 2.0; + while (angleDeg > pi) { + angleDeg -= pi * 2.0; + } + while (angleDeg <= -pi) { + angleDeg += pi * 2.0; + } return angleDeg; }; - if (xinv && yinv) { // rotation 180 degrees around normal axis - if (newConstr->First ==-1 || newConstr->Second == -1 - || newConstr->First == -2 || newConstr->Second == -2 - || newConstr->Second == GeoEnum::GeoUndef) { + if (xinv && yinv) { // rotation 180 degrees around normal axis + if (newConstr->First == -1 || newConstr->Second == -1 || newConstr->First == -2 + || newConstr->Second == -2 || newConstr->Second == GeoEnum::GeoUndef) { // angle to horizontal or vertical axis newConstr->setValue(normalizeAngle(newConstr->getValue() + pi)); } @@ -7257,8 +7264,9 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) // do nothing } } - else if (xinv) { // rotation 180 degrees around vertical axis - if (newConstr->First == -1 || newConstr->Second == -1 || newConstr->Second == GeoEnum::GeoUndef) { + else if (xinv) { // rotation 180 degrees around vertical axis + if (newConstr->First == -1 || newConstr->Second == -1 + || newConstr->Second == GeoEnum::GeoUndef) { // angle to horizontal axis newConstr->setValue(normalizeAngle(pi - newConstr->getValue())); } @@ -7267,7 +7275,7 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) newConstr->setValue(normalizeAngle(-newConstr->getValue())); } } - else if (yinv) { // rotation 180 degrees around horizontal axis + else if (yinv) { // rotation 180 degrees around horizontal axis if (newConstr->First == -2 || newConstr->Second == -2) { // angle to vertical axis newConstr->setValue(normalizeAngle(pi - newConstr->getValue())); @@ -7280,22 +7288,27 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) } }; - for (std::vector::const_iterator it = scvals.begin(); it != scvals.end(); - ++it) { - Sketcher::Constraint* newConstr = (*it)->copy(); - if ((*it)->First >= 0) + for (const auto& constr : scvals) { + Sketcher::Constraint* newConstr = constr->copy(); + if (constr->First >= 0) { newConstr->First += nextgeoid; - if ((*it)->Second >= 0) + } + if (constr->Second >= 0) { newConstr->Second += nextgeoid; - if ((*it)->Third >= 0) + } + if (constr->Third >= 0) { newConstr->Third += nextgeoid; + } - if ((*it)->First < -2 && (*it)->First != GeoEnum::GeoUndef) + if (constr->First < -2 && constr->First != GeoEnum::GeoUndef) { newConstr->First -= (nextextgeoid - 2); - if ((*it)->Second < -2 && (*it)->Second != GeoEnum::GeoUndef) + } + if (constr->Second < -2 && constr->Second != GeoEnum::GeoUndef) { newConstr->Second -= (nextextgeoid - 2); - if ((*it)->Third < -2 && (*it)->Third != GeoEnum::GeoUndef) + } + if (constr->Third < -2 && constr->Third != GeoEnum::GeoUndef) { newConstr->Third -= (nextextgeoid - 2); + } if (xinv || yinv) { // corrections for flipped constraints @@ -7316,29 +7329,28 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) // ViewProvider::UpdateData is triggered. Geometry.touch(); - auto makeCorrectedExpressionString = [xinv, yinv] - (const Sketcher::Constraint* constr, const std::string expr) - -> std::string { + auto makeCorrectedExpressionString = + [xinv, yinv](const Sketcher::Constraint* constr, const std::string expr) -> std::string { if (!xinv && !yinv) { return expr; } // DistanceX, DistanceY - if ((xinv && constr->Type == Sketcher::DistanceX) || - (yinv && constr->Type == Sketcher::DistanceY)) { + if ((xinv && constr->Type == Sketcher::DistanceX) + || (yinv && constr->Type == Sketcher::DistanceY)) { if (constr->First == constr->Second) { return expr; - } else{ + } + else { return "-(" + expr + ")"; } } // Angle if (constr->Type == Sketcher::Angle) { - if (xinv && yinv) { // rotation 180 degrees around normal axis - if (constr->First ==-1 || constr->Second == -1 - || constr->First == -2 || constr->Second == -2 - || constr->Second == GeoEnum::GeoUndef) { + if (xinv && yinv) { // rotation 180 degrees around normal axis + if (constr->First == -1 || constr->Second == -1 || constr->First == -2 + || constr->Second == -2 || constr->Second == GeoEnum::GeoUndef) { // angle to horizontal or vertical axis return "(" + expr + ") + 180 deg"; } @@ -7348,8 +7360,9 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) return expr; } } - else if (xinv) { // rotation 180 degrees around vertical axis - if (constr->First == -1 || constr->Second == -1 || constr->Second == GeoEnum::GeoUndef) { + else if (xinv) { // rotation 180 degrees around vertical axis + if (constr->First == -1 || constr->Second == -1 + || constr->Second == GeoEnum::GeoUndef) { // angle to horizontal axis return "180 deg - (" + expr + ")"; } @@ -7358,7 +7371,7 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) return "-(" + expr + ")"; } } - else if (yinv) { // rotation 180 degrees around horizontal axis + else if (yinv) { // rotation 180 degrees around horizontal axis if (constr->First == -2 || constr->Second == -2) { // angle to vertical axis return "180 deg - (" + expr + ")"; @@ -7373,34 +7386,30 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) }; int sourceid = 0; - for (std::vector::const_iterator it = scvals.begin(); it != scvals.end(); - ++it, nextcid++, sourceid++) { - - if ((*it)->isDimensional()) { - // then we link its value to the parent - if ((*it)->isDriving) { - App::ObjectIdentifier spath; - std::shared_ptr expr; - std::string scname = (*it)->Name; - std::string sref; - if (App::ExpressionParser::isTokenAnIndentifier(scname)) { - spath = App::ObjectIdentifier(psObj->Constraints) - << App::ObjectIdentifier::SimpleComponent(scname); - sref = spath.getDocumentObjectName().getString() + spath.toString(); - } - else { - spath = psObj->Constraints.createPath(sourceid); - sref = spath.getDocumentObjectName().getString() - + std::string(1, '.') + spath.toString(); - } - if (xinv || yinv) { - // corrections for flipped expressions - sref = makeCorrectedExpressionString((*it), sref); - } - expr = std::shared_ptr(App::Expression::parse(this, sref)); - setExpression(Constraints.createPath(nextcid), std::move(expr)); - } + for (auto it = scvals.cbegin(); it != scvals.cend(); ++it, ++nextcid, ++sourceid) { + if (!((*it)->isDimensional() && (*it)->isDriving)) { + continue; } + + App::ObjectIdentifier spath; + std::shared_ptr expr; + std::string scname = (*it)->Name; + std::string sref; + if (App::ExpressionParser::isTokenAnIndentifier(scname)) { + spath = App::ObjectIdentifier(psObj->Constraints) + << App::ObjectIdentifier::SimpleComponent(scname); + sref = spath.getDocumentObjectName().getString() + spath.toString(); + } + else { + spath = psObj->Constraints.createPath(sourceid); + sref = spath.getDocumentObjectName().getString() + std::string(1, '.') + spath.toString(); + } + if (xinv || yinv) { + // corrections for flipped expressions + sref = makeCorrectedExpressionString((*it), sref); + } + expr = std::shared_ptr(App::Expression::parse(this, sref)); + setExpression(Constraints.createPath(nextcid), std::move(expr)); } // Solve even if `noRecomputes==false`, because recompute may fail, and leave the @@ -7414,7 +7423,6 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) return svals.size(); } -// clang-format on int SketchObject::addExternal(App::DocumentObject* Obj, const char* SubName, bool defining, bool intersection) { // no need to check input data validity as this is an sketchobject managed operation. @@ -7758,11 +7766,10 @@ int SketchObject::delConstraintsToExternal(DeleteOptions options) const std::vector& constraints = Constraints.getValuesForce(); std::vector newConstraints(0); int GeoId = GeoEnum::RefExt, NullId = GeoEnum::GeoUndef; - for (std::vector::const_iterator it = constraints.begin(); it != constraints.end(); - ++it) { - if ((*it)->First > GeoId && ((*it)->Second > GeoId || (*it)->Second == NullId) - && ((*it)->Third > GeoId || (*it)->Third == NullId)) { - newConstraints.push_back(*it); + for (const auto& constr : constraints) { + if (constr->First > GeoId && (constr->Second > GeoId || constr->Second == NullId) + && (constr->Third > GeoId || constr->Third == NullId)) { + newConstraints.push_back(constr); } } @@ -9727,30 +9734,29 @@ void SketchObject::getDirectlyCoincidentPoints(int GeoId, PointPos PosId, PosIdList.clear(); GeoIdList.push_back(GeoId); PosIdList.push_back(PosId); - for (std::vector::const_iterator it = constraints.begin(); it != constraints.end(); - ++it) { - if ((*it)->Type == Sketcher::Coincident) { - if ((*it)->First == GeoId && (*it)->FirstPos == PosId) { - GeoIdList.push_back((*it)->Second); - PosIdList.push_back((*it)->SecondPos); + for (const auto& constr : constraints) { + if (constr->Type == Sketcher::Coincident) { + if (constr->First == GeoId && constr->FirstPos == PosId) { + GeoIdList.push_back(constr->Second); + PosIdList.push_back(constr->SecondPos); } - else if ((*it)->Second == GeoId && (*it)->SecondPos == PosId) { - GeoIdList.push_back((*it)->First); - PosIdList.push_back((*it)->FirstPos); + else if (constr->Second == GeoId && constr->SecondPos == PosId) { + GeoIdList.push_back(constr->First); + PosIdList.push_back(constr->FirstPos); } } - if ((*it)->Type == Sketcher::Tangent) { - if ((*it)->First == GeoId && (*it)->FirstPos == PosId && - ((*it)->SecondPos == Sketcher::PointPos::start || - (*it)->SecondPos == Sketcher::PointPos::end)) { - GeoIdList.push_back((*it)->Second); - PosIdList.push_back((*it)->SecondPos); + if (constr->Type == Sketcher::Tangent) { + if (constr->First == GeoId && constr->FirstPos == PosId && + (constr->SecondPos == Sketcher::PointPos::start || + constr->SecondPos == Sketcher::PointPos::end)) { + GeoIdList.push_back(constr->Second); + PosIdList.push_back(constr->SecondPos); } - if ((*it)->Second == GeoId && (*it)->SecondPos == PosId && - ((*it)->FirstPos == Sketcher::PointPos::start || - (*it)->FirstPos == Sketcher::PointPos::end)) { - GeoIdList.push_back((*it)->First); - PosIdList.push_back((*it)->FirstPos); + if (constr->Second == GeoId && constr->SecondPos == PosId && + (constr->FirstPos == Sketcher::PointPos::start || + constr->FirstPos == Sketcher::PointPos::end)) { + GeoIdList.push_back(constr->First); + PosIdList.push_back(constr->FirstPos); } } } From b23d3736a72b8b5fd62204a04c8ae3f4ca9c078f Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Tue, 13 Aug 2024 17:50:42 +0530 Subject: [PATCH 09/23] [Sketcher] Move some checks to `Constraint` --- src/Mod/Sketcher/App/SketchObject.cpp | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index ee2a26c819..9e7a437efc 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -3069,9 +3069,7 @@ void SketchObject::changeConstraintAfterDeletingGeo(Constraint* constr, return; } - if (constr->First == deletedGeoId || - constr->Second == deletedGeoId || - constr->Third == deletedGeoId) { + if (constr->involvesGeoId(deletedGeoId)) { constr->Type = ConstraintType::None; return; } @@ -5516,9 +5514,7 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) for (size_t i = 0; i < constrvals.size(); i++) { for (const auto& geoid : geoIdList) { - if (constrvals[i]->First != geoid - && constrvals[i]->Second != geoid - && constrvals[i]->Third != geoid) { + if (!constrvals[i]->involvesGeoId(geoid)) { continue; } switch (constrvals[i]->Type) { @@ -6599,14 +6595,10 @@ bool SketchObject::convertToNURBS(int GeoId) // to-be-converted curve. for (; index >= 0; index--) { auto otherthancoincident = cvals[index]->Type != Sketcher::Coincident - && (cvals[index]->First == GeoId || cvals[index]->Second == GeoId - || cvals[index]->Third == GeoId); + && cvals[index]->involvesGeoId(GeoId); auto coincidentonmidpoint = cvals[index]->Type == Sketcher::Coincident - && ((cvals[index]->First == GeoId - && cvals[index]->FirstPos == Sketcher::PointPos::mid) - || (cvals[index]->Second == GeoId - && cvals[index]->SecondPos == Sketcher::PointPos::mid)); + && cvals[index]->involvesGeoIdAndPosId(GeoId, Sketcher::PointPos::mid); if (otherthancoincident || coincidentonmidpoint) newcVals.erase(newcVals.begin() + index); @@ -9812,7 +9804,7 @@ void SketchObject::getConstraintIndices(int GeoId, std::vector& constraintL int i = 0; for (const auto& constr : constraints) { - if (constr->First == GeoId || constr->Second == GeoId || constr->Third == GeoId) { + if (constr->involvesGeoId(GeoId)) { constraintList.push_back(i); } ++i; From 42caab0037f6f5dbd7ee3acd2593607ec5d9f073 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 31 Aug 2025 15:48:44 +0530 Subject: [PATCH 10/23] Sketcher: [test] Add test for `addExternal` and `delExternal` --- .../Mod/Sketcher/App/SketchObjectChanges.cpp | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp b/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp index 7d777b1ba1..a0b20603aa 100644 --- a/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp +++ b/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp @@ -8,12 +8,81 @@ #include #include #include +#include #include #include #include "SketcherTestHelpers.h" using namespace SketcherTestHelpers; +TEST_F(SketchObjectTest, testAddExternalIncreasesCount) +{ + // Arrange + auto* doc = getObject()->getDocument(); + auto box {doc->addObject("Part::Box")}; + int numboxes = doc->countObjectsOfType(); + int numExtPre = getObject()->ExternalGeo.getSize(); + doc->recompute(); + + // Act + getObject()->addExternal(box, "Face6"); + int numExt = getObject()->ExternalGeo.getSize(); + + // Assert + EXPECT_TRUE(numExt > numExtPre); +} + +TEST_F(SketchObjectTest, testDelExternalUndef) +{ + // Act + int res = getObject()->delExternal(Sketcher::GeoEnum::GeoUndef); + + // Assert + EXPECT_EQ(res, -1); +} + +TEST_F(SketchObjectTest, testDelExternalWhenEmpty) +{ + // Act + int res = getObject()->delExternal(Sketcher::GeoEnum::RefExt); + + // Assert + EXPECT_EQ(res, -1); +} + +TEST_F(SketchObjectTest, testDelExternalWhenEmptyWithPositiveId) +{ + // Act + int res = getObject()->delExternal(1); + + // Assert + EXPECT_EQ(res, -1); +} + +TEST_F(SketchObjectTest, testDelExternalReducesCount) +{ + // Arrange + auto* doc = getObject()->getDocument(); + auto box {doc->addObject("Part::Box")}; + doc->recompute(); + // NOTE: When adding, say, `Face6` instead, `delExternal` removes all the edges. This could be + // intended behaviour, ensuring that adding a face always gives a closed loop, or a side effect, + // or should instead be an option at time of external geometry creation. + // TODO: Consider whether this should be intended behaviour, and add tests accordingly. + getObject()->addExternal(box, "Edge6"); + getObject()->addExternal(box, "Edge4"); + int numExt = getObject()->ExternalGeo.getSize(); + + // Act + int res = getObject()->delExternal(Sketcher::GeoEnum::RefExt); + + // Assert + EXPECT_EQ(getObject()->ExternalGeo.getSize(), numExt - 1); +} + +// TODO: `delExternal` situation of constraints +// TODO: `delExternal` situation of constraint containing more than 3 entities + TEST_F(SketchObjectTest, testReplaceGeometriesOneToOne) { // Arrange From d31ddccd029133c2eb33fe0e737762035d8ddc7c Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Tue, 13 Aug 2024 20:35:54 +0530 Subject: [PATCH 11/23] [Sketcher] Refactor `delExternalPrivate()` --- src/Mod/Sketcher/App/SketchObject.cpp | 135 ++++++++++++-------------- 1 file changed, 60 insertions(+), 75 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 9e7a437efc..ec7376df61 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -7580,112 +7580,97 @@ int SketchObject::delExternal(const std::vector& ExtGeoIds) return 0; } -void SketchObject::delExternalPrivate(const std::set &ids, bool removeRef) { - - Base::StateLocker lock(managedoperation, true); // no need to check input data validity as this is an sketchobject managed operation. +// clang-format on +void SketchObject::delExternalPrivate(const std::set& ids, bool removeRef) +{ + Base::StateLocker lock(managedoperation, true); // no need to check input data validity as this + // is an sketchobject managed operation. std::set refs; // Must sort in reverse order so as to delete geo from back to front to // avoid index change std::set> geoIds; - for(auto id : ids) { + for (auto id : ids) { auto it = externalGeoMap.find(id); - if(it == externalGeoMap.end()) + if (it == externalGeoMap.end()) { continue; + } auto egf = ExternalGeometryFacade::getFacade(ExternalGeo[it->second]); - if(removeRef && egf->getRef().size()) + if (removeRef && egf->getRef().size()) { refs.insert(egf->getRef()); - geoIds.insert(-it->second-1); + } + geoIds.insert(-it->second - 1); } - if(geoIds.empty()) + if (geoIds.empty()) { return; + } - std::vector< Constraint * > newConstraints; - for(auto cstr : Constraints.getValues()) { - if(!geoIds.count(cstr->First) && - (cstr->Second==GeoEnum::GeoUndef || !geoIds.count(cstr->Second)) && - (cstr->Third==GeoEnum::GeoUndef || !geoIds.count(cstr->Third))) - { - bool cloned = false; - int offset = 0; - for(auto GeoId : geoIds) { - GeoId += offset++; - bool done = true; - if (cstr->First < GeoId && cstr->First != GeoEnum::GeoUndef) { - if (!cloned) { - cloned = true; - cstr = cstr->clone(); - } - cstr->First += 1; - done = false; - } - if (cstr->Second < GeoId && cstr->Second != GeoEnum::GeoUndef) { - if (!cloned) { - cloned = true; - cstr = cstr->clone(); - } - cstr->Second += 1; - done = false; - } - if (cstr->Third < GeoId && cstr->Third != GeoEnum::GeoUndef) { - if (!cloned) { - cloned = true; - cstr = cstr->clone(); - } - cstr->Third += 1; - done = false; - } - if(done) break; - } - newConstraints.push_back(cstr); + std::vector newConstraints; + for (const auto& cstr : Constraints.getValues()) { + if (geoIds.count(cstr->First) + || (cstr->Second != GeoEnum::GeoUndef && geoIds.count(cstr->Second)) + || (cstr->Third != GeoEnum::GeoUndef && geoIds.count(cstr->Third))) { + continue; } + int offset = 0; + std::unique_ptr newCstr(cstr->clone()); + for (auto GeoId : geoIds) { + GeoId += offset++; + if (newCstr->First >= GeoId && newCstr->Second >= GeoId && newCstr->Third >= GeoId) { + break; + } + changeConstraintAfterDeletingGeo(newCstr.get(), GeoId); + } + // need to provide raw pointer because that's the only one supported by `setValues` + newConstraints.push_back(newCstr.release()); } auto geos = ExternalGeo.getValues(); int offset = 0; - for(auto geoId : geoIds) { - int idx = -geoId-1; - geos.erase(geos.begin()+idx-offset); + for (auto geoId : geoIds) { + int idx = -geoId - 1; + geos.erase(geos.begin() + idx - offset); ++offset; } - if(refs.size()) { - std::vector newSubs; - std::vector newObjs; - const auto &subs = ExternalGeometry.getSubValues(); - auto itSub = subs.begin(); - const auto &objs = ExternalGeometry.getValues(); - auto itObj = objs.begin(); - bool touched = false; - assert(externalGeoRef.size() == objs.size()); - assert(externalGeoRef.size() == subs.size()); - for(auto it=externalGeoRef.begin();it!=externalGeoRef.end();++it,++itObj,++itSub) { - if(refs.count(*it)) { - if(!touched) { - touched = true; - if(newObjs.empty()) { - newObjs.insert(newObjs.end(),objs.begin(),itObj); - newSubs.insert(newSubs.end(),subs.begin(),itSub); - } - } - }else if(touched) { - newObjs.push_back(*itObj); - newSubs.push_back(*itSub); - } + if (refs.empty()) { + ExternalGeo.setValues(std::move(geos)); + + solverNeedsUpdate = true; + Constraints.setValues(std::move(newConstraints)); + acceptGeometry(); // This may need to be refactored into OnChanged for ExternalGeometry. + } + + std::vector newSubs; + std::vector newObjs; + const auto& subs = ExternalGeometry.getSubValues(); + auto itSub = subs.begin(); + const auto& objs = ExternalGeometry.getValues(); + auto itObj = objs.begin(); + bool touched = false; + assert(externalGeoRef.size() == objs.size()); + assert(externalGeoRef.size() == subs.size()); + for (auto it = externalGeoRef.begin(); it != externalGeoRef.end(); ++it, ++itObj, ++itSub) { + if (refs.count(*it) == 0) { + touched = true; + newObjs.push_back(*itObj); + newSubs.push_back(*itSub); } - if(touched) - ExternalGeometry.setValues(newObjs,newSubs); + } + if (touched) { + ExternalGeometry.setValues(newObjs, newSubs); } ExternalGeo.setValues(std::move(geos)); solverNeedsUpdate = true; Constraints.setValues(std::move(newConstraints)); - acceptGeometry(); // This may need to be refactored into OnChanged for ExternalGeometry. + acceptGeometry(); // This may need to be refactored into OnChanged for ExternalGeometry. } +// clang-format off // clang-format on int SketchObject::delAllExternal() From f3616304d8ea381442b50aedf899b0b6e288bfdf Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Thu, 22 Aug 2024 21:52:27 +0530 Subject: [PATCH 12/23] [Sketcher] Refactor `addSymmetric` and `getSymmetric` Rearrange some if-else and use `std::find_if` instead of looping. [Sketcher][WIP] Refactor `getSymmetric` and `addSymmetric` --- src/Mod/Sketcher/App/SketchObject.cpp | 542 +++++++++++++------------- 1 file changed, 272 insertions(+), 270 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index ec7376df61..44c2507bc6 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -4319,8 +4319,11 @@ bool SketchObject::isCarbonCopyAllowed(App::Document* pDoc, App::DocumentObject* return true; } -int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, - Sketcher::PointPos refPosId , bool addSymmetryConstraints ) +// clang-format on +int SketchObject::addSymmetric(const std::vector& geoIdList, + int refGeoId, + Sketcher::PointPos refPosId, + bool addSymmetryConstraints) { // no need to check input data validity as this is an sketchobject managed operation. Base::StateLocker lock(managedoperation, true); @@ -4334,25 +4337,20 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, // Find out if reference is aligned with V or H axis, // if so we can keep Vertical and Horizontal constraints in the mirrored geometry. bool refIsLine = refPosId == Sketcher::PointPos::none; - bool refIsAxisAligned = false; - if (refGeoId == Sketcher::GeoEnum::VAxis || refGeoId == Sketcher::GeoEnum::HAxis || !refIsLine) { - refIsAxisAligned = true; - } - else { - for (auto* constr : constrvals) { - if (constr->First == refGeoId - && (constr->Type == Sketcher::Vertical || constr->Type == Sketcher::Horizontal)) { - refIsAxisAligned = true; - } - } - } + bool refIsAxisAligned = + refGeoId == Sketcher::GeoEnum::VAxis || refGeoId == Sketcher::GeoEnum::HAxis || !refIsLine + || std::ranges::any_of(constrvals, [&refGeoId](auto* constr) { + return constr->First == refGeoId + && (constr->Type == Sketcher::Vertical || constr->Type == Sketcher::Horizontal); + }); - std::vector symgeos = getSymmetric(geoIdList, geoIdMap, isStartEndInverted, refGeoId, refPosId); + std::vector symgeos = + getSymmetric(geoIdList, geoIdMap, isStartEndInverted, refGeoId, refPosId); { addGeometry(symgeos); - for (auto* constr : constrvals) { + for (auto* constr : constrvals) { // we look in the map, because we might have skipped internal alignment geometry auto firstIt = geoIdMap.find(constr->First); @@ -4362,7 +4360,8 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, // if First of constraint is in geoIdList if (addSymmetryConstraints && constr->Type != Sketcher::InternalAlignment) { - // if we are making symmetric constraints, then we don't want to copy all constraints + // if we are making symmetric constraints, then we don't want to copy all + // constraints continue; } @@ -4377,8 +4376,7 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, continue; } if (!refIsAxisAligned - && (constr->Type == Sketcher::DistanceX - || constr->Type == Sketcher::DistanceY + && (constr->Type == Sketcher::DistanceX || constr->Type == Sketcher::DistanceY || constr->Type == Sketcher::Vertical || constr->Type == Sketcher::Horizontal)) { // this includes all non-directional single GeoId constraints, as radius, @@ -4401,15 +4399,26 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, } // Second is also in the list + + auto flipStartEndIfRelevant = [&isStartEndInverted](int geoId, + const Sketcher::PointPos posId, + Sketcher::PointPos& posIdNew) { + if (isStartEndInverted[geoId]) { + if (posId == Sketcher::PointPos::start) { + posIdNew = Sketcher::PointPos::end; + } + else if (posId == Sketcher::PointPos::end) { + posIdNew = Sketcher::PointPos::start; + } + } + }; + if (constr->Third == GeoEnum::GeoUndef) { if (!(constr->Type == Sketcher::Coincident || constr->Type == Sketcher::Perpendicular - || constr->Type == Sketcher::Parallel - || constr->Type == Sketcher::Tangent - || constr->Type == Sketcher::Distance - || constr->Type == Sketcher::Equal - || constr->Type == Sketcher::Angle - || constr->Type == Sketcher::PointOnObject + || constr->Type == Sketcher::Parallel || constr->Type == Sketcher::Tangent + || constr->Type == Sketcher::Distance || constr->Type == Sketcher::Equal + || constr->Type == Sketcher::Angle || constr->Type == Sketcher::PointOnObject || constr->Type == Sketcher::InternalAlignment)) { continue; } @@ -4418,24 +4427,14 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, constNew->Name = ""; constNew->First = firstIt->second; constNew->Second = secondIt->second; - if (isStartEndInverted[constr->First]) { - if (constr->FirstPos == Sketcher::PointPos::start) - constNew->FirstPos = Sketcher::PointPos::end; - else if (constr->FirstPos == Sketcher::PointPos::end) - constNew->FirstPos = Sketcher::PointPos::start; - } - if (isStartEndInverted[constr->Second]) { - if (constr->SecondPos == Sketcher::PointPos::start) - constNew->SecondPos = Sketcher::PointPos::end; - else if (constr->SecondPos == Sketcher::PointPos::end) - constNew->SecondPos = Sketcher::PointPos::start; - } + flipStartEndIfRelevant(constr->First, constr->FirstPos, constNew->FirstPos); + flipStartEndIfRelevant(constr->Second, constr->SecondPos, constNew->SecondPos); - if (constNew->Type == Tangent || constNew->Type == Perpendicular) + if (constNew->Type == Tangent || constNew->Type == Perpendicular) { AutoLockTangencyAndPerpty(constNew, true); + } - if ((constr->Type == Sketcher::Angle) - && (refPosId == Sketcher::PointPos::none)) { + if ((constr->Type == Sketcher::Angle) && (refPosId == Sketcher::PointPos::none)) { constNew->setValue(-constr->getValue()); } @@ -4456,30 +4455,27 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, constNew->First = firstIt->second; constNew->Second = secondIt->second; constNew->Third = thirdIt->second; - if (isStartEndInverted[constr->First]) { - if (constr->FirstPos == Sketcher::PointPos::start) - constNew->FirstPos = Sketcher::PointPos::end; - else if (constr->FirstPos == Sketcher::PointPos::end) - constNew->FirstPos = Sketcher::PointPos::start; - } - if (isStartEndInverted[constr->Second]) { - if (constr->SecondPos == Sketcher::PointPos::start) - constNew->SecondPos = Sketcher::PointPos::end; - else if (constr->SecondPos == Sketcher::PointPos::end) - constNew->SecondPos = Sketcher::PointPos::start; - } - if (isStartEndInverted[constr->Third]) { - if (constr->ThirdPos == Sketcher::PointPos::start) - constNew->ThirdPos = Sketcher::PointPos::end; - else if (constr->ThirdPos == Sketcher::PointPos::end) - constNew->ThirdPos = Sketcher::PointPos::start; - } + flipStartEndIfRelevant(constr->First, constr->FirstPos, constNew->FirstPos); + flipStartEndIfRelevant(constr->Second, constr->SecondPos, constNew->SecondPos); + flipStartEndIfRelevant(constr->Third, constr->ThirdPos, constNew->ThirdPos); newconstrVals.push_back(constNew); } - if (addSymmetryConstraints) { - auto createSymConstr = [&] - (int first, int second, Sketcher::PointPos firstPos, Sketcher::PointPos secondPos) { + if (!addSymmetryConstraints) { + if (newconstrVals.size() > constrvals.size()) { + Constraints.setValues(std::move(newconstrVals)); + } + + // we delayed update, so trigger it now. + // Update geometry indices and rebuild vertexindex now via onChanged, so that + // ViewProvider::UpdateData is triggered. + Geometry.touch(); + + return Geometry.getSize() - 1; + } + + auto createSymConstr = + [&](int first, int second, Sketcher::PointPos firstPos, Sketcher::PointPos secondPos) { auto symConstr = new Constraint(); symConstr->Type = Symmetric; symConstr->First = first; @@ -4490,52 +4486,57 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, symConstr->ThirdPos = refPosId; newconstrVals.push_back(symConstr); }; - auto createEqualityConstr = [&] - (int first, int second) { - auto symConstr = new Constraint(); - symConstr->Type = Equal; - symConstr->First = first; - symConstr->Second = second; - newconstrVals.push_back(symConstr); - }; + auto createEqualityConstr = [&](int first, int second) { + auto symConstr = new Constraint(); + symConstr->Type = Equal; + symConstr->First = first; + symConstr->Second = second; + newconstrVals.push_back(symConstr); + }; - for (auto geoIdPair : geoIdMap) { - int geoId1 = geoIdPair.first; - int geoId2 = geoIdPair.second; - const Part::Geometry* geo = getGeometry(geoId1); + for (auto geoIdPair : geoIdMap) { + int geoId1 = geoIdPair.first; + int geoId2 = geoIdPair.second; + const Part::Geometry* geo = getGeometry(geoId1); - if (geo->is()) { - auto gf = GeometryFacade::getFacade(geo); - if (!gf->isInternalAligned()) { - // Note internal aligned lines (ellipse, parabola, hyperbola) are causing redundant constraint. - createSymConstr(geoId1, geoId2, PointPos::start, isStartEndInverted[geoId1] ? PointPos::end : PointPos::start); - createSymConstr(geoId1, geoId2, PointPos::end, isStartEndInverted[geoId1] ? PointPos::start : PointPos::end); - } + if (geo->is()) { + auto gf = GeometryFacade::getFacade(geo); + if (!gf->isInternalAligned()) { + // Note internal aligned lines (ellipse, parabola, hyperbola) are causing + // redundant constraint. + createSymConstr(geoId1, + geoId2, + PointPos::start, + isStartEndInverted[geoId1] ? PointPos::end : PointPos::start); + createSymConstr(geoId1, + geoId2, + PointPos::end, + isStartEndInverted[geoId1] ? PointPos::start : PointPos::end); } - else if (geo->is() || geo->is()) { - createEqualityConstr(geoId1, geoId2); - createSymConstr(geoId1, geoId2, PointPos::mid, PointPos::mid); - } - else if (geo->is() - || geo->is() - || geo->is() - || geo->is()) { - createEqualityConstr(geoId1, geoId2); - createSymConstr(geoId1, geoId2, PointPos::start, isStartEndInverted[geoId1] ? PointPos::end : PointPos::start); - createSymConstr(geoId1, geoId2, PointPos::end, isStartEndInverted[geoId1] ? PointPos::start : PointPos::end); - } - else if (geo->is()) { - auto gf = GeometryFacade::getFacade(geo); - if (!gf->isInternalAligned()) { - createSymConstr(geoId1, geoId2, PointPos::start, PointPos::start); - } - } - // Note bspline has symmetric by the internal aligned circles. } - } - - if (newconstrVals.size() > constrvals.size()){ - Constraints.setValues(std::move(newconstrVals)); + else if (geo->is() || geo->is()) { + createEqualityConstr(geoId1, geoId2); + createSymConstr(geoId1, geoId2, PointPos::mid, PointPos::mid); + } + else if (geo->is() || geo->is() + || geo->is() || geo->is()) { + createEqualityConstr(geoId1, geoId2); + createSymConstr(geoId1, + geoId2, + PointPos::start, + isStartEndInverted[geoId1] ? PointPos::end : PointPos::start); + createSymConstr(geoId1, + geoId2, + PointPos::end, + isStartEndInverted[geoId1] ? PointPos::start : PointPos::end); + } + else if (geo->is()) { + auto gf = GeometryFacade::getFacade(geo); + if (!gf->isInternalAligned()) { + createSymConstr(geoId1, geoId2, PointPos::start, PointPos::start); + } + } + // Note bspline has symmetric by the internal aligned circles. } } @@ -4548,10 +4549,10 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, int refGeoId, } std::vector SketchObject::getSymmetric(const std::vector& geoIdList, - std::map& geoIdMap, - std::map& isStartEndInverted, - int refGeoId, - Sketcher::PointPos refPosId) + std::map& geoIdMap, + std::map& isStartEndInverted, + int refGeoId, + Sketcher::PointPos refPosId) { using std::numbers::pi; @@ -4567,13 +4568,11 @@ std::vector SketchObject::getSymmetric(const std::vector& } // only add if the corresponding geometry it defines is also in the list. - int definedGeo = GeoEnum::GeoUndef; - for (auto c : Constraints.getValues()) { - if (c->Type == Sketcher::InternalAlignment && c->First == geoId) { - definedGeo = c->Second; - break; - } - } + const auto& constraints = Constraints.getValues(); + auto constrIt = std::ranges::find_if(constraints, [&geoId](auto* c) { + return c->Type == Sketcher::InternalAlignment && c->First == geoId; + }); + int definedGeo = (constrIt == constraints.end()) ? GeoEnum::GeoUndef : (*constrIt)->Second; // Return true if definedGeo is in geoIdList, false otherwise return std::ranges::find(geoIdList, definedGeo) != geoIdList.end(); }; @@ -4631,8 +4630,10 @@ std::vector SketchObject::getSymmetric(const std::vector& Base::Vector3d scp = cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); - double theta1 = Base::fmod(atan2(sep.y - scp.y, sep.x - scp.x), 2.f * std::numbers::pi); - double theta2 = Base::fmod(atan2(ssp.y - scp.y, ssp.x - scp.x), 2.f * std::numbers::pi); + double theta1 = + Base::fmod(atan2(sep.y - scp.y, sep.x - scp.x), 2.f * std::numbers::pi); + double theta2 = + Base::fmod(atan2(ssp.y - scp.y, ssp.x - scp.x), 2.f * std::numbers::pi); geoaoc->setCenter(scp); geoaoc->setRange(theta1, theta2, true); @@ -4772,170 +4773,171 @@ std::vector SketchObject::getSymmetric(const std::vector& geoIdMap.insert(std::make_pair(geoId, cgeoid)); cgeoid++; } - } - else {// reference is a point - Vector3d refpoint; - const Part::Geometry* georef = getGeometry(refGeoId); - if (georef->is()) { - refpoint = static_cast(georef)->getPoint(); + return symmetricVals; + } + + // reference is a point + Vector3d refpoint; + const Part::Geometry* georef = getGeometry(refGeoId); + + if (georef->is()) { + refpoint = static_cast(georef)->getPoint(); + } + else if (refGeoId == -1 && refPosId == Sketcher::PointPos::start) { + refpoint = Vector3d(0, 0, 0); + } + else { + if (refPosId == Sketcher::PointPos::none) { + Base::Console().error("Wrong PointPosId.\n"); + return {}; } - else if (refGeoId == -1 && refPosId == Sketcher::PointPos::start) { - refpoint = Vector3d(0, 0, 0); + refpoint = getPoint(georef, refPosId); + } + + for (auto geoId : geoIdList) { + const Part::Geometry* geo = getGeometry(geoId); + Part::Geometry* geosym; + + if (!shouldCopyGeometry(geo, geoId)) { + continue; + } + + geosym = geo->copy(); + + // Handle Geometry + if (geosym->is()) { + auto* geosymline = static_cast(geosym); + Base::Vector3d sp = geosymline->getStartPoint(); + Base::Vector3d ep = geosymline->getEndPoint(); + Base::Vector3d ssp = sp + 2.0 * (refpoint - sp); + Base::Vector3d sep = ep + 2.0 * (refpoint - ep); + + geosymline->setPoints(ssp, sep); + isStartEndInverted.insert(std::make_pair(geoId, false)); + } + else if (geosym->is()) { + auto* geosymcircle = static_cast(geosym); + Base::Vector3d cp = geosymcircle->getCenter(); + + geosymcircle->setCenter(cp + 2.0 * (refpoint - cp)); + isStartEndInverted.insert(std::make_pair(geoId, false)); + } + else if (geosym->is()) { + auto* geoaoc = static_cast(geosym); + Base::Vector3d sp = geoaoc->getStartPoint(true); + Base::Vector3d ep = geoaoc->getEndPoint(true); + Base::Vector3d cp = geoaoc->getCenter(); + + Base::Vector3d ssp = sp + 2.0 * (refpoint - sp); + Base::Vector3d sep = ep + 2.0 * (refpoint - ep); + Base::Vector3d scp = cp + 2.0 * (refpoint - cp); + + double theta1 = Base::fmod(atan2(ssp.y - scp.y, ssp.x - scp.x), 2.f * pi); + double theta2 = Base::fmod(atan2(sep.y - scp.y, sep.x - scp.x), 2.f * pi); + + geoaoc->setCenter(scp); + geoaoc->setRange(theta1, theta2, true); + isStartEndInverted.insert(std::make_pair(geoId, false)); + } + else if (geosym->is()) { + auto* geosymellipse = static_cast(geosym); + Base::Vector3d cp = geosymellipse->getCenter(); + + Base::Vector3d majdir = geosymellipse->getMajorAxisDir(); + double majord = geosymellipse->getMajorRadius(); + double minord = geosymellipse->getMinorRadius(); + double df = sqrt(majord * majord - minord * minord); + Base::Vector3d f1 = cp + df * majdir; + + Base::Vector3d sf1 = f1 + 2.0 * (refpoint - f1); + Base::Vector3d scp = cp + 2.0 * (refpoint - cp); + + geosymellipse->setMajorAxisDir(sf1 - scp); + + geosymellipse->setCenter(scp); + isStartEndInverted.insert(std::make_pair(geoId, false)); + } + else if (geosym->is()) { + auto* geosymaoe = static_cast(geosym); + Base::Vector3d cp = geosymaoe->getCenter(); + + Base::Vector3d majdir = geosymaoe->getMajorAxisDir(); + double majord = geosymaoe->getMajorRadius(); + double minord = geosymaoe->getMinorRadius(); + double df = sqrt(majord * majord - minord * minord); + Base::Vector3d f1 = cp + df * majdir; + + Base::Vector3d sf1 = f1 + 2.0 * (refpoint - f1); + Base::Vector3d scp = cp + 2.0 * (refpoint - cp); + + geosymaoe->setMajorAxisDir(sf1 - scp); + + geosymaoe->setCenter(scp); + isStartEndInverted.insert(std::make_pair(geoId, false)); + } + else if (geosym->is()) { + auto* geosymaoe = static_cast(geosym); + Base::Vector3d cp = geosymaoe->getCenter(); + + Base::Vector3d majdir = geosymaoe->getMajorAxisDir(); + double majord = geosymaoe->getMajorRadius(); + double minord = geosymaoe->getMinorRadius(); + double df = sqrt(majord * majord + minord * minord); + Base::Vector3d f1 = cp + df * majdir; + + Base::Vector3d sf1 = f1 + 2.0 * (refpoint - f1); + Base::Vector3d scp = cp + 2.0 * (refpoint - cp); + + geosymaoe->setMajorAxisDir(sf1 - scp); + + geosymaoe->setCenter(scp); + isStartEndInverted.insert(std::make_pair(geoId, false)); + } + else if (geosym->is()) { + auto* geosymaoe = static_cast(geosym); + Base::Vector3d cp = geosymaoe->getCenter(); + Base::Vector3d f1 = geosymaoe->getFocus(); + + Base::Vector3d sf1 = f1 + 2.0 * (refpoint - f1); + Base::Vector3d scp = cp + 2.0 * (refpoint - cp); + + geosymaoe->setXAxisDir(sf1 - scp); + geosymaoe->setCenter(scp); + + isStartEndInverted.insert(std::make_pair(geoId, false)); + } + else if (geosym->is()) { + auto* geosymbsp = static_cast(geosym); + + std::vector poles = geosymbsp->getPoles(); + + for (auto& pole : poles) { + pole = pole + 2.0 * (refpoint - pole); + } + + geosymbsp->setPoles(poles); + } + else if (geosym->is()) { + auto* geosympoint = static_cast(geosym); + Base::Vector3d cp = geosympoint->getPoint(); + + geosympoint->setPoint(cp + 2.0 * (refpoint - cp)); + isStartEndInverted.insert(std::make_pair(geoId, false)); } else { - if (refPosId == Sketcher::PointPos::none) { - Base::Console().error("Wrong PointPosId.\n"); - return {}; - } - refpoint = getPoint(georef, refPosId); + Base::Console().error("Unsupported Geometry!! Just copying it.\n"); + isStartEndInverted.insert(std::make_pair(geoId, false)); } - for (auto geoId : geoIdList) { - const Part::Geometry* geo = getGeometry(geoId); - Part::Geometry* geosym; - - if (!shouldCopyGeometry(geo, geoId)) { - continue; - } - - geosym = geo->copy(); - - // Handle Geometry - if (geosym->is()) { - auto* geosymline = static_cast(geosym); - Base::Vector3d sp = geosymline->getStartPoint(); - Base::Vector3d ep = geosymline->getEndPoint(); - Base::Vector3d ssp = sp + 2.0 * (refpoint - sp); - Base::Vector3d sep = ep + 2.0 * (refpoint - ep); - - geosymline->setPoints(ssp, sep); - isStartEndInverted.insert(std::make_pair(geoId, false)); - } - else if (geosym->is()) { - auto* geosymcircle = static_cast(geosym); - Base::Vector3d cp = geosymcircle->getCenter(); - - geosymcircle->setCenter(cp + 2.0 * (refpoint - cp)); - isStartEndInverted.insert(std::make_pair(geoId, false)); - } - else if (geosym->is()) { - auto* geoaoc = static_cast(geosym); - Base::Vector3d sp = geoaoc->getStartPoint(true); - Base::Vector3d ep = geoaoc->getEndPoint(true); - Base::Vector3d cp = geoaoc->getCenter(); - - Base::Vector3d ssp = sp + 2.0 * (refpoint - sp); - Base::Vector3d sep = ep + 2.0 * (refpoint - ep); - Base::Vector3d scp = cp + 2.0 * (refpoint - cp); - - double theta1 = Base::fmod(atan2(ssp.y - scp.y, ssp.x - scp.x), 2.f * pi); - double theta2 = Base::fmod(atan2(sep.y - scp.y, sep.x - scp.x), 2.f * pi); - - geoaoc->setCenter(scp); - geoaoc->setRange(theta1, theta2, true); - isStartEndInverted.insert(std::make_pair(geoId, false)); - } - else if (geosym->is()) { - auto* geosymellipse = static_cast(geosym); - Base::Vector3d cp = geosymellipse->getCenter(); - - Base::Vector3d majdir = geosymellipse->getMajorAxisDir(); - double majord = geosymellipse->getMajorRadius(); - double minord = geosymellipse->getMinorRadius(); - double df = sqrt(majord * majord - minord * minord); - Base::Vector3d f1 = cp + df * majdir; - - Base::Vector3d sf1 = f1 + 2.0 * (refpoint - f1); - Base::Vector3d scp = cp + 2.0 * (refpoint - cp); - - geosymellipse->setMajorAxisDir(sf1 - scp); - - geosymellipse->setCenter(scp); - isStartEndInverted.insert(std::make_pair(geoId, false)); - } - else if (geosym->is()) { - auto* geosymaoe = static_cast(geosym); - Base::Vector3d cp = geosymaoe->getCenter(); - - Base::Vector3d majdir = geosymaoe->getMajorAxisDir(); - double majord = geosymaoe->getMajorRadius(); - double minord = geosymaoe->getMinorRadius(); - double df = sqrt(majord * majord - minord * minord); - Base::Vector3d f1 = cp + df * majdir; - - Base::Vector3d sf1 = f1 + 2.0 * (refpoint - f1); - Base::Vector3d scp = cp + 2.0 * (refpoint - cp); - - geosymaoe->setMajorAxisDir(sf1 - scp); - - geosymaoe->setCenter(scp); - isStartEndInverted.insert(std::make_pair(geoId, false)); - } - else if (geosym->is()) { - auto* geosymaoe = static_cast(geosym); - Base::Vector3d cp = geosymaoe->getCenter(); - - Base::Vector3d majdir = geosymaoe->getMajorAxisDir(); - double majord = geosymaoe->getMajorRadius(); - double minord = geosymaoe->getMinorRadius(); - double df = sqrt(majord * majord + minord * minord); - Base::Vector3d f1 = cp + df * majdir; - - Base::Vector3d sf1 = f1 + 2.0 * (refpoint - f1); - Base::Vector3d scp = cp + 2.0 * (refpoint - cp); - - geosymaoe->setMajorAxisDir(sf1 - scp); - - geosymaoe->setCenter(scp); - isStartEndInverted.insert(std::make_pair(geoId, false)); - } - else if (geosym->is()) { - auto* geosymaoe = static_cast(geosym); - Base::Vector3d cp = geosymaoe->getCenter(); - Base::Vector3d f1 = geosymaoe->getFocus(); - - Base::Vector3d sf1 = f1 + 2.0 * (refpoint - f1); - Base::Vector3d scp = cp + 2.0 * (refpoint - cp); - - geosymaoe->setXAxisDir(sf1 - scp); - geosymaoe->setCenter(scp); - - isStartEndInverted.insert(std::make_pair(geoId, false)); - } - else if (geosym->is()) { - auto* geosymbsp = static_cast(geosym); - - std::vector poles = geosymbsp->getPoles(); - - for (auto& pole : poles) { - pole = pole + 2.0 * (refpoint - pole); - } - - geosymbsp->setPoles(poles); - - } - else if (geosym->is()) { - auto* geosympoint = static_cast(geosym); - Base::Vector3d cp = geosympoint->getPoint(); - - geosympoint->setPoint(cp + 2.0 * (refpoint - cp)); - isStartEndInverted.insert(std::make_pair(geoId, false)); - } - else { - Base::Console().error("Unsupported Geometry!! Just copying it.\n"); - isStartEndInverted.insert(std::make_pair(geoId, false)); - } - - symmetricVals.push_back(geosym); - geoIdMap.insert(std::make_pair(geoId, cgeoid)); - cgeoid++; - } + symmetricVals.push_back(geosym); + geoIdMap.insert(std::make_pair(geoId, cgeoid)); + cgeoid++; } + return symmetricVals; } -// clang-format on int SketchObject::addCopy(const std::vector& geoIdList, const Base::Vector3d& displacement, bool moveonly, From 973b7d778cbc6346c59e6d688e4158582ca0f9e5 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Fri, 23 Aug 2024 05:17:42 +0530 Subject: [PATCH 13/23] [Sketcher] Refactor some if-else statements in SketchObject --- src/Mod/Sketcher/App/SketchObject.cpp | 93 ++++++++++++++++----------- 1 file changed, 54 insertions(+), 39 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 44c2507bc6..9db00e34da 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -11079,7 +11079,8 @@ int SketchObject::port_reversedExternalArcs(bool justAnalyze) for (std::size_t ic = 0; ic < newVals.size(); ic++) {// ic = index of constraint bool affected = false; Constraint* constNew = nullptr; - for (int ig = 1; ig <= 3; ig++) {// cycle through constraint.first, second, third + for (int ig = 1; ig <= 3; ig++) { + // cycle through constraint.first, second, third int geoId = 0; Sketcher::PointPos posId = PointPos::none; switch (ig) { @@ -11102,8 +11103,7 @@ int SketchObject::port_reversedExternalArcs(bool justAnalyze) // we are dealing with a link to an endpoint of external geom Part::Geometry* g = this->ExternalGeo[-geoId - 1]; if (g->is()) { - const Part::GeomArcOfCircle* segm = - static_cast(g); + const auto* segm = static_cast(g); if (segm->isReversed()) { // Gotcha! a link to an endpoint of external arc that is reversed. // create a constraint copy, affect it, replace the pointer @@ -11263,8 +11263,9 @@ App::DocumentObject *SketchObject::getSubObject( while(subname && *subname=='.') ++subname; // skip leading . std::string sub; const char *mapped = Data::isMappedElement(subname); - if(!subname || !subname[0]) + if(!subname || !subname[0]) { return Part2DObject::getSubObject(subname,pyObj,pmat,transform,depth); + } const char *element = Data::findElementName(subname); if(element != subname) { const char *dot = strchr(subname,'.'); @@ -11297,11 +11298,12 @@ App::DocumentObject *SketchObject::getSubObject( } else if (!pyObj || !mapped) { if (!pyObj - || (index > 0 - && !boost::algorithm::contains(subname, "edge") - && !boost::algorithm::contains(subname, "vertex"))) + || (index > 0 + && !boost::algorithm::contains(subname, "edge") + && !boost::algorithm::contains(subname, "vertex"))) return Part2DObject::getSubObject(subname,pyObj,pmat,transform,depth); - } else { + } + else { subshape = Shape.getShape().getSubTopoShape(subname, true); if (!subshape.isNull()) return Part2DObject::getSubObject(subname,pyObj,pmat,transform,depth); @@ -11313,14 +11315,16 @@ App::DocumentObject *SketchObject::getSubObject( geo = getGeometry(index - 1); if (!geo) return nullptr; - } else if (boost::equals(shapetype,"ExternalEdge")) { + } + else if (boost::equals(shapetype,"ExternalEdge")) { int GeoId = index - 1; GeoId = -GeoId - 3; geo = getGeometry(GeoId); if(!geo) return nullptr; - } else if (boost::equals(shapetype,"Vertex") || - boost::equals(shapetype,"vertex")) { + } + else if (boost::equals(shapetype,"Vertex") || + boost::equals(shapetype,"vertex")) { int VtId = index- 1; int GeoId; PointPos PosId; @@ -11343,41 +11347,51 @@ App::DocumentObject *SketchObject::getSubObject( if(pyObj) *pyObj = vals[ConstrId]->getPyObject(); return const_cast(this); - } else + } + else { return nullptr; + } } if (pmat && transform) *pmat *= Placement.getValue().toMatrix(); - if (pyObj) { - Part::TopoShape shape; - std::string name = convertSubName(indexedName,false); - if (geo) { - shape = getEdge(geo,name.c_str()); - if(pmat && !shape.isNull()) - shape.transformShape(*pmat,false,true); - } else if (!subshape.isNull()) { - shape = subshape; - if (pmat) - shape.transformShape(*pmat,false,true); - } else { - if(pmat) - point = (*pmat)*point; - shape = BRepBuilderAPI_MakeVertex(gp_Pnt(point.x,point.y,point.z)).Vertex(); - // Originally in ComplexGeoData::setElementName - // LinkStable/src/App/ComplexGeoData.cpp#L1631 - // No longer possible after map separated in ElementMap.cpp - if ( !shape.hasElementMap() ) { - shape.resetElementMap(std::make_shared()); - } - shape.setElementName(Data::IndexedName::fromConst("Vertex", 1), - Data::MappedName::fromRawData(name.c_str()),0); - } - shape.Tag = getID(); - *pyObj = Py::new_reference_to(Part::shape2pyshape(shape)); + if (!pyObj) { + return const_cast(this); } + // pyObj exists from here + Part::TopoShape shape; + std::string name = convertSubName(indexedName,false); + if (geo) { + shape = getEdge(geo,name.c_str()); + if(pmat && !shape.isNull()) { + shape.transformShape(*pmat,false,true); + } + } + else if (!subshape.isNull()) { + shape = subshape; + if (pmat) { + shape.transformShape(*pmat,false,true); + } + } + else { + if(pmat) { + point = (*pmat)*point; + } + shape = BRepBuilderAPI_MakeVertex(gp_Pnt(point.x,point.y,point.z)).Vertex(); + // Originally in ComplexGeoData::setElementName + // LinkStable/src/App/ComplexGeoData.cpp#L1631 + // No longer possible after map separated in ElementMap.cpp + if (!shape.hasElementMap()) { + shape.resetElementMap(std::make_shared()); + } + shape.setElementName(Data::IndexedName::fromConst("Vertex", 1), + Data::MappedName::fromRawData(name.c_str()),0); + } + shape.Tag = getID(); + *pyObj = Py::new_reference_to(Part::shape2pyshape(shape)); + return const_cast(this); } @@ -11570,7 +11584,8 @@ Part::TopoShape SketchObject::getEdge(const Part::Geometry *geo, const char *nam return shape; } -Data::IndexedName SketchObject::checkSubName(const char *subname) const{ +Data::IndexedName SketchObject::checkSubName(const char *subname) const +{ static std::vector types = { "Edge", "Vertex", From cd9e4185495f1a7232a976f0af2b64bc4403fbe4 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Thu, 22 Aug 2024 22:41:31 +0530 Subject: [PATCH 14/23] [Sketcher] Refactor `SketchObject::migrateSketch()` Manipulate some if-else statements, for loops, and possibly replace them with std algorithms. --- src/Mod/Sketcher/App/SketchObject.cpp | 300 +++++++++++++------------- 1 file changed, 154 insertions(+), 146 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 9db00e34da..6639e92af3 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -10814,60 +10814,64 @@ void SketchObject::onSketchRestore() } } +// clang-format on void SketchObject::migrateSketch() { - bool noextensions = false; - for (const auto& g : getInternalGeometry()) - // no extension - legacy file - if (!g->hasExtension(SketchGeometryExtension::getClassTypeId())) - noextensions = true; + const auto& allGeoms = getInternalGeometry(); + bool noextensions = std::ranges::any_of(allGeoms, [](const auto& geo) { + return !geo->hasExtension(SketchGeometryExtension::getClassTypeId()); + }); if (noextensions) { - for (auto c : Constraints.getValues()) { + for (const auto& c : Constraints.getValues()) { addGeometryState(c); // Convert B-Spline controlpoints radius/diameter constraints to Weight constraints - if (c->Type == InternalAlignment && c->AlignmentType == BSplineControlPoint) { - int circlegeoid = c->First; - int bsplinegeoid = c->Second; + if (c->Type != InternalAlignment || c->AlignmentType != BSplineControlPoint) { + continue; + } - auto bsp = static_cast(getGeometry(bsplinegeoid)); + int circleGeoId = c->First; + int bSplineGeoId = c->Second; - std::vector weights = bsp->getWeights(); + auto bsp = static_cast(getGeometry(bSplineGeoId)); - for (auto ccp : Constraints.getValues()) { - if ((ccp->Type == Radius || ccp->Type == Diameter) - && ccp->First == circlegeoid) { - if (c->InternalAlignmentIndex < int(weights.size())) { - ccp->Type = Weight; - ccp->setValue(weights[c->InternalAlignmentIndex]); - } - } + std::vector weights = bsp->getWeights(); + + if (!(c->InternalAlignmentIndex < int(weights.size()))) { + continue; + } + + for (auto& ccp : Constraints.getValues()) { + if ((ccp->Type == Radius || ccp->Type == Diameter) && ccp->First == circleGeoId) { + ccp->Type = Weight; + ccp->setValue(weights[c->InternalAlignmentIndex]); } } } // Construction migration to extension - for (auto g : Geometry.getValues()) { - if (g->hasExtension(Part::GeometryMigrationExtension::getClassTypeId())) { - auto ext = std::static_pointer_cast( - g->getExtension(Part::GeometryMigrationExtension::getClassTypeId()).lock()); - - if (ext->testMigrationType(Part::GeometryMigrationExtension::Construction)) { - // at this point IA geometry is already migrated - auto gf = GeometryFacade::getFacade(g); - - bool oldconstr = ext->getConstruction(); - - if (g->is() && !gf->isInternalAligned()) - oldconstr = true; - - GeometryFacade::setConstruction(g, oldconstr); - } - - g->deleteExtension(Part::GeometryMigrationExtension::getClassTypeId()); + for (auto& g : Geometry.getValues()) { + if (!g->hasExtension(Part::GeometryMigrationExtension::getClassTypeId())) { + continue; } + + auto ext = std::static_pointer_cast( + g->getExtension(Part::GeometryMigrationExtension::getClassTypeId()).lock()); + + if (!ext->testMigrationType(Part::GeometryMigrationExtension::Construction)) { + continue; + } + // at this point IA geometry is already migrated + auto gf = GeometryFacade::getFacade(g); + + bool oldConstr = + ext->getConstruction() || (g->is() && !gf->isInternalAligned()); + + GeometryFacade::setConstruction(g, oldConstr); + + g->deleteExtension(Part::GeometryMigrationExtension::getClassTypeId()); } } @@ -10875,124 +10879,128 @@ void SketchObject::migrateSketch() auto constraints = Constraints.getValues(); auto geometries = getInternalGeometry(); - bool parabolaFound = std::ranges::any_of(geometries, &Part::Geometry::is); + bool parabolaFound = + std::ranges::any_of(geometries, &Part::Geometry::is); - if (parabolaFound) { - bool focalaxisfound = std::ranges::any_of(constraints, [](auto& c) { - return c->Type == InternalAlignment && c->AlignmentType == ParabolaFocalAxis; - }); + if (!parabolaFound) { + return; + } - // 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) { - // maps parabola geoid to focusgeoid - std::map parabolageoid2focusgeoid; + auto focalAxisFound = std::ranges::any_of(constraints, [](auto c) { + return c->Type == InternalAlignment && c->AlignmentType == ParabolaFocalAxis; + }); - // populate parabola and focus geoids - for (const auto& c : constraints) { - if (c->Type == InternalAlignment && c->AlignmentType == ParabolaFocus) { - parabolageoid2focusgeoid[c->Second] = {c->First}; - } - } + if (focalAxisFound) { + return; + } - // maps axis geoid to parabolageoid - std::map axisgeoid2parabolageoid; + // There are parabolas and there isn't an IA axis. (1) there are no axis or (2) there is a + // legacy construction line - // populate axis geoid - for (const auto& [parabolageoid, focusgeoid] : parabolageoid2focusgeoid) { - // look for a line from focusgeoid:start to Geoid:mid_external - std::vector focusgeoidlistgeoidlist; - std::vector focusposidlist; - getDirectlyCoincidentPoints( - focusgeoid, Sketcher::PointPos::start, focusgeoidlistgeoidlist, focusposidlist); + // maps parabola geoid to focusGeoId + std::map parabolaGeoId2FocusGeoId; - std::vector parabgeoidlistgeoidlist; - std::vector parabposidlist; - getDirectlyCoincidentPoints(parabolageoid, - Sketcher::PointPos::mid, - parabgeoidlistgeoidlist, - parabposidlist); - - if (!focusgeoidlistgeoidlist.empty() && !parabgeoidlistgeoidlist.empty()) { - std::size_t i, j; - for (i = 0; i < focusgeoidlistgeoidlist.size(); i++) { - for (j = 0; j < parabgeoidlistgeoidlist.size(); j++) { - if (focusgeoidlistgeoidlist[i] == parabgeoidlistgeoidlist[j]) { - axisgeoid2parabolageoid[focusgeoidlistgeoidlist[i]] = parabolageoid; - } - } - } - } - } - - std::vector newconstraints; - newconstraints.reserve(constraints.size()); - - for (const auto& c : constraints) { - - if (c->Type != Coincident) { - newconstraints.push_back(c); - } - else { - auto axismajorcoincidentfound = std::ranges::any_of(axisgeoid2parabolageoid, - [&](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) { - // we skip this coincident, the other coincident on axis will be substituted - // by internal geometry constraint - continue; - } - - auto focuscoincidentfound = - std::ranges::find_if(axisgeoid2parabolageoid, - [&](const auto& pair) { - auto parabolageoid = pair.second; - auto axisgeoid = pair.first; - auto focusgeoid = parabolageoid2focusgeoid[parabolageoid]; - return (c->First == axisgeoid && c->Second == focusgeoid - && c->SecondPos == PointPos::start) - || (c->Second == axisgeoid && c->First == focusgeoid - && c->FirstPos == PointPos::start); - }); - - if (focuscoincidentfound != axisgeoid2parabolageoid.end()) { - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); - newConstr->Type = Sketcher::InternalAlignment; - newConstr->AlignmentType = Sketcher::ParabolaFocalAxis; - newConstr->First = focuscoincidentfound->first;// axis geoid - newConstr->FirstPos = Sketcher::PointPos::none; - newConstr->Second = focuscoincidentfound->second;// parabola geoid - newConstr->SecondPos = Sketcher::PointPos::none; - newconstraints.push_back(newConstr); - - addGeometryState(newConstr); - - // we skip the coincident, as we have substituted it by internal geometry - // constraint - continue; - } - - newconstraints.push_back(c); - } - } - - Constraints.setValues(std::move(newconstraints)); - - Base::Console().critical( - this->getFullName(), - QT_TRANSLATE_NOOP("Notifications", - "Parabolas were migrated. Migrated files won't open in previous " - "versions of FreeCAD!!\n")); + // populate parabola and focus geoids + for (const auto& c : constraints) { + if (c->Type == InternalAlignment && c->AlignmentType == ParabolaFocus) { + parabolaGeoId2FocusGeoId[c->Second] = {c->First}; } } + + // maps axis geoid to parabolaGeoId + std::map axisGeoId2ParabolaGeoId; + + // populate axis geoid + for (const auto& [parabolaGeoId, focusGeoId] : parabolaGeoId2FocusGeoId) { + // look for a line from focusGeoId:start to Geoid:mid_external + std::vector focusGeoIdListGeoIdList; + std::vector focusPosIdList; + getDirectlyCoincidentPoints(focusGeoId, + Sketcher::PointPos::start, + focusGeoIdListGeoIdList, + focusPosIdList); + + std::vector parabGeoIdListGeoIdList; + std::vector parabposidlist; + getDirectlyCoincidentPoints(parabolaGeoId, + Sketcher::PointPos::mid, + parabGeoIdListGeoIdList, + parabposidlist); + + for (const auto& parabGeoIdListGeoId : parabGeoIdListGeoIdList) { + auto iterParabolaGeoId = + std::ranges::find(focusGeoIdListGeoIdList, parabGeoIdListGeoId); + if (iterParabolaGeoId != focusGeoIdListGeoIdList.end()) { + axisGeoId2ParabolaGeoId[*iterParabolaGeoId] = parabolaGeoId; + } + } + } + + std::vector newConstraints; + newConstraints.reserve(constraints.size()); + + for (const auto& c : constraints) { + if (c->Type != Coincident) { + newConstraints.push_back(c); + continue; + } + + auto axisMajorCoincidentFound = + std::ranges::any_of(axisGeoId2ParabolaGeoId, [&](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) { + // we skip this coincident, the other coincident on axis will be substituted + // by internal geometry constraint + continue; + } + + auto focusCoincidentFound = + std::ranges::find_if(axisGeoId2ParabolaGeoId, [&](const auto& pair) { + auto parabolaGeoId = pair.second; + auto axisgeoid = pair.first; + auto focusGeoId = parabolaGeoId2FocusGeoId[parabolaGeoId]; + return (c->First == axisgeoid && c->Second == focusGeoId + && c->SecondPos == PointPos::start) + || (c->Second == axisgeoid && c->First == focusGeoId + && c->FirstPos == PointPos::start); + }); + + if (focusCoincidentFound != axisGeoId2ParabolaGeoId.end()) { + auto* newConstr = new Sketcher::Constraint(); + newConstr->Type = Sketcher::InternalAlignment; + newConstr->AlignmentType = Sketcher::ParabolaFocalAxis; + newConstr->First = focusCoincidentFound->first; // axis geoid + newConstr->FirstPos = Sketcher::PointPos::none; + newConstr->Second = focusCoincidentFound->second; // parabola geoid + newConstr->SecondPos = Sketcher::PointPos::none; + newConstraints.push_back(newConstr); + + addGeometryState(newConstr); + + // we skip the coincident, as we have substituted it by internal geometry + // constraint + continue; + } + + newConstraints.push_back(c); + } + + Constraints.setValues(std::move(newConstraints)); + + Base::Console().critical( + this->getFullName(), + QT_TRANSLATE_NOOP("Notifications", + "Parabolas were migrated. Migrated files won't open in previous " + "versions of FreeCAD!!\n")); } +// clang-format off void SketchObject::getGeoVertexIndex(int VertexId, int& GeoId, PointPos& PosId) const { From 0d1bf450e9fab00ec1e5fb39a082d711a29aaa15 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Wed, 28 Aug 2024 19:41:01 +0530 Subject: [PATCH 15/23] [Sketcher] Refactor `SketchObject::validateExpression()` --- src/Mod/Sketcher/App/SketchObject.cpp | 58 +++++++++++++++------------ 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 6639e92af3..77d5032a28 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -10037,33 +10037,39 @@ std::string SketchObject::validateExpression(const App::ObjectIdentifier& path, auto deps = expr->getDeps(); auto it = deps.find(this); - if (it != deps.end()) { - auto it2 = it->second.find("Constraints"); - if (it2 != it->second.end()) { - for (auto& oid : it2->second) { - const Constraint* constraint = Constraints.getConstraint(oid); - - if (!constraint->isDriving) - return "Reference constraint from this sketch cannot be used in this " - "expression."; - } - } - geoMap.clear(); - const auto &vals = getInternalGeometry(); - for(long i=0;i<(long)vals.size();++i) { - auto geo = vals[i]; - auto gf = GeometryFacade::getFacade(geo); - if(!gf->getId()) - gf->setId(++geoLastId); - else if(gf->getId() > geoLastId) - geoLastId = gf->getId(); - while(!geoMap.insert(std::make_pair(gf->getId(),i)).second) { - FC_WARN("duplicate geometry id " << gf->getId() << " -> " << geoLastId+1); - gf->setId(++geoLastId); - } - } - updateGeoHistory(); + if (it == deps.end()) { + return ""; } + + auto it2 = it->second.find("Constraints"); + if (it2 != it->second.end()) { + for (auto& oid : it2->second) { + const Constraint* constraint = Constraints.getConstraint(oid); + + if (!constraint->isDriving) + return "Reference constraint from this sketch cannot be used in this " + "expression."; + } + } + + geoMap.clear(); + const auto &vals = getInternalGeometry(); + for(long i=0; i<(long)vals.size(); ++i) { + auto geo = vals[i]; + auto gf = GeometryFacade::getFacade(geo); + if(!gf->getId()) { + gf->setId(++geoLastId); + } + else if(gf->getId() > geoLastId) { + geoLastId = gf->getId(); + } + while(!geoMap.insert(std::make_pair(gf->getId(),i)).second) { + FC_WARN("duplicate geometry id " << gf->getId() << " -> " << geoLastId+1); + gf->setId(++geoLastId); + } + } + updateGeoHistory(); + return ""; } From 5e67c9111683f35b3908a815ccb4466899316f15 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Thu, 22 Aug 2024 21:51:27 +0530 Subject: [PATCH 16/23] [Sketcher] Refactor `removeAxesAlignment()` Use switch case for more readability, and move some if and for indentations --- src/Mod/Sketcher/App/SketchObject.cpp | 136 +++++++++++++------------- 1 file changed, 67 insertions(+), 69 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 77d5032a28..44a49a18dd 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -5507,52 +5507,54 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) const std::vector& constrvals = this->Constraints.getValues(); - unsigned int nhoriz = 0; - unsigned int nvert = 0; + std::map numConstrOfType = + {{Sketcher::Horizontal, 0}, {Sketcher::Vertical, 0}}; bool changed = false; std::vector> changeConstraintIndices; + auto chooseActionForConstraint = [&] + (size_t i, const int geoid) { + if (!constrvals[i]->involvesGeoId(geoid)) { + return; + } + switch (constrvals[i]->Type) { + case Sketcher::Horizontal: + case Sketcher::Vertical: { + if (constrvals[i]->FirstPos == Sketcher::PointPos::none + && constrvals[i]->SecondPos == Sketcher::PointPos::none) { + changeConstraintIndices.emplace_back(i, constrvals[i]->Type); + numConstrOfType[constrvals[i]->Type]++; + } + break; + } + case Sketcher::Symmetric: { + // only remove symmetric to axes + if ((constrvals[i]->Third == GeoEnum::HAxis || constrvals[i]->Third == GeoEnum::VAxis) + && constrvals[i]->ThirdPos == Sketcher::PointPos::none) + changeConstraintIndices.emplace_back(i, constrvals[i]->Type); + break; + } + case Sketcher::PointOnObject: { + if ((constrvals[i]->Second == GeoEnum::HAxis || constrvals[i]->Second == GeoEnum::VAxis) + && constrvals[i]->SecondPos == Sketcher::PointPos::none) + changeConstraintIndices.emplace_back(i, constrvals[i]->Type); + break; + } + case Sketcher::DistanceX: + case Sketcher::DistanceY: { + changeConstraintIndices.emplace_back(i, constrvals[i]->Type); + break; + } + default: + break; + } + }; + for (size_t i = 0; i < constrvals.size(); i++) { for (const auto& geoid : geoIdList) { - if (!constrvals[i]->involvesGeoId(geoid)) { - continue; - } - switch (constrvals[i]->Type) { - case Sketcher::Horizontal: - if (constrvals[i]->FirstPos == Sketcher::PointPos::none - && constrvals[i]->SecondPos == Sketcher::PointPos::none) { - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - nhoriz++; - } - break; - case Sketcher::Vertical: - if (constrvals[i]->FirstPos == Sketcher::PointPos::none - && constrvals[i]->SecondPos == Sketcher::PointPos::none) { - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - nvert++; - } - break; - case Sketcher::Symmetric:// only remove symmetric to axes - if ((constrvals[i]->Third == GeoEnum::HAxis - || constrvals[i]->Third == GeoEnum::VAxis) - && constrvals[i]->ThirdPos == Sketcher::PointPos::none) - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - break; - case Sketcher::PointOnObject: - if ((constrvals[i]->Second == GeoEnum::HAxis - || constrvals[i]->Second == GeoEnum::VAxis) - && constrvals[i]->SecondPos == Sketcher::PointPos::none) - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - break; - case Sketcher::DistanceX: - case Sketcher::DistanceY: - changeConstraintIndices.emplace_back(i, constrvals[i]->Type); - break; - default: - break; - } + chooseActionForConstraint(i, geoid); } } @@ -5562,8 +5564,9 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) std::vector newconstrVals; newconstrVals.reserve(constrvals.size()); - int referenceHorizontal = GeoEnum::GeoUndef; - int referenceVertical = GeoEnum::GeoUndef; + std::map refConstrOfType = + {{Sketcher::Horizontal, GeoEnum::GeoUndef}, + {Sketcher::Vertical, GeoEnum::GeoUndef}}; int cindex = 0; for (size_t i = 0; i < constrvals.size(); i++) { @@ -5572,57 +5575,52 @@ int SketchObject::removeAxesAlignment(const std::vector& geoIdList) continue; } - if (changeConstraintIndices[cindex].second == Sketcher::Horizontal && nhoriz > 0) { + switch (changeConstraintIndices[cindex].second) { + case Sketcher::Horizontal: + case Sketcher::Vertical: { + if (!(numConstrOfType[changeConstraintIndices[cindex].second] > 0)) { + break; + } changed = true; - if (referenceHorizontal == GeoEnum::GeoUndef) { - referenceHorizontal = constrvals[i]->First; + if (refConstrOfType[changeConstraintIndices[cindex].second] == GeoEnum::GeoUndef) { + refConstrOfType[changeConstraintIndices[cindex].second] = constrvals[i]->First; ++cindex; continue; } auto newConstr = new Constraint(); newConstr->Type = Sketcher::Parallel; - newConstr->First = referenceHorizontal; + newConstr->First = refConstrOfType[changeConstraintIndices[cindex].second]; newConstr->Second = constrvals[i]->First; newconstrVals.push_back(newConstr); + break; } - else if (changeConstraintIndices[cindex].second == Sketcher::Vertical && nvert > 0) { + case Sketcher::Symmetric: + case Sketcher::PointOnObject: { + changed = true; // We remove symmetric/point-on-object on axes + break; + } + case Sketcher::DistanceX: + case Sketcher::DistanceY: { changed = true; - if (referenceVertical == GeoEnum::GeoUndef) { - referenceVertical = constrvals[i]->First; - ++cindex; - continue; - } - auto newConstr = new Constraint(); - - newConstr->Type = Sketcher::Parallel; - newConstr->First = referenceVertical; - newConstr->Second = constrvals[i]->First; - - newconstrVals.push_back(newConstr); - } - else if (changeConstraintIndices[cindex].second == Sketcher::Symmetric - || changeConstraintIndices[cindex].second == Sketcher::PointOnObject) { - changed = true;// We remove symmetric on axes - } - else if (changeConstraintIndices[cindex].second == Sketcher::DistanceX - || changeConstraintIndices[cindex].second == Sketcher::DistanceY) { - changed = true;// We remove symmetric on axes // TODO: Handle pathological cases like DistanceY on horizontal constraint newconstrVals.push_back(constrvals[i]->clone()); newconstrVals.back()->Type = Sketcher::Distance; + break; + } + default: break; } ++cindex; } - if (nhoriz > 0 && nvert > 0) { + if (numConstrOfType[Sketcher::Horizontal] > 0 && numConstrOfType[Sketcher::Vertical] > 0) { auto newConstr = new Constraint(); newConstr->Type = Sketcher::Perpendicular; - newConstr->First = referenceVertical; - newConstr->Second = referenceHorizontal; + newConstr->First = refConstrOfType[Sketcher::Horizontal]; + newConstr->Second = refConstrOfType[Sketcher::Vertical]; newconstrVals.push_back(newConstr); } From 8c5a2afce57ba2414f0b035c663a1b35759caff6 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sat, 31 Aug 2024 20:04:05 +0530 Subject: [PATCH 17/23] [Sketcher] Refactor `SketchObject::onChanged()` Break into pieces depending on which property has been passed. --- src/Mod/Sketcher/App/SketchObject.cpp | 552 ++++++++++++++------------ src/Mod/Sketcher/App/SketchObject.h | 8 + 2 files changed, 316 insertions(+), 244 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 44a49a18dd..8f52bfbf18 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -10260,268 +10260,331 @@ static inline bool checkMigration(Part::PropertyGeometryList &prop) void SketchObject::onChanged(const App::Property* prop) { if (prop == &Geometry) { - if (isRestoring() && checkMigration(Geometry)) { - // Construction migration to extension - for (auto geometryValue : Geometry.getValues()) { - if (geometryValue->hasExtension( - Part::GeometryMigrationExtension::getClassTypeId())) { - auto ext = std::static_pointer_cast( - geometryValue - ->getExtension(Part::GeometryMigrationExtension::getClassTypeId()) - .lock()); - - auto gf = GeometryFacade::getFacade( - geometryValue); // at this point IA geometry is already migrated - - if (ext->testMigrationType(Part::GeometryMigrationExtension::Construction)) { - bool oldconstr = ext->getConstruction(); - if (geometryValue->is() - && !gf->isInternalAligned()) { - oldconstr = true; - } - gf->setConstruction(oldconstr); - } - if (ext->testMigrationType(Part::GeometryMigrationExtension::GeometryId)) { - gf->setId(ext->getId()); - } - } - } - } - geoMap.clear(); - const auto &vals = getInternalGeometry(); - for(long i=0;i<(long)vals.size();++i) { - auto geo = vals[i]; - auto gf = GeometryFacade::getFacade(geo); - if (gf->getId() == 0) { - gf->setId(++geoLastId); - } - else if (gf->getId() > geoLastId) { - geoLastId = gf->getId(); - } - while (!geoMap.insert(std::make_pair(gf->getId(), i)).second) { - FC_WARN("duplicate geometry id " << gf->getId() << " -> " - << geoLastId + 1); // NOLINT - gf->setId(++geoLastId); - } - } - updateGeoHistory(); + onGeometryChanged(); } - - auto doc = getDocument(); - - if (prop == &Geometry || prop == &Constraints) { - if (doc && doc->isPerformingTransaction()) {// undo/redo - setStatus(App::PendingTransactionUpdate, true); - } - else { - if (!internaltransaction) { - // internal sketchobject operations changing both geometry and constraints will - // explicitly perform an update - if (prop == &Geometry) { - if (managedoperation || isRestoring()) { - // if geometry changed, the constraint geometry indices must be updated - acceptGeometry(); - } - else { - // this change was not effect via SketchObject, but using direct access to - // properties, check input data - - // declares constraint invalid if indices go beyond the geometry and any - // call to getValues with return an empty list until this is fixed. - bool invalidinput = Constraints.checkConstraintIndices( - getHighestCurveIndex(), -getExternalGeometryCount()); - - if (!invalidinput) { - acceptGeometry(); - } - else { - Base::Console().error( - this->getFullLabel() + " SketchObject::onChanged ", - QT_TRANSLATE_NOOP("Notifications", "Unmanaged change of Geometry Property " - "results in invalid constraint indices") "\n"); - } - Base::StateLocker lock(internaltransaction, true); - setUpSketch(); - } - } - else {// Change is in Constraints - - if (managedoperation || isRestoring()) { - Constraints.checkGeometry(getCompleteGeometry()); - } - else { - // this change was not effect via SketchObject, but using direct access to - // properties, check input data - - // declares constraint invalid if indices go beyond the geometry and any - // call to getValues with return an empty list until this is fixed. - bool invalidinput = Constraints.checkConstraintIndices( - getHighestCurveIndex(), -getExternalGeometryCount()); - - if (!invalidinput) { - if (Constraints.checkGeometry(getCompleteGeometry())) { - // if there are invalid geometry indices in the constraints, we need - // to update them - acceptGeometry(); - } - } - else { - Base::Console().error( - this->getFullLabel() + " SketchObject::onChanged ", - QT_TRANSLATE_NOOP("Notifications", "Unmanaged change of Constraint " - "Property results in invalid constraint indices") "\n"); - } - Base::StateLocker lock(internaltransaction, true); - setUpSketch(); - } - } - } - } + else if (prop == &Constraints) { + onConstraintsChanged(); } else if (prop == &ExternalGeo && !prop->testStatus(App::Property::User3)) { - if (doc && doc->isPerformingTransaction()) { - setStatus(App::PendingTransactionUpdate, true); - } - - if (isRestoring() && checkMigration(ExternalGeo)) { - for (auto geometryValue : ExternalGeo.getValues()) { - if (geometryValue->hasExtension( - Part::GeometryMigrationExtension::getClassTypeId())) { - auto ext = std::static_pointer_cast( - geometryValue - ->getExtension(Part::GeometryMigrationExtension::getClassTypeId()) - .lock()); - std::unique_ptr egf; - if (ext->testMigrationType(Part::GeometryMigrationExtension::GeometryId)) { - egf = ExternalGeometryFacade::getFacade(geometryValue); - egf->setId(ext->getId()); - } - - if (ext->testMigrationType( - Part::GeometryMigrationExtension::ExternalReference)) { - if (!egf) { - egf = ExternalGeometryFacade::getFacade(geometryValue); - } - egf->setRef(ext->getRef()); - egf->setRefIndex(ext->getRefIndex()); - egf->setFlags(ext->getFlags()); - } - } - } - } - externalGeoRefMap.clear(); - externalGeoMap.clear(); - std::set detached; - for(int i=0;itestFlag(ExternalGeometryExtension::Detached)) { - if (!egf->getRef().empty()) { - detached.insert(egf->getRef()); - egf->setRef(std::string()); - } - egf->setFlag(ExternalGeometryExtension::Detached,false); - egf->setFlag(ExternalGeometryExtension::Missing,false); - } - if (egf->getId() > geoLastId) { - geoLastId = egf->getId(); - } - if (!externalGeoMap.emplace(egf->getId(), i).second) { - FC_WARN("duplicate geometry id " << egf->getId() << " -> " - << geoLastId + 1); // NOLINT - egf->setId(++geoLastId); - externalGeoMap[egf->getId()] = i; - } - if (!egf->getRef().empty()) { - externalGeoRefMap[egf->getRef()].push_back(egf->getId()); - } - } - if (!detached.empty()) { - auto objs = ExternalGeometry.getValues(); - assert(externalGeoRef.size() == objs.size()); - auto itObj = objs.begin(); - auto subs = ExternalGeometry.getSubValues(); - auto itSub = subs.begin(); - for (const auto& i : externalGeoRef) { - if (detached.count(i) != 0U) { - itObj = objs.erase(itObj); - itSub = subs.erase(itSub); - auto& refs = externalGeoRefMap[i]; - for (long id : refs) { - auto it = externalGeoMap.find(id); - if(it!=externalGeoMap.end()) { - auto geo = ExternalGeo[it->second]; - ExternalGeometryFacade::getFacade(geo)->setRef(std::string()); - } - } - refs.clear(); - } else { - ++itObj; - ++itSub; - } - } - ExternalGeometry.setValues(objs, subs); - } - else { - signalElementsChanged(); - } + onExternalGeoChanged(); } else if (prop == &ExternalGeometry) { - if (doc && doc->isPerformingTransaction()) { - setStatus(App::PendingTransactionUpdate, true); - } - - if(!isRestoring()) { - // must wait till onDocumentRestored() when shadow references are - // fully restored - updateGeometryRefs(); - signalElementsChanged(); - } + onExternalGeometryChanged(); } else if (prop == &Placement) { - if (ExternalGeometry.getSize() > 0) { - touch(); - } + onPlacementChanged(); } else if (prop == &ExpressionEngine) { - if (!isRestoring() && doc && !doc->isPerformingTransaction() && noRecomputes - && !managedoperation) { - // if we do not have a recompute, the sketch must be solved to - // update the DoF of the solver, constraints and UI - try { - auto res = ExpressionEngine.execute(); - if (res) { - FC_ERR("Failed to recompute " << ExpressionEngine.getFullName() << ": " - << res->Why); // NOLINT - delete res; - } - } catch (Base::Exception &e) { - e.reportException(); - FC_ERR("Failed to recompute " << ExpressionEngine.getFullName() << ": " - << e.what()); // NOLINT - } - solve(); - } + onExpressionEngineChanged(); } #if 0 // For now do not delete anything (#0001791). When changing the support // face it might be better to check which external geometries can be kept. else if (prop == &AttachmentSupport) { - // make sure not to change anything while restoring this object - if (!isRestoring()) { - // if support face has changed then clear the external geometry - delConstraintsToExternal(); - for (int i=0; i < getExternalGeometryCount(); i++) { - delExternal(0); - } - rebuildExternalGeometry(); - } + onAttachmentSupportChanged(); } #endif Part::Part2DObject::onChanged(prop); } -void SketchObject::onUpdateElementReference(const App::Property *prop) { +void SketchObject::onGeometryChanged() +{ + if (isRestoring() && checkMigration(Geometry)) { + // Construction migration to extension + for (auto geometryValue : Geometry.getValues()) { + if (!geometryValue->hasExtension( + Part::GeometryMigrationExtension::getClassTypeId())) { + continue; + } + + auto ext = std::static_pointer_cast( + geometryValue + ->getExtension(Part::GeometryMigrationExtension::getClassTypeId()) + .lock()); + + // at this point IA geometry is already migrated + auto gf = GeometryFacade::getFacade(geometryValue); + + if (ext->testMigrationType(Part::GeometryMigrationExtension::Construction)) { + bool oldconstr = ext->getConstruction() + || (geometryValue->is() && !gf->isInternalAligned()); + gf->setConstruction(oldconstr); + } + if (ext->testMigrationType(Part::GeometryMigrationExtension::GeometryId)) { + gf->setId(ext->getId()); + } + } + } + geoMap.clear(); + const auto &vals = getInternalGeometry(); + for (long i = 0; i < (long)vals.size(); ++i) { + auto geo = vals[i]; + auto gf = GeometryFacade::getFacade(geo); + if (gf->getId() == 0) { + gf->setId(++geoLastId); + } + else if (gf->getId() > geoLastId) { + geoLastId = gf->getId(); + } + while (!geoMap.insert(std::make_pair(gf->getId(), i)).second) { + FC_WARN("duplicate geometry id " << gf->getId() << " -> " + << geoLastId + 1); // NOLINT + gf->setId(++geoLastId); + } + } + updateGeoHistory(); + + auto doc = getDocument(); + + if (doc && doc->isPerformingTransaction()) { + // undo/redo + setStatus(App::PendingTransactionUpdate, true); + return; + } + + if (internaltransaction) { + return; + } + + // internal sketchobject operations changing both geometry and constraints will + // explicitly perform an update + + if (managedoperation || isRestoring()) { + // if geometry changed, the constraint geometry indices must be updated + acceptGeometry(); + return; + } + + // this change was not effect via SketchObject, but using direct access to + // properties, check input data + + // declares constraint invalid if indices go beyond the geometry and any + // call to getValues with return an empty list until this is fixed. + bool invalidinput = Constraints.checkConstraintIndices( + getHighestCurveIndex(), -getExternalGeometryCount()); + + if (!invalidinput) { + acceptGeometry(); + } + else { + Base::Console().error( + this->getFullLabel() + " SketchObject::onChanged ", + QT_TRANSLATE_NOOP("Notifications", "Unmanaged change of Constraint " + "Property results in invalid constraint indices") "\n"); + } + Base::StateLocker lock(internaltransaction, true); + setUpSketch(); +} + +void SketchObject::onConstraintsChanged() +{ + auto doc = getDocument(); + + if (doc && doc->isPerformingTransaction()) { + // undo/redo + setStatus(App::PendingTransactionUpdate, true); + return; + } + + if (internaltransaction) { + return; + } + + if (managedoperation || isRestoring()) { + Constraints.checkGeometry(getCompleteGeometry()); + return; + } + + // this change was not effect via SketchObject, but using direct access to + // properties, check input data + + // declares constraint invalid if indices go beyond the geometry and any + // call to getValues with return an empty list until this is fixed. + bool invalidinput = Constraints.checkConstraintIndices( + getHighestCurveIndex(), -getExternalGeometryCount()); + + if (!invalidinput) { + if (Constraints.checkGeometry(getCompleteGeometry())) { + // if there are invalid geometry indices in the constraints, we need + // to update them + acceptGeometry(); + } + } + else { + Base::Console().error( + this->getFullLabel() + " SketchObject::onChanged ", + QT_TRANSLATE_NOOP("Notifications", "Unmanaged change of Constraint " + "Property results in invalid constraint indices") "\n"); + } + Base::StateLocker lock(internaltransaction, true); + setUpSketch(); +} + +/// not to be confused with `onExternalGeometryChanged`. These names may need fixing. +void SketchObject::onExternalGeoChanged() +{ + if (ExternalGeo.testStatus(App::Property::User3)) { + return; + } + + auto doc = getDocument(); + + if (doc && doc->isPerformingTransaction()) { + setStatus(App::PendingTransactionUpdate, true); + } + + if (isRestoring() && checkMigration(ExternalGeo)) { + for (auto geometryValue : ExternalGeo.getValues()) { + if (!geometryValue->hasExtension( + Part::GeometryMigrationExtension::getClassTypeId())) { + continue; + } + + auto ext = std::static_pointer_cast( + geometryValue + ->getExtension(Part::GeometryMigrationExtension::getClassTypeId()) + .lock()); + std::unique_ptr egf; + if (ext->testMigrationType(Part::GeometryMigrationExtension::GeometryId)) { + egf = ExternalGeometryFacade::getFacade(geometryValue); + egf->setId(ext->getId()); + } + + if (!ext->testMigrationType(Part::GeometryMigrationExtension::ExternalReference)) { + continue; + } + + if (!egf) { + egf = ExternalGeometryFacade::getFacade(geometryValue); + } + egf->setRef(ext->getRef()); + egf->setRefIndex(ext->getRefIndex()); + egf->setFlags(ext->getFlags()); + } + } + externalGeoRefMap.clear(); + externalGeoMap.clear(); + std::set detached; + for(int i=0; itestFlag(ExternalGeometryExtension::Detached)) { + if (!egf->getRef().empty()) { + detached.insert(egf->getRef()); + egf->setRef(std::string()); + } + egf->setFlag(ExternalGeometryExtension::Detached,false); + egf->setFlag(ExternalGeometryExtension::Missing,false); + } + if (egf->getId() > geoLastId) { + geoLastId = egf->getId(); + } + if (!externalGeoMap.emplace(egf->getId(), i).second) { + FC_WARN("duplicate geometry id " << egf->getId() << " -> " + << geoLastId + 1); // NOLINT + egf->setId(++geoLastId); + externalGeoMap[egf->getId()] = i; + } + if (!egf->getRef().empty()) { + externalGeoRefMap[egf->getRef()].push_back(egf->getId()); + } + } + if (detached.empty()) { + signalElementsChanged(); + return; + } + + auto objs = ExternalGeometry.getValues(); + assert(externalGeoRef.size() == objs.size()); + auto itObj = objs.begin(); + auto subs = ExternalGeometry.getSubValues(); + auto itSub = subs.begin(); + for (const auto& i : externalGeoRef) { + if (detached.count(i) == 0U) { + ++itObj; + ++itSub; + continue; + } + + itObj = objs.erase(itObj); + itSub = subs.erase(itSub); + auto& refs = externalGeoRefMap[i]; + for (long id : refs) { + auto it = externalGeoMap.find(id); + if (it!=externalGeoMap.end()) { + auto geo = ExternalGeo[it->second]; + ExternalGeometryFacade::getFacade(geo)->setRef(std::string()); + } + } + refs.clear(); + } + ExternalGeometry.setValues(objs, subs); +} + +void SketchObject::onExternalGeometryChanged() +{ + auto doc = getDocument(); + + if (doc && doc->isPerformingTransaction()) { + setStatus(App::PendingTransactionUpdate, true); + } + + if(!isRestoring()) { + // must wait till onDocumentRestored() when shadow references are + // fully restored + updateGeometryRefs(); + signalElementsChanged(); + } +} + +void SketchObject::onPlacementChanged() +{ + if (ExternalGeometry.getSize() > 0) { + touch(); + } +} + +void SketchObject::onExpressionEngineChanged() +{ + auto doc = getDocument(); + + if (!isRestoring() && doc && !doc->isPerformingTransaction() && noRecomputes + && !managedoperation) { + // if we do not have a recompute, the sketch must be solved to + // update the DoF of the solver, constraints and UI + try { + auto res = ExpressionEngine.execute(); + if (res) { + FC_ERR("Failed to recompute " << ExpressionEngine.getFullName() << ": " + << res->Why); // NOLINT + delete res; + } + } catch (Base::Exception &e) { + e.reportException(); + FC_ERR("Failed to recompute " << ExpressionEngine.getFullName() << ": " + << e.what()); // NOLINT + } + solve(); + } +} + +void SketchObject::onAttachmentSupportChanged() +{ + // make sure not to change anything while restoring this object + if (isRestoring()) { + return; + } + + // if support face has changed then clear the external geometry + delConstraintsToExternal(); + for (int i=0; i < getExternalGeometryCount(); i++) { + delExternal(0); + } + rebuildExternalGeometry(); +} + +void SketchObject::onUpdateElementReference(const App::Property *prop) +{ if(prop == &ExternalGeometry) { updateGeoRef = true; // Must call updateGeometryRefs() now to avoid the case of recursive @@ -10533,7 +10596,8 @@ void SketchObject::onUpdateElementReference(const App::Property *prop) { } } -void SketchObject::updateGeometryRefs() { +void SketchObject::updateGeometryRefs() +{ const auto &objs = ExternalGeometry.getValues(); const auto &subs = ExternalGeometry.getSubValues(); const auto &shadows = ExternalGeometry.getShadowSubs(); diff --git a/src/Mod/Sketcher/App/SketchObject.h b/src/Mod/Sketcher/App/SketchObject.h index f42e398a38..5fe64f2f99 100644 --- a/src/Mod/Sketcher/App/SketchObject.h +++ b/src/Mod/Sketcher/App/SketchObject.h @@ -1010,6 +1010,14 @@ protected: /// b-splines need their own treatment int deleteUnusedInternalGeometryWhenBSpline(int GeoId, bool delgeoid = false); + void onGeometryChanged(); + void onConstraintsChanged(); + void onExternalGeoChanged(); + void onExternalGeometryChanged(); + void onPlacementChanged(); + void onExpressionEngineChanged(); + void onAttachmentSupportChanged(); + void onDocumentRestored() override; void restoreFinished() override; void onSketchRestore(); From 4fb27ad75863c3ce3467765f8ba0f7bbdba6d679 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 1 Sep 2024 11:21:10 +0530 Subject: [PATCH 18/23] [Sketcher] Refactor `SketchObject::updateGeometryRefs()` --- src/Mod/Sketcher/App/SketchObject.cpp | 119 ++++++++++++++------------ 1 file changed, 65 insertions(+), 54 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 8f52bfbf18..f6db11fd63 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -10604,14 +10604,14 @@ void SketchObject::updateGeometryRefs() assert(subs.size() == shadows.size()); std::vector originalRefs; std::map refMap; - if(updateGeoRef) { + if (updateGeoRef) { assert(externalGeoRef.size() == objs.size()); updateGeoRef = false; originalRefs = std::move(externalGeoRef); } externalGeoRef.clear(); std::unordered_map legacyMap; - for(int i=0;i<(int)objs.size();++i) { + for (int i=0;i<(int)objs.size();++i) { auto obj = objs[i]; const std::string& sub = shadows[i].newName.empty() ? subs[i] : shadows[i].newName; externalGeoRef.emplace_back(obj->getNameInDocument()); @@ -10629,56 +10629,7 @@ void SketchObject::updateGeometryRefs() } bool touched = false; auto geos = ExternalGeo.getValues(); - if(refMap.empty()) { - for(auto geo : geos) { - auto egf = ExternalGeometryFacade::getFacade(geo); - if (egf->getRefIndex() < 0) { - if (egf->getId() < 0 && !egf->getRef().empty()) { - // NOLINTNEXTLINE - FC_ERR("External geometry reference corrupted in " << getFullName() - << " Please check."); - // This could happen if someone saved the sketch containing - // external geometries using some rogue releases during the - // migration period. As a remedy, We re-initiate the - // external geometry here to trigger rebuild later, with - // call to rebuildExternalGeometry() - initExternalGeo(); - return; - } - auto it = legacyMap.find(egf->getRef()); - if (it != legacyMap.end() && egf->getRef() != externalGeoRef[it->second]) { - if(getDocument() && !getDocument()->isPerformingTransaction()) { - // FIXME: this is a bug. Find out when and why does this happen - // - // Amendment: maybe the original bug is because of not - // handling external geometry changes during undo/redo, - // which should be considered as normal. So warning only - // if not undo/redo. - // - // NOLINTNEXTLINE - FC_WARN("Update legacy external reference " - << egf->getRef() << " -> " << externalGeoRef[it->second] << " in " - << getFullName()); - } - else { - // NOLINTNEXTLINE - FC_LOG("Update undo/redo external reference " - << egf->getRef() << " -> " << externalGeoRef[it->second] << " in " - << getFullName()); - } - touched = true; - egf->setRef(externalGeoRef[it->second]); - } - continue; - } - if (egf->getRefIndex() < (int)externalGeoRef.size() - && egf->getRef() != externalGeoRef[egf->getRefIndex()]) { - touched = true; - egf->setRef(externalGeoRef[egf->getRefIndex()]); - } - egf->setRefIndex(-1); - } - }else{ + if (!refMap.empty()) { for(auto &v : refMap) { auto it = externalGeoRefMap.find(v.first); if (it == externalGeoRefMap.end()) { @@ -10686,19 +10637,79 @@ void SketchObject::updateGeometryRefs() } for (long id : it->second) { auto iter = externalGeoMap.find(id); - if(iter!=externalGeoMap.end()) { + if (iter != externalGeoMap.end()) { auto &geo = geos[iter->second]; geo = geo->clone(); auto egf = ExternalGeometryFacade::getFacade(geo); // NOLINTNEXTLINE FC_LOG(getFullName() << " ref change on ExternalEdge" << iter->second - 1 << ' ' - << egf->getRef() << " -> " << v.second); + << egf->getRef() << " -> " << v.second); egf->setRef(v.second); touched = true; } } } + + if (touched) { + ExternalGeo.setValues(std::move(geos)); + } + return; } + + // `refMap` is empty from here + + for (auto geo : geos) { + auto egf = ExternalGeometryFacade::getFacade(geo); + if (egf->getRefIndex() >= 0) { + if (egf->getRefIndex() < (int)externalGeoRef.size() + && egf->getRef() != externalGeoRef[egf->getRefIndex()]) { + touched = true; + egf->setRef(externalGeoRef[egf->getRefIndex()]); + } + egf->setRefIndex(-1); + continue; + } + + if (egf->getId() < 0 && !egf->getRef().empty()) { + // NOLINTNEXTLINE + FC_ERR("External geometry reference corrupted in " << getFullName() + << " Please check."); + // This could happen if someone saved the sketch containing + // external geometries using some rogue releases during the + // migration period. As a remedy, We re-initiate the + // external geometry here to trigger rebuild later, with + // call to rebuildExternalGeometry() + initExternalGeo(); + return; + } + + auto it = legacyMap.find(egf->getRef()); + if (it == legacyMap.end() || egf->getRef() == externalGeoRef[it->second]) { + continue; + } + + if (getDocument() && !getDocument()->isPerformingTransaction()) { + // FIXME: this is a bug. Find out when and why does this happen + // + // Amendment: maybe the original bug is because of not handling external geometry + // changes during undo/redo, which should be considered as normal. So warning + // only if not undo/redo. + // + // NOLINTNEXTLINE + FC_WARN("Update legacy external reference " + << egf->getRef() << " -> " << externalGeoRef[it->second] << " in " + << getFullName()); + } + else { + // NOLINTNEXTLINE + FC_LOG("Update undo/redo external reference " + << egf->getRef() << " -> " << externalGeoRef[it->second] << " in " + << getFullName()); + } + touched = true; + egf->setRef(externalGeoRef[it->second]); + } + if (touched) { ExternalGeo.setValues(std::move(geos)); } From 507e936f2598dda559899b81c244a1a2c07672da Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 7 Apr 2025 01:07:08 +0530 Subject: [PATCH 19/23] [Sketcher] Use `auto` where possible --- src/Mod/Sketcher/App/SketchObject.cpp | 116 +++++++++++++------------- 1 file changed, 57 insertions(+), 59 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index f6db11fd63..c2faa33cfb 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -403,7 +403,7 @@ void SketchObject::buildShape() } Part::TopoShape result(0, getDocument()->getStringHasher()); if (vertices.empty()) { - // Notice here we supply op code Part::OpCodes::Sketch to makEWires(). + // Notice here we supply op code Part::OpCodes::Sketch to makeElementWires(). result.makeElementWires(shapes, Part::OpCodes::Sketch); } else { @@ -2613,12 +2613,12 @@ void SketchObject::transferFilletConstraints(int geoId1, PointPos posId1, int ge } // Add a vertex to preserve the original intersection of the filleted lines - Part::GeomPoint* originalCorner = new Part::GeomPoint(getPoint(geoId1, posId1)); + auto* originalCorner = new Part::GeomPoint(getPoint(geoId1, posId1)); int originalCornerId = addGeometry(originalCorner, true); delete originalCorner; // Constrain the vertex to the two lines - Sketcher::Constraint* cornerToLine1 = new Sketcher::Constraint(); + auto* cornerToLine1 = new Sketcher::Constraint(); cornerToLine1->Type = Sketcher::PointOnObject; cornerToLine1->First = originalCornerId; cornerToLine1->FirstPos = PointPos::start; @@ -2626,7 +2626,7 @@ void SketchObject::transferFilletConstraints(int geoId1, PointPos posId1, int ge cornerToLine1->SecondPos = PointPos::none; addConstraint(cornerToLine1); delete cornerToLine1; - Sketcher::Constraint* cornerToLine2 = new Sketcher::Constraint(); + auto* cornerToLine2 = new Sketcher::Constraint(); cornerToLine2->Type = Sketcher::PointOnObject; cornerToLine2->First = originalCornerId; cornerToLine2->FirstPos = PointPos::start; @@ -2976,7 +2976,7 @@ int SketchObject::extend(int GeoId, double increment, PointPos endpoint) Part::Geometry* geom = geomList[GeoId]; int retcode = -1; if (geom->is()) { - Part::GeomLineSegment* seg = static_cast(geom); + auto* seg = static_cast(geom); Base::Vector3d startVec = seg->getStartPoint(); Base::Vector3d endVec = seg->getEndPoint(); if (endpoint == PointPos::start) { @@ -2997,7 +2997,7 @@ int SketchObject::extend(int GeoId, double increment, PointPos endpoint) } } else if (geom->is()) { - Part::GeomArcOfCircle* arc = static_cast(geom); + auto* arc = static_cast(geom); double startArc, endArc; arc->getRange(startArc, endArc, true); if (endpoint == PointPos::start) { @@ -3936,7 +3936,7 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) geoAsCurve = getGeometry(GeoId); if (!isOriginalCurvePeriodic) { - Constraint* joint = new Constraint(); + auto* joint = new Constraint(); joint->Type = Coincident; joint->First = newIds.front(); joint->FirstPos = PointPos::end; @@ -3951,7 +3951,7 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) // 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 (geoAsCurve->is()) { - Constraint* joint = new Constraint(); + auto* joint = new Constraint(); joint->Type = Coincident; joint->First = newIds.front(); joint->FirstPos = PointPos::mid; @@ -4113,7 +4113,7 @@ int SketchObject::join(int geoId1, Sketcher::PointPos posId1, int geoId2, Sketch std::make_move_iterator(mults2.begin()), std::make_move_iterator(mults2.end())); - Part::GeomBSplineCurve* newSpline = new Part::GeomBSplineCurve( + auto* newSpline = new Part::GeomBSplineCurve( newPoles, newWeights, newKnots, newMults, bsp1->getDegree(), false, true); int newGeoId = addGeometry(newSpline); @@ -4476,7 +4476,7 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, auto createSymConstr = [&](int first, int second, Sketcher::PointPos firstPos, Sketcher::PointPos secondPos) { - auto symConstr = new Constraint(); + auto* symConstr = new Constraint(); symConstr->Type = Symmetric; symConstr->First = first; symConstr->Second = second; @@ -4487,7 +4487,7 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, newconstrVals.push_back(symConstr); }; auto createEqualityConstr = [&](int first, int second) { - auto symConstr = new Constraint(); + auto* symConstr = new Constraint(); symConstr->Type = Equal; symConstr->First = first; symConstr->Second = second; @@ -5185,7 +5185,7 @@ int SketchObject::addCopy(const std::vector& geoIdList, } } 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; @@ -5315,7 +5315,7 @@ int SketchObject::addCopy(const std::vector& geoIdList, } // add a construction line - Part::GeomLineSegment* constrline = new Part::GeomLineSegment(); + auto* constrline = new Part::GeomLineSegment(); // position of the reference point Base::Vector3d sp = getPoint(refgeoid, refposId) @@ -5693,12 +5693,12 @@ int SketchObject::exposeInternalGeometryForType(const int Geo Base::Vector3d focus2P = center - df * majdir; if (!major) { - Part::GeomLineSegment* lmajor = new Part::GeomLineSegment(); + auto* lmajor = new Part::GeomLineSegment(); lmajor->setPoints(majorpositiveend, majornegativeend); igeo.push_back(lmajor); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = EllipseMajorDiameter; newConstr->First = currentgeoid + incrgeo + 1; @@ -5708,12 +5708,12 @@ int SketchObject::exposeInternalGeometryForType(const int Geo incrgeo++; } if (!minor) { - Part::GeomLineSegment* lminor = new Part::GeomLineSegment(); + auto* lminor = new Part::GeomLineSegment(); lminor->setPoints(minorpositiveend, minornegativeend); igeo.push_back(lminor); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = EllipseMinorDiameter; newConstr->First = currentgeoid + incrgeo + 1; @@ -5723,12 +5723,12 @@ int SketchObject::exposeInternalGeometryForType(const int Geo incrgeo++; } if (!focus1) { - Part::GeomPoint* pf1 = new Part::GeomPoint(); + auto* pf1 = new Part::GeomPoint(); pf1->setPoint(focus1P); igeo.push_back(pf1); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = EllipseFocus1; newConstr->First = currentgeoid + incrgeo + 1; @@ -5739,11 +5739,11 @@ int SketchObject::exposeInternalGeometryForType(const int Geo incrgeo++; } if (!focus2) { - Part::GeomPoint* pf2 = new Part::GeomPoint(); + auto* pf2 = new Part::GeomPoint(); pf2->setPoint(focus2P); igeo.push_back(pf2); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = EllipseFocus2; newConstr->First = currentgeoid + incrgeo + 1; @@ -5833,12 +5833,12 @@ int SketchObject::exposeInternalGeometryForType(const in Base::Vector3d focus2P {center - df * majdir}; if (!major) { - Part::GeomLineSegment* lmajor = new Part::GeomLineSegment(); + auto* lmajor = new Part::GeomLineSegment(); lmajor->setPoints(majorpositiveend, majornegativeend); igeo.push_back(lmajor); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = EllipseMajorDiameter; newConstr->First = currentgeoid + incrgeo + 1; @@ -5848,12 +5848,12 @@ int SketchObject::exposeInternalGeometryForType(const in incrgeo++; } if (!minor) { - Part::GeomLineSegment* lminor = new Part::GeomLineSegment(); + auto* lminor = new Part::GeomLineSegment(); lminor->setPoints(minorpositiveend, minornegativeend); igeo.push_back(lminor); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = EllipseMinorDiameter; newConstr->First = currentgeoid + incrgeo + 1; @@ -5863,12 +5863,12 @@ int SketchObject::exposeInternalGeometryForType(const in incrgeo++; } if (!focus1) { - Part::GeomPoint* pf1 = new Part::GeomPoint(); + auto* pf1 = new Part::GeomPoint(); pf1->setPoint(focus1P); igeo.push_back(pf1); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = EllipseFocus1; newConstr->First = currentgeoid + incrgeo + 1; @@ -5879,11 +5879,11 @@ int SketchObject::exposeInternalGeometryForType(const in incrgeo++; } if (!focus2) { - Part::GeomPoint* pf2 = new Part::GeomPoint(); + auto* pf2 = new Part::GeomPoint(); pf2->setPoint(focus2P); igeo.push_back(pf2); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = EllipseFocus2; newConstr->First = currentgeoid + incrgeo + 1; @@ -5953,12 +5953,12 @@ int SketchObject::exposeInternalGeometryForType(const Base::Vector3d focus1P = center + df * majdir; if (!major) { - Part::GeomLineSegment* lmajor = new Part::GeomLineSegment(); + auto* lmajor = new Part::GeomLineSegment(); lmajor->setPoints(majorpositiveend, majornegativeend); igeo.push_back(lmajor); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = Sketcher::HyperbolaMajor; newConstr->First = currentgeoid + incrgeo + 1; @@ -5968,12 +5968,12 @@ int SketchObject::exposeInternalGeometryForType(const incrgeo++; } if (!minor) { - Part::GeomLineSegment* lminor = new Part::GeomLineSegment(); + auto* lminor = new Part::GeomLineSegment(); lminor->setPoints(minorpositiveend, minornegativeend); igeo.push_back(lminor); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = Sketcher::HyperbolaMinor; newConstr->First = currentgeoid + incrgeo + 1; @@ -5984,12 +5984,12 @@ int SketchObject::exposeInternalGeometryForType(const incrgeo++; } if (!focus) { - Part::GeomPoint* pf1 = new Part::GeomPoint(); + auto* pf1 = new Part::GeomPoint(); pf1->setPoint(focus1P); igeo.push_back(pf1); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = Sketcher::HyperbolaFocus; newConstr->First = currentgeoid + incrgeo + 1; @@ -6043,12 +6043,12 @@ int SketchObject::exposeInternalGeometryForType(const i std::vector icon; if (!focus) { - Part::GeomPoint* pf1 = new Part::GeomPoint(); + auto* pf1 = new Part::GeomPoint(); pf1->setPoint(focusp); igeo.push_back(pf1); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = Sketcher::ParabolaFocus; newConstr->First = currentgeoid + incrgeo + 1; @@ -6060,12 +6060,12 @@ int SketchObject::exposeInternalGeometryForType(const i } if (!focus_to_vertex) { - Part::GeomLineSegment* paxis = new Part::GeomLineSegment(); + auto* paxis = new Part::GeomLineSegment(); paxis->setPoints(center, focusp); igeo.push_back(paxis); - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = Sketcher::ParabolaFocalAxis; newConstr->First = currentgeoid + incrgeo + 1; @@ -6140,14 +6140,14 @@ int SketchObject::exposeInternalGeometryForType(const in } // if controlpoint not existing - Part::GeomCircle* pc = new Part::GeomCircle(); + auto* 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(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = Sketcher::BSplineControlPoint; newConstr->First = currentgeoid + incrgeo; @@ -6161,7 +6161,7 @@ int SketchObject::exposeInternalGeometryForType(const in 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(); + auto* newConstr3 = new Sketcher::Constraint(); newConstr3->Type = Sketcher::Weight; newConstr3->First = controlpointgeoids[0]; newConstr3->setValue(weights[0]); @@ -6177,7 +6177,7 @@ int SketchObject::exposeInternalGeometryForType(const in 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(); + auto* newConstr2 = new Sketcher::Constraint(); newConstr2->Type = Sketcher::Equal; newConstr2->First = currentgeoid + incrgeo; newConstr2->FirstPos = Sketcher::PointPos::none; @@ -6195,14 +6195,14 @@ int SketchObject::exposeInternalGeometryForType(const in } // if knot point not existing - Part::GeomPoint* kp = new Part::GeomPoint(); + auto* kp = new Part::GeomPoint(); kp->setPoint(bsp->pointAtParameter(knots[index])); igeo.push_back(kp); incrgeo++; - Sketcher::Constraint* newConstr = new Sketcher::Constraint(); + auto* newConstr = new Sketcher::Constraint(); newConstr->Type = Sketcher::InternalAlignment; newConstr->AlignmentType = Sketcher::BSplineKnotPoint; newConstr->First = currentgeoid + incrgeo; @@ -7068,7 +7068,7 @@ int SketchObject::carbonCopy(App::DocumentObject* pObj, bool construction) return -1; } - SketchObject* psObj = static_cast(pObj); + auto* psObj = static_cast(pObj); const std::vector& vals = getInternalGeometry(); @@ -8015,12 +8015,12 @@ Part::Geometry* projectLine(const BRepAdaptor_Curve& curve, const Handle(Geom_Pl if (Base::Distance(p1, p2) < Precision::Confusion()) { Base::Vector3d p = (p1 + p2) / 2; - Part::GeomPoint* point = new Part::GeomPoint(p); + auto* point = new Part::GeomPoint(p); GeometryFacade::setConstruction(point, true); return point; } else { - Part::GeomLineSegment* line = new Part::GeomLineSegment(); + auto* line = new Part::GeomLineSegment(); line->setPoints(p1, p2); GeometryFacade::setConstruction(line, true); return line; @@ -8063,7 +8063,7 @@ static Part::Geometry *fitArcs(std::vector > &ar return nullptr; } if (P1.SquareDistance(P2) < Precision::Confusion()) { - Part::GeomCircle* circle = new Part::GeomCircle(); + auto* circle = new Part::GeomCircle(); circle->setCenter(center); circle->setRadius(radius); return circle; @@ -8077,7 +8077,7 @@ static Part::Geometry *fitArcs(std::vector > &ar GeomLProp_CLProps prop(Handle(Geom_Curve)::DownCast(arcs.front()->handle()),m,0,Precision::Confusion()); gp_Pnt midPoint = prop.Value(); GC_MakeArcOfCircle arc(P1, midPoint, P2); - auto geo = new Part::GeomArcOfCircle(); + auto* geo = new Part::GeomArcOfCircle(); geo->setHandle(arc.Value()); return geo; } @@ -8099,14 +8099,14 @@ void SketchObject::validateExternalLinks() TopoDS_Shape refSubShape; try { if (Obj->isDerivedFrom()) { - const Part::Datum* datum = static_cast(Obj); + const auto* datum = static_cast(Obj); refSubShape = datum->getShape(); } else if (Obj->isDerivedFrom()) { // do nothing - shape will be calculated later during rebuild } else { - const Part::Feature* refObj = static_cast(Obj); + const auto* refObj = static_cast(Obj); const Part::TopoShape& refShape = refObj->Shape.getShape(); refSubShape = refShape.getSubShape(SubElement.c_str()); } @@ -8445,7 +8445,7 @@ void processEdge(const TopoDS_Edge& edge, GeomAPI_ProjectPointOnSurf proj(cnt, gPlane); cnt = proj.NearestPoint(); - gp_Dir dirOrientation = gp_Dir(vec1 ^ vec2); + gp_Dir dirOrientation {vec1 ^ vec2}; gp_Dir dirLine(dirOrientation); auto* projectedSegment = new Part::GeomLineSegment(); @@ -8779,7 +8779,7 @@ void processEdge(const TopoDS_Edge& edge, // check for degenerated case when the line is collapsed to a point if (p1.SquareDistance(p2) < Precision::SquareConfusion()) { - Part::GeomPoint* point = new Part::GeomPoint((P1 + P2) / 2); + auto* point = new Part::GeomPoint((P1 + P2) / 2); GeometryFacade::setConstruction(point, true); geos.emplace_back(point); } @@ -9169,7 +9169,7 @@ void SketchObject::rebuildExternalGeometry(std::optional extToAdd Base::Vector3d p(P.X(), P.Y(), P.Z()); invPlm.multVec(p, p); - Part::GeomPoint* point = new Part::GeomPoint(p); + auto* point = new Part::GeomPoint(p); GeometryFacade::setConstruction(point, true); geos.emplace_back(point); }; @@ -9518,9 +9518,7 @@ void SketchObject::rebuildVertexIndex() const std::vector geometry = getCompleteGeometry(); if (geometry.size() <= 2) return; - for (std::vector::const_iterator it = geometry.begin(); - it != geometry.end() - 2; - ++it, i++) { + for (auto it = geometry.cbegin(); it != geometry.cend() - 2; ++it, ++i) { if (i > imax) i = -getExternalGeometryCount(); if ((*it)->is()) { @@ -10114,7 +10112,7 @@ void SketchObject::constraintsRenamed( void SketchObject::constraintsRemoved(const std::set& removed) { - std::set::const_iterator i = removed.begin(); + auto i = removed.begin(); while (i != removed.end()) { ExpressionEngine.setValue(*i, std::shared_ptr()); From 5283381d70673ba3ccadd910abc00b26795167b9 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 7 Apr 2025 15:42:24 +0530 Subject: [PATCH 20/23] [Sketcher] Use `replaceGeometries()` in `join()` --- src/Mod/Sketcher/App/SketchObject.cpp | 71 ++++++++++++++++----------- 1 file changed, 41 insertions(+), 30 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index c2faa33cfb..c43ef106ec 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -3980,9 +3980,11 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) return 0; } -// clang-format off - -int SketchObject::join(int geoId1, Sketcher::PointPos posId1, int geoId2, Sketcher::PointPos posId2, int continuity) +int SketchObject::join(int geoId1, + Sketcher::PointPos posId1, + int geoId2, + Sketcher::PointPos posId2, + int continuity) { // No need to check input data validity as this is an sketchobject managed operation @@ -4012,6 +4014,7 @@ int SketchObject::join(int geoId1, Sketcher::PointPos posId1, int geoId2, Sketch THROWM(ValueError, "Cannot join construction and non-construction geometries."); return -1; } + bool areOriginalCurvesConstruction = GeometryFacade::getConstruction(geo1); // TODO: make both curves b-splines here itself if (!geo1 || !geo2) { @@ -4032,16 +4035,20 @@ int SketchObject::join(int geoId1, Sketcher::PointPos posId1, int geoId2, Sketch } // reverse the splines if needed: join end of 1st to start of 2nd - if (Sketcher::PointPos::start == posId1) + if (Sketcher::PointPos::start == posId1) { bsp1->reverse(); - if (Sketcher::PointPos::end == posId2) + } + if (Sketcher::PointPos::end == posId2) { bsp2->reverse(); + } // ensure the degrees of both curves are the same - if (bsp1->getDegree() < bsp2->getDegree()) + if (bsp1->getDegree() < bsp2->getDegree()) { bsp1->increaseDegree(bsp2->getDegree()); - else if (bsp2->getDegree() < bsp1->getDegree()) + } + else if (bsp2->getDegree() < bsp1->getDegree()) { bsp2->increaseDegree(bsp1->getDegree()); + } // TODO: Check for tangent constraint here bool makeC1Continuous = (continuity >= 1); @@ -4077,16 +4084,18 @@ int SketchObject::join(int geoId1, Sketcher::PointPos posId1, int geoId2, Sketch std::vector newMults(std::move(mults1)); poles2.erase(poles2.begin()); - if (makeC1Continuous) - newPoles.erase(newPoles.end()-1); + if (makeC1Continuous) { + newPoles.erase(newPoles.end() - 1); + } newPoles.insert(newPoles.end(), std::make_move_iterator(poles2.begin()), std::make_move_iterator(poles2.end())); // TODO: Weights might need to be scaled weights2.erase(weights2.begin()); - if (makeC1Continuous) - newWeights.erase(newWeights.end()-1); + if (makeC1Continuous) { + newWeights.erase(newWeights.end() - 1); + } newWeights.insert(newWeights.end(), std::make_move_iterator(weights2.begin()), std::make_move_iterator(weights2.end())); @@ -4094,8 +4103,9 @@ int SketchObject::join(int geoId1, Sketcher::PointPos posId1, int geoId2, Sketch // knots of the second spline come after all of the first double offset = newKnots.back() - knots2.front(); knots2.erase(knots2.begin()); - for (auto& knot : knots2) + for (auto& knot : knots2) { knot += offset; + } newKnots.insert(newKnots.end(), std::make_move_iterator(knots2.begin()), std::make_move_iterator(knots2.end())); @@ -4113,32 +4123,33 @@ int SketchObject::join(int geoId1, Sketcher::PointPos posId1, int geoId2, Sketch std::make_move_iterator(mults2.begin()), std::make_move_iterator(mults2.end())); - auto* newSpline = new Part::GeomBSplineCurve( - newPoles, newWeights, newKnots, newMults, bsp1->getDegree(), false, true); + auto* newSpline = new Part::GeomBSplineCurve(newPoles, + newWeights, + newKnots, + newMults, + bsp1->getDegree(), + false, + true); - int newGeoId = addGeometry(newSpline); + // int newGeoId = addGeometry(newSpline); + std::vector newGeos {newSpline}; + replaceGeometries({geoId1, geoId2}, newGeos); - if (newGeoId < 0) { - THROWM(ValueError, "Failed to create joined curve."); - return -1; - } - - exposeInternalGeometry(newGeoId); - setConstruction(newGeoId, GeometryFacade::getConstruction(geo1)); + exposeInternalGeometry(geoId1); + setConstruction(geoId1, areOriginalCurvesConstruction); // 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; + auto otherPosId1 = + (Sketcher::PointPos::start == posId1) ? Sketcher::PointPos::end : Sketcher::PointPos::start; + auto otherPosId2 = + (Sketcher::PointPos::start == posId2) ? Sketcher::PointPos::end : Sketcher::PointPos::start; - transferConstraints(geoId1, otherPosId1, newGeoId, PointPos::start, true); - transferConstraints(geoId2, otherPosId2, newGeoId, PointPos::end, true); - - delGeometries({geoId1, geoId2}); + transferConstraints(geoId1, otherPosId1, geoId1, PointPos::start, true); + transferConstraints(geoId2, otherPosId2, geoId1, PointPos::end, true); return 0; } +// clang-format off bool SketchObject::isExternalAllowed(App::Document* pDoc, App::DocumentObject* pObj, eReasonList* rsn) const From 3b1c6df852638b39b6f5641ed0ccea78ff7216fb Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Mon, 25 Aug 2025 18:32:30 +0530 Subject: [PATCH 21/23] Sketcher: Add test stubs for `SketchObject::addCopy()` --- tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp b/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp index a0b20603aa..9acdaf4236 100644 --- a/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp +++ b/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp @@ -83,6 +83,12 @@ TEST_F(SketchObjectTest, testDelExternalReducesCount) // TODO: `delExternal` situation of constraints // TODO: `delExternal` situation of constraint containing more than 3 entities +// TODO: `addCopy` tests +// TODO: ensure new item(s) is/are added and of same type +// TODO: behaviour of `addCopy` when external +// TODO: constraints on new copies? +// TODO: when empty list is passed + TEST_F(SketchObjectTest, testReplaceGeometriesOneToOne) { // Arrange From 983e70d04667b09862d1f52fd05a82f21e663d6e Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Tue, 6 Jan 2026 07:05:50 +0530 Subject: [PATCH 22/23] Sketcher: Override clang-format for some readability See https://github.com/FreeCAD/FreeCAD/pull/22951#discussion_r2314544159. --- src/Mod/Sketcher/App/SketchObject.cpp | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index c43ef106ec..df06d41940 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -4387,8 +4387,9 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, continue; } if (!refIsAxisAligned - && (constr->Type == Sketcher::DistanceX || constr->Type == Sketcher::DistanceY - || constr->Type == Sketcher::Vertical + && (constr->Type == Sketcher::DistanceX // + || constr->Type == Sketcher::DistanceY // + || constr->Type == Sketcher::Vertical // || constr->Type == Sketcher::Horizontal)) { // this includes all non-directional single GeoId constraints, as radius, // diameter, weight,... @@ -4425,11 +4426,14 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, }; if (constr->Third == GeoEnum::GeoUndef) { - if (!(constr->Type == Sketcher::Coincident - || constr->Type == Sketcher::Perpendicular - || constr->Type == Sketcher::Parallel || constr->Type == Sketcher::Tangent - || constr->Type == Sketcher::Distance || constr->Type == Sketcher::Equal - || constr->Type == Sketcher::Angle || constr->Type == Sketcher::PointOnObject + if (!(constr->Type == Sketcher::Coincident // + || constr->Type == Sketcher::Perpendicular // + || constr->Type == Sketcher::Parallel // + || constr->Type == Sketcher::Tangent // + || constr->Type == Sketcher::Distance // + || constr->Type == Sketcher::Equal // + || constr->Type == Sketcher::Angle // + || constr->Type == Sketcher::PointOnObject // || constr->Type == Sketcher::InternalAlignment)) { continue; } @@ -4529,8 +4533,10 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, createEqualityConstr(geoId1, geoId2); createSymConstr(geoId1, geoId2, PointPos::mid, PointPos::mid); } - else if (geo->is() || geo->is() - || geo->is() || geo->is()) { + else if (geo->is() // + || geo->is() // + || geo->is() // + || geo->is()) { createEqualityConstr(geoId1, geoId2); createSymConstr(geoId1, geoId2, From ee53b892017b7eb082bd5ddc67343f6438a9a082 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 6 Jan 2026 01:40:24 +0000 Subject: [PATCH 23/23] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/Mod/Sketcher/App/SketchObject.cpp | 325 ++++++++++-------- .../Mod/Sketcher/App/SketchObjectChanges.cpp | 6 +- 2 files changed, 183 insertions(+), 148 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index df06d41940..811d09d833 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -2020,15 +2020,12 @@ int SketchObject::delGeometriesExclusiveList(const std::vector& GeoIds, Del } // clang-format on -void SketchObject::replaceGeometries(std::vector oldGeoIds, - std::vector& newGeos) +void SketchObject::replaceGeometries(std::vector oldGeoIds, std::vector& newGeos) { auto& vals = getInternalGeometry(); auto newVals(vals); - if (std::ranges::any_of(oldGeoIds, [](auto geoId) { - return geoId < 0; - })) { + if (std::ranges::any_of(oldGeoIds, [](auto geoId) { return geoId < 0; })) { THROWM(ValueError, "Cannot replace external geometries and axes."); } @@ -3980,11 +3977,13 @@ int SketchObject::split(int GeoId, const Base::Vector3d& point) return 0; } -int SketchObject::join(int geoId1, - Sketcher::PointPos posId1, - int geoId2, - Sketcher::PointPos posId2, - int continuity) +int SketchObject::join( + int geoId1, + Sketcher::PointPos posId1, + int geoId2, + Sketcher::PointPos posId2, + int continuity +) { // No need to check input data validity as this is an sketchobject managed operation @@ -4025,9 +4024,11 @@ int SketchObject::join(int geoId1, // we need the splines to be mutable because we may reverse them // and/or change their degree std::unique_ptr bsp1( - geo1->toNurbs(geo1->getFirstParameter(), geo1->getLastParameter())); + geo1->toNurbs(geo1->getFirstParameter(), geo1->getLastParameter()) + ); std::unique_ptr bsp2( - geo2->toNurbs(geo2->getFirstParameter(), geo2->getLastParameter())); + geo2->toNurbs(geo2->getFirstParameter(), geo2->getLastParameter()) + ); if (bsp1->isPeriodic() || bsp2->isPeriodic()) { THROWM(ValueError, "It is only possible to join non-periodic curves."); @@ -4087,18 +4088,22 @@ int SketchObject::join(int geoId1, if (makeC1Continuous) { newPoles.erase(newPoles.end() - 1); } - newPoles.insert(newPoles.end(), - std::make_move_iterator(poles2.begin()), - std::make_move_iterator(poles2.end())); + newPoles.insert( + newPoles.end(), + std::make_move_iterator(poles2.begin()), + std::make_move_iterator(poles2.end()) + ); // TODO: Weights might need to be scaled weights2.erase(weights2.begin()); if (makeC1Continuous) { newWeights.erase(newWeights.end() - 1); } - newWeights.insert(newWeights.end(), - std::make_move_iterator(weights2.begin()), - std::make_move_iterator(weights2.end())); + newWeights.insert( + newWeights.end(), + std::make_move_iterator(weights2.begin()), + std::make_move_iterator(weights2.end()) + ); // knots of the second spline come after all of the first double offset = newKnots.back() - knots2.front(); @@ -4106,9 +4111,11 @@ int SketchObject::join(int geoId1, for (auto& knot : knots2) { knot += offset; } - newKnots.insert(newKnots.end(), - std::make_move_iterator(knots2.begin()), - std::make_move_iterator(knots2.end())); + newKnots.insert( + newKnots.end(), + std::make_move_iterator(knots2.begin()), + std::make_move_iterator(knots2.end()) + ); // end knots can have a multiplicity of (degree + 1) if (bsp1->getDegree() < newMults.back()) { @@ -4119,17 +4126,21 @@ int SketchObject::join(int geoId1, } mults2.erase(mults2.begin()); - newMults.insert(newMults.end(), - std::make_move_iterator(mults2.begin()), - std::make_move_iterator(mults2.end())); + newMults.insert( + newMults.end(), + std::make_move_iterator(mults2.begin()), + std::make_move_iterator(mults2.end()) + ); - auto* newSpline = new Part::GeomBSplineCurve(newPoles, - newWeights, - newKnots, - newMults, - bsp1->getDegree(), - false, - true); + auto* newSpline = new Part::GeomBSplineCurve( + newPoles, + newWeights, + newKnots, + newMults, + bsp1->getDegree(), + false, + true + ); // int newGeoId = addGeometry(newSpline); std::vector newGeos {newSpline}; @@ -4139,10 +4150,10 @@ int SketchObject::join(int geoId1, setConstruction(geoId1, areOriginalCurvesConstruction); // 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; + auto otherPosId1 = (Sketcher::PointPos::start == posId1) ? Sketcher::PointPos::end + : Sketcher::PointPos::start; + auto otherPosId2 = (Sketcher::PointPos::start == posId2) ? Sketcher::PointPos::end + : Sketcher::PointPos::start; transferConstraints(geoId1, otherPosId1, geoId1, PointPos::start, true); transferConstraints(geoId2, otherPosId2, geoId1, PointPos::end, true); @@ -4331,10 +4342,12 @@ bool SketchObject::isCarbonCopyAllowed(App::Document* pDoc, App::DocumentObject* } // clang-format on -int SketchObject::addSymmetric(const std::vector& geoIdList, - int refGeoId, - Sketcher::PointPos refPosId, - bool addSymmetryConstraints) +int SketchObject::addSymmetric( + const std::vector& geoIdList, + int refGeoId, + Sketcher::PointPos refPosId, + bool addSymmetryConstraints +) { // no need to check input data validity as this is an sketchobject managed operation. Base::StateLocker lock(managedoperation, true); @@ -4348,15 +4361,16 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, // Find out if reference is aligned with V or H axis, // if so we can keep Vertical and Horizontal constraints in the mirrored geometry. bool refIsLine = refPosId == Sketcher::PointPos::none; - bool refIsAxisAligned = - refGeoId == Sketcher::GeoEnum::VAxis || refGeoId == Sketcher::GeoEnum::HAxis || !refIsLine + bool refIsAxisAligned = refGeoId == Sketcher::GeoEnum::VAxis + || refGeoId == Sketcher::GeoEnum::HAxis || !refIsLine || std::ranges::any_of(constrvals, [&refGeoId](auto* constr) { - return constr->First == refGeoId - && (constr->Type == Sketcher::Vertical || constr->Type == Sketcher::Horizontal); - }); + return constr->First == refGeoId + && (constr->Type == Sketcher::Vertical + || constr->Type == Sketcher::Horizontal); + }); - std::vector symgeos = - getSymmetric(geoIdList, geoIdMap, isStartEndInverted, refGeoId, refPosId); + std::vector symgeos + = getSymmetric(geoIdList, geoIdMap, isStartEndInverted, refGeoId, refPosId); { addGeometry(symgeos); @@ -4378,8 +4392,7 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, if (constr->Second == GeoEnum::GeoUndef /*&& constr->Third == GeoEnum::GeoUndef*/) { if (refIsAxisAligned - && (constr->Type == Sketcher::DistanceX - || constr->Type == Sketcher::DistanceY)) { + && (constr->Type == Sketcher::DistanceX || constr->Type == Sketcher::DistanceY)) { // In this case we want to keep the Vertical, Horizontal constraints. // DistanceX and DistanceY constraints should also be possible to keep in // this case, but keeping them causes segfault, not sure why. @@ -4412,9 +4425,11 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, // Second is also in the list - auto flipStartEndIfRelevant = [&isStartEndInverted](int geoId, - const Sketcher::PointPos posId, - Sketcher::PointPos& posIdNew) { + auto flipStartEndIfRelevant = [&isStartEndInverted]( + int geoId, + const Sketcher::PointPos posId, + Sketcher::PointPos& posIdNew + ) { if (isStartEndInverted[geoId]) { if (posId == Sketcher::PointPos::start) { posIdNew = Sketcher::PointPos::end; @@ -4519,14 +4534,18 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, if (!gf->isInternalAligned()) { // Note internal aligned lines (ellipse, parabola, hyperbola) are causing // redundant constraint. - createSymConstr(geoId1, - geoId2, - PointPos::start, - isStartEndInverted[geoId1] ? PointPos::end : PointPos::start); - createSymConstr(geoId1, - geoId2, - PointPos::end, - isStartEndInverted[geoId1] ? PointPos::start : PointPos::end); + createSymConstr( + geoId1, + geoId2, + PointPos::start, + isStartEndInverted[geoId1] ? PointPos::end : PointPos::start + ); + createSymConstr( + geoId1, + geoId2, + PointPos::end, + isStartEndInverted[geoId1] ? PointPos::start : PointPos::end + ); } } else if (geo->is() || geo->is()) { @@ -4538,14 +4557,18 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, || geo->is() // || geo->is()) { createEqualityConstr(geoId1, geoId2); - createSymConstr(geoId1, - geoId2, - PointPos::start, - isStartEndInverted[geoId1] ? PointPos::end : PointPos::start); - createSymConstr(geoId1, - geoId2, - PointPos::end, - isStartEndInverted[geoId1] ? PointPos::start : PointPos::end); + createSymConstr( + geoId1, + geoId2, + PointPos::start, + isStartEndInverted[geoId1] ? PointPos::end : PointPos::start + ); + createSymConstr( + geoId1, + geoId2, + PointPos::end, + isStartEndInverted[geoId1] ? PointPos::start : PointPos::end + ); } else if (geo->is()) { auto gf = GeometryFacade::getFacade(geo); @@ -4565,11 +4588,13 @@ int SketchObject::addSymmetric(const std::vector& geoIdList, return Geometry.getSize() - 1; } -std::vector SketchObject::getSymmetric(const std::vector& geoIdList, - std::map& geoIdMap, - std::map& isStartEndInverted, - int refGeoId, - Sketcher::PointPos refPosId) +std::vector SketchObject::getSymmetric( + const std::vector& geoIdList, + std::map& geoIdMap, + std::map& isStartEndInverted, + int refGeoId, + Sketcher::PointPos refPosId +) { using std::numbers::pi; @@ -4623,7 +4648,8 @@ std::vector SketchObject::getSymmetric(const std::vector& geosymline->setPoints( sp + 2.0 * (sp.Perpendicular(refGeoLine->getStartPoint(), vectline) - sp), - ep + 2.0 * (ep.Perpendicular(refGeoLine->getStartPoint(), vectline) - ep)); + ep + 2.0 * (ep.Perpendicular(refGeoLine->getStartPoint(), vectline) - ep) + ); isStartEndInverted.insert(std::make_pair(geoId, false)); } else if (geosym->is()) { @@ -4631,7 +4657,8 @@ std::vector SketchObject::getSymmetric(const std::vector& Base::Vector3d cp = geosymcircle->getCenter(); geosymcircle->setCenter( - cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp)); + cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp) + ); isStartEndInverted.insert(std::make_pair(geoId, false)); } else if (geosym->is()) { @@ -4640,17 +4667,15 @@ std::vector SketchObject::getSymmetric(const std::vector& Base::Vector3d ep = geoaoc->getEndPoint(true); Base::Vector3d cp = geoaoc->getCenter(); - Base::Vector3d ssp = - sp + 2.0 * (sp.Perpendicular(refGeoLine->getStartPoint(), vectline) - sp); - Base::Vector3d sep = - ep + 2.0 * (ep.Perpendicular(refGeoLine->getStartPoint(), vectline) - ep); - Base::Vector3d scp = - cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); + Base::Vector3d ssp = sp + + 2.0 * (sp.Perpendicular(refGeoLine->getStartPoint(), vectline) - sp); + Base::Vector3d sep = ep + + 2.0 * (ep.Perpendicular(refGeoLine->getStartPoint(), vectline) - ep); + Base::Vector3d scp = cp + + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); - double theta1 = - Base::fmod(atan2(sep.y - scp.y, sep.x - scp.x), 2.f * std::numbers::pi); - double theta2 = - Base::fmod(atan2(ssp.y - scp.y, ssp.x - scp.x), 2.f * std::numbers::pi); + double theta1 = Base::fmod(atan2(sep.y - scp.y, sep.x - scp.x), 2.f * std::numbers::pi); + double theta2 = Base::fmod(atan2(ssp.y - scp.y, ssp.x - scp.x), 2.f * std::numbers::pi); geoaoc->setCenter(scp); geoaoc->setRange(theta1, theta2, true); @@ -4666,10 +4691,10 @@ std::vector SketchObject::getSymmetric(const std::vector& double df = sqrt(majord * majord - minord * minord); Base::Vector3d f1 = cp + df * majdir; - Base::Vector3d sf1 = - f1 + 2.0 * (f1.Perpendicular(refGeoLine->getStartPoint(), vectline) - f1); - Base::Vector3d scp = - cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); + Base::Vector3d sf1 = f1 + + 2.0 * (f1.Perpendicular(refGeoLine->getStartPoint(), vectline) - f1); + Base::Vector3d scp = cp + + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); geosymellipse->setMajorAxisDir(sf1 - scp); @@ -4686,10 +4711,10 @@ std::vector SketchObject::getSymmetric(const std::vector& double df = sqrt(majord * majord - minord * minord); Base::Vector3d f1 = cp + df * majdir; - Base::Vector3d sf1 = - f1 + 2.0 * (f1.Perpendicular(refGeoLine->getStartPoint(), vectline) - f1); - Base::Vector3d scp = - cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); + Base::Vector3d sf1 = f1 + + 2.0 * (f1.Perpendicular(refGeoLine->getStartPoint(), vectline) - f1); + Base::Vector3d scp = cp + + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); geosymaoe->setMajorAxisDir(sf1 - scp); @@ -4718,10 +4743,10 @@ std::vector SketchObject::getSymmetric(const std::vector& double df = sqrt(majord * majord + minord * minord); Base::Vector3d f1 = cp + df * majdir; - Base::Vector3d sf1 = - f1 + 2.0 * (f1.Perpendicular(refGeoLine->getStartPoint(), vectline) - f1); - Base::Vector3d scp = - cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); + Base::Vector3d sf1 = f1 + + 2.0 * (f1.Perpendicular(refGeoLine->getStartPoint(), vectline) - f1); + Base::Vector3d scp = cp + + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); geosymaoe->setMajorAxisDir(sf1 - scp); @@ -4742,10 +4767,10 @@ std::vector SketchObject::getSymmetric(const std::vector& Base::Vector3d f1 = geosymaoe->getFocus(); - Base::Vector3d sf1 = - f1 + 2.0 * (f1.Perpendicular(refGeoLine->getStartPoint(), vectline) - f1); - Base::Vector3d scp = - cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); + Base::Vector3d sf1 = f1 + + 2.0 * (f1.Perpendicular(refGeoLine->getStartPoint(), vectline) - f1); + Base::Vector3d scp = cp + + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp); geosymaoe->setXAxisDir(sf1 - scp); geosymaoe->setCenter(scp); @@ -4778,7 +4803,8 @@ std::vector SketchObject::getSymmetric(const std::vector& Base::Vector3d cp = geosympoint->getPoint(); geosympoint->setPoint( - cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp)); + cp + 2.0 * (cp.Perpendicular(refGeoLine->getStartPoint(), vectline) - cp) + ); isStartEndInverted.insert(std::make_pair(geoId, false)); } else { @@ -4955,14 +4981,16 @@ std::vector SketchObject::getSymmetric(const std::vector& return symmetricVals; } -int SketchObject::addCopy(const std::vector& geoIdList, - const Base::Vector3d& displacement, - bool moveonly, - bool clone, - int csize, - int rsize, - bool constraindisplacement, - double perpscale) +int SketchObject::addCopy( + const std::vector& geoIdList, + const Base::Vector3d& displacement, + bool moveonly, + bool clone, + int csize, + int rsize, + bool constraindisplacement, + double perpscale +) { // no need to check input data validity as this is an sketchobject managed operation. Base::StateLocker lock(managedoperation, true); @@ -10952,7 +10980,8 @@ void SketchObject::migrateSketch() } auto ext = std::static_pointer_cast( - g->getExtension(Part::GeometryMigrationExtension::getClassTypeId()).lock()); + g->getExtension(Part::GeometryMigrationExtension::getClassTypeId()).lock() + ); if (!ext->testMigrationType(Part::GeometryMigrationExtension::Construction)) { continue; @@ -10960,8 +10989,8 @@ void SketchObject::migrateSketch() // at this point IA geometry is already migrated auto gf = GeometryFacade::getFacade(g); - bool oldConstr = - ext->getConstruction() || (g->is() && !gf->isInternalAligned()); + bool oldConstr = ext->getConstruction() + || (g->is() && !gf->isInternalAligned()); GeometryFacade::setConstruction(g, oldConstr); @@ -10973,8 +11002,7 @@ void SketchObject::migrateSketch() auto constraints = Constraints.getValues(); auto geometries = getInternalGeometry(); - bool parabolaFound = - std::ranges::any_of(geometries, &Part::Geometry::is); + bool parabolaFound = std::ranges::any_of(geometries, &Part::Geometry::is); if (!parabolaFound) { return; @@ -11009,21 +11037,24 @@ void SketchObject::migrateSketch() // look for a line from focusGeoId:start to Geoid:mid_external std::vector focusGeoIdListGeoIdList; std::vector focusPosIdList; - getDirectlyCoincidentPoints(focusGeoId, - Sketcher::PointPos::start, - focusGeoIdListGeoIdList, - focusPosIdList); + getDirectlyCoincidentPoints( + focusGeoId, + Sketcher::PointPos::start, + focusGeoIdListGeoIdList, + focusPosIdList + ); std::vector parabGeoIdListGeoIdList; std::vector parabposidlist; - getDirectlyCoincidentPoints(parabolaGeoId, - Sketcher::PointPos::mid, - parabGeoIdListGeoIdList, - parabposidlist); + getDirectlyCoincidentPoints( + parabolaGeoId, + Sketcher::PointPos::mid, + parabGeoIdListGeoIdList, + parabposidlist + ); for (const auto& parabGeoIdListGeoId : parabGeoIdListGeoIdList) { - auto iterParabolaGeoId = - std::ranges::find(focusGeoIdListGeoIdList, parabGeoIdListGeoId); + auto iterParabolaGeoId = std::ranges::find(focusGeoIdListGeoIdList, parabGeoIdListGeoId); if (iterParabolaGeoId != focusGeoIdListGeoIdList.end()) { axisGeoId2ParabolaGeoId[*iterParabolaGeoId] = parabolaGeoId; } @@ -11039,15 +11070,15 @@ void SketchObject::migrateSketch() continue; } - auto axisMajorCoincidentFound = - std::ranges::any_of(axisGeoId2ParabolaGeoId, [&](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); - }); + auto axisMajorCoincidentFound + = std::ranges::any_of(axisGeoId2ParabolaGeoId, [&](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) { // we skip this coincident, the other coincident on axis will be substituted @@ -11055,16 +11086,15 @@ void SketchObject::migrateSketch() continue; } - auto focusCoincidentFound = - std::ranges::find_if(axisGeoId2ParabolaGeoId, [&](const auto& pair) { - auto parabolaGeoId = pair.second; - auto axisgeoid = pair.first; - auto focusGeoId = parabolaGeoId2FocusGeoId[parabolaGeoId]; - return (c->First == axisgeoid && c->Second == focusGeoId - && c->SecondPos == PointPos::start) - || (c->Second == axisgeoid && c->First == focusGeoId - && c->FirstPos == PointPos::start); - }); + auto focusCoincidentFound = std::ranges::find_if(axisGeoId2ParabolaGeoId, [&](const auto& pair) { + auto parabolaGeoId = pair.second; + auto axisgeoid = pair.first; + auto focusGeoId = parabolaGeoId2FocusGeoId[parabolaGeoId]; + return (c->First == axisgeoid && c->Second == focusGeoId + && c->SecondPos == PointPos::start) + || (c->Second == axisgeoid && c->First == focusGeoId + && c->FirstPos == PointPos::start); + }); if (focusCoincidentFound != axisGeoId2ParabolaGeoId.end()) { auto* newConstr = new Sketcher::Constraint(); @@ -11090,9 +11120,12 @@ void SketchObject::migrateSketch() Base::Console().critical( this->getFullName(), - QT_TRANSLATE_NOOP("Notifications", - "Parabolas were migrated. Migrated files won't open in previous " - "versions of FreeCAD!!\n")); + QT_TRANSLATE_NOOP( + "Notifications", + "Parabolas were migrated. Migrated files won't open in previous " + "versions of FreeCAD!!\n" + ) + ); } // clang-format off diff --git a/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp b/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp index 9acdaf4236..ecc1b75fee 100644 --- a/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp +++ b/tests/src/Mod/Sketcher/App/SketchObjectChanges.cpp @@ -133,8 +133,10 @@ TEST_F(SketchObjectTest, testReplaceGeometriesOneToTwo) Part::GeomLineSegment lineSeg1; setupLineSegment(lineSeg1); int geoId1 = getObject()->addGeometry(&lineSeg1); - std::vector newCurves {createTypicalNonPeriodicBSpline().release(), - createTypicalNonPeriodicBSpline().release()}; + std::vector newCurves { + createTypicalNonPeriodicBSpline().release(), + createTypicalNonPeriodicBSpline().release() + }; // Act getObject()->replaceGeometries({geoId1}, newCurves);