From 1a6a7d07c2f304030dfcdaacff477ff3df6e16d3 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Fri, 31 May 2024 16:57:45 +0530 Subject: [PATCH 1/5] [Sketcher] Rearrange directives to appease syntax highlight Spacemacs doesn't handle the braces flow very well. This commit can be dropped if necessary. --- src/Mod/Sketcher/App/SketchObject.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index c595c57d8b..34f7bdaea9 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -260,25 +260,28 @@ void SketchObject::buildShape() ++i; if(GeometryFacade::getConstruction(geo)) continue; - if (geo->isDerivedFrom(Part::GeomPoint::getClassTypeId())) { + if (geo->isDerivedFrom()) #ifdef FC_USE_TNP_FIX + { Part::TopoShape vertex(TopoDS::Vertex(geo->toShape())); int idx = getVertexIndexGeoPos(i-1, Sketcher::PointPos::start); std::string name = convertSubName(Data::IndexedName::fromConst("Vertex", idx+1), false); vertices.push_back(vertex); vertices.back().copyElementMap(vertex, Part::OpCodes::Sketch); - } else { + } + else { auto indexedName = Data::IndexedName::fromConst("Edge", i); shapes.push_back(getEdge(geo,convertSubName(indexedName, false).c_str())); } #else + { vertices.emplace_back(TopoDS::Vertex(geo->toShape())); int idx = getVertexIndexGeoPos(i-1, PointPos::start); std::string name = convertSubName(Data::IndexedName::fromConst("Vertex", idx+1), false); - - } else + } + else shapes.push_back(getEdge(geo,convertSubName( Data::IndexedName::fromConst("Edge", i), false).c_str())); #endif From e1c3dd2fea1a674781654003911e0416032c09f6 Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Wed, 5 Jun 2024 18:24:04 +0530 Subject: [PATCH 2/5] [Part][test] Add tests for `GeomBSplineCurve::Trim()` --- tests/src/Mod/Part/App/CMakeLists.txt | 1 + tests/src/Mod/Part/App/Geometry.cpp | 92 +++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 tests/src/Mod/Part/App/Geometry.cpp diff --git a/tests/src/Mod/Part/App/CMakeLists.txt b/tests/src/Mod/Part/App/CMakeLists.txt index bfd77aea3b..3a3e19dade 100644 --- a/tests/src/Mod/Part/App/CMakeLists.txt +++ b/tests/src/Mod/Part/App/CMakeLists.txt @@ -16,6 +16,7 @@ target_sources( ${CMAKE_CURRENT_SOURCE_DIR}/FeaturePartCut.cpp ${CMAKE_CURRENT_SOURCE_DIR}/FeaturePartFuse.cpp ${CMAKE_CURRENT_SOURCE_DIR}/FeatureRevolution.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/Geometry.cpp ${CMAKE_CURRENT_SOURCE_DIR}/PartFeature.cpp ${CMAKE_CURRENT_SOURCE_DIR}/PartFeatures.cpp ${CMAKE_CURRENT_SOURCE_DIR}/PartTestHelpers.cpp diff --git a/tests/src/Mod/Part/App/Geometry.cpp b/tests/src/Mod/Part/App/Geometry.cpp new file mode 100644 index 0000000000..025bced432 --- /dev/null +++ b/tests/src/Mod/Part/App/Geometry.cpp @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later + +#include + +#include +#include "Mod/Part/App/Geometry.h" +#include +#include +#include "PartTestHelpers.h" +#include "App/MappedElement.h" + +// using namespace Part; +// using namespace PartTestHelpers; + +class GeometryTest: public ::testing::Test, public PartTestHelpers::PartTestHelperClass +{ +protected: + static void SetUpTestSuite() + { + tests::initApplication(); + } + + void SetUp() override + { + createTestDoc(); + } + + void TearDown() override + {} +}; + +TEST_F(GeometryTest, testTrimBSpline) +{ + // Arrange + // create arbitrary B-splines periodic and non-periodic, with arbitrary knots + // NOTE: Avoid B-spline with typical knots like those ranging from [0,1] or [-1,1] + int degree = 3; + std::vector poles; + poles.emplace_back(1, 0, 0); + poles.emplace_back(1, 1, 0); + poles.emplace_back(1, 0.5, 0); + poles.emplace_back(0, 1, 0); + poles.emplace_back(0, 0, 0); + std::vector weights(5, 1.0); + std::vector knotsNonPeriodic = {0.0, 1.0, 2.0}; + std::vector multiplicitiesNonPeriodic = {degree + 1, 1, degree + 1}; + Part::GeomBSplineCurve nonPeriodicBSpline1(poles, + weights, + knotsNonPeriodic, + multiplicitiesNonPeriodic, + degree, + false); + Part::GeomBSplineCurve nonPeriodicBSpline2(poles, + weights, + knotsNonPeriodic, + multiplicitiesNonPeriodic, + degree, + false); + std::vector knotsPeriodic = {0.0, 0.3, 1.0, 1.5, 1.8, 2.0}; + double period = knotsPeriodic.back() - knotsPeriodic.front(); + std::vector multiplicitiesPeriodic(6, 1); + Part::GeomBSplineCurve periodicBSpline1(poles, + weights, + knotsPeriodic, + multiplicitiesPeriodic, + degree, + true); + Part::GeomBSplineCurve periodicBSpline2(poles, + weights, + knotsPeriodic, + multiplicitiesPeriodic, + degree, + true); + // NOTE: These should be within the knot range, with param1 < param2 + double param1 = 0.5, param2 = 1.4; + // TODO: Decide what to do if params are outside the range + + // Act + periodicBSpline1.Trim(param1, param2); + periodicBSpline2.Trim(param2, param1); + nonPeriodicBSpline1.Trim(param1, param2); + // TODO: What happens when a non-periodic B-spline is trimmed this way? + nonPeriodicBSpline2.Trim(param2, param1); + + // Assert + EXPECT_DOUBLE_EQ(periodicBSpline1.getFirstParameter(), param1); + EXPECT_DOUBLE_EQ(periodicBSpline1.getLastParameter(), param2); + EXPECT_DOUBLE_EQ(periodicBSpline2.getFirstParameter(), param2); + EXPECT_DOUBLE_EQ(periodicBSpline2.getLastParameter(), param1 + period); + EXPECT_DOUBLE_EQ(nonPeriodicBSpline1.getFirstParameter(), param1); + EXPECT_DOUBLE_EQ(nonPeriodicBSpline1.getLastParameter(), param2); +} From dbef3747ebf83d95e94377e78a88ba45eef6fe7a Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Sun, 2 Jun 2024 23:07:28 +0530 Subject: [PATCH 3/5] [Part] Do not assume default period in B-spline --- src/Mod/Part/App/Geometry.cpp | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/Mod/Part/App/Geometry.cpp b/src/Mod/Part/App/Geometry.cpp index 9f52a3fd60..046d2624d6 100644 --- a/src/Mod/Part/App/Geometry.cpp +++ b/src/Mod/Part/App/Geometry.cpp @@ -1894,19 +1894,10 @@ void GeomBSplineCurve::Trim(double u, double v) }; try { - if(!isPeriodic()) { - splitUnwrappedBSpline(u, v); - } - else { // periodic - if( v < u ) { // wraps over origin - v = v + 1.0; // v needs one extra lap (1.0) - - splitUnwrappedBSpline(u, v); - } - else { - splitUnwrappedBSpline(u, v); - } + if (isPeriodic() && (v < u)) { + v = v + (getLastParameter() - getFirstParameter()); // v needs one extra lap } + splitUnwrappedBSpline(u, v); } catch (Standard_Failure& e) { THROWM(Base::CADKernelError,e.GetMessageString()) From ee2d32474429582645370b33d6d37e8470f686fa Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Wed, 5 Jun 2024 02:58:39 +0530 Subject: [PATCH 4/5] [Sketcher] Fix some issues with trimming splines 1. Transfer point-on-object to the correct resulting curve: Currently the constraint remains on the "start" curve created after the trim. 2. Expose internal geometries of both resulting B-splines. --- src/Mod/Sketcher/App/SketchObject.cpp | 46 ++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 34f7bdaea9..e4f4cc526a 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -2992,6 +2992,46 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) delConstraints(delete_list, false); }; + // Pick the appropriate portion which should inherit the point-on-object constraint + // Assume that GeoId1 is the pre-existing curve + auto chooseCurveForPointOnObject = [this](int GeoId1, + double curve1Start, + double curve1End, + int GeoId2, + double curve2Start, + double curve2End) { + const Part::GeomCurve* curve = static_cast(this->getGeometry(GeoId1)); + const std::vector& constraints = this->Constraints.getValues(); + std::vector newConstraints(constraints); + int constrId = 0; + std::vector delete_list; + bool changed = false; + for (const auto* constr : constraints) { + // There is a preexisting PointOnObject constraint, see where it belongs + if (constr->Type == Sketcher::PointOnObject && constr->Second == GeoId1) { + Base::Vector3d pp = getPoint(constr->First, constr->FirstPos); + double u; + curve->closestParameter(pp, u); + if ((curve2Start <= u) && (u <= curve2End)) { + std::unique_ptr constrNew(newConstraints[constrId]->clone()); + constrNew->Second = GeoId2; + Constraint* constrPtr = constrNew.release(); + newConstraints[constrId] = constrPtr; + changed = true; + } + else if (!((curve1Start <= u) && (u <= curve1End))) { + // TODO: Not on any of the curves. Delete? + } + + } + ++constrId; + } + + if (changed) { + this->Constraints.setValues(std::move(newConstraints)); + } + }; + // makes an equality constraint between GeoId1 and GeoId2 auto constrainAsEqual = [this](int GeoId1, int GeoId2) { auto newConstr = std::make_unique(); @@ -3274,6 +3314,8 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) newVals.push_back(newVals[GeoId]->clone()); int newGeoId = newVals.size() - 1; + chooseCurveForPointOnObject(GeoId, firstParam, point1Param, newGeoId, point2Param, lastParam); + if (isDerivedFromTrimmedCurve) { static_cast(newVals[GeoId]) ->setRange(firstParam, point1Param); @@ -3328,8 +3370,10 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) Sketcher::PointPos::mid); } - if (isNonPeriodicBSpline) + if (isNonPeriodicBSpline) { exposeInternalGeometry(GeoId); + exposeInternalGeometry(newGeoId); + } // if we do not have a recompute, the sketch must be solved to update the DoF of the // solver From bf649e241914eb74daffdfec014b4311b91b341a Mon Sep 17 00:00:00 2001 From: Ajinkya Dahale Date: Wed, 5 Jun 2024 12:27:10 +0530 Subject: [PATCH 5/5] [Sketcher] Delete point-on-object on trimmed portion This could cause undesirable shape change, especially in B-splines. --- src/Mod/Sketcher/App/SketchObject.cpp | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index e4f4cc526a..18ef861b57 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -2993,13 +2993,16 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) }; // Pick the appropriate portion which should inherit the point-on-object constraint + // (or delete it altogether if it's not on any curve) // Assume that GeoId1 is the pre-existing curve + // GeoId2 can be same as GeoId1 (for example for periodic B-spline) auto chooseCurveForPointOnObject = [this](int GeoId1, double curve1Start, double curve1End, int GeoId2, double curve2Start, - double curve2End) { + double curve2End, + bool deleteIfOutside = true) { const Part::GeomCurve* curve = static_cast(this->getGeometry(GeoId1)); const std::vector& constraints = this->Constraints.getValues(); std::vector newConstraints(constraints); @@ -3019,8 +3022,10 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) newConstraints[constrId] = constrPtr; changed = true; } - else if (!((curve1Start <= u) && (u <= curve1End))) { + else if (deleteIfOutside && !((curve1Start <= u) && (u <= curve1End))) { // TODO: Not on any of the curves. Delete? + delete_list.push_back(constrId); + changed = true; } } @@ -3029,6 +3034,7 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) if (changed) { this->Constraints.setValues(std::move(newConstraints)); + delConstraints(delete_list, false); } }; @@ -3314,7 +3320,7 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) newVals.push_back(newVals[GeoId]->clone()); int newGeoId = newVals.size() - 1; - chooseCurveForPointOnObject(GeoId, firstParam, point1Param, newGeoId, point2Param, lastParam); + chooseCurveForPointOnObject(GeoId, firstParam, point1Param, newGeoId, point2Param, lastParam, isBSpline); if (isDerivedFromTrimmedCurve) { static_cast(newVals[GeoId]) @@ -3493,6 +3499,14 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point) else if (isPeriodicBSpline) { auto bspline = std::unique_ptr( static_cast(geo->clone())); + // Delete any point-on-object constraints on trimmed portion. + // Such constraint can cause undesirable shape change. + chooseCurveForPointOnObject(GeoId, + bspline->getFirstParameter(), + std::min(point1Param, point2Param), + GeoId, + std::max(point1Param, point2Param), + bspline->getLastParameter()); bspline->Trim(point2Param, point1Param); geoNew = std::move(bspline); }