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()) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 36e2c579c5..4afebcf61d 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -264,8 +264,9 @@ 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); @@ -273,17 +274,19 @@ void SketchObject::buildShape() Data::MappedName::fromRawData(name.c_str()), 0L); 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 @@ -2994,6 +2997,52 @@ 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 + // (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, + bool deleteIfOutside = true) { + 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 (deleteIfOutside && !((curve1Start <= u) && (u <= curve1End))) { + // TODO: Not on any of the curves. Delete? + delete_list.push_back(constrId); + changed = true; + } + + } + ++constrId; + } + + if (changed) { + this->Constraints.setValues(std::move(newConstraints)); + delConstraints(delete_list, false); + } + }; + // makes an equality constraint between GeoId1 and GeoId2 auto constrainAsEqual = [this](int GeoId1, int GeoId2) { auto newConstr = std::make_unique(); @@ -3276,6 +3325,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, isBSpline); + if (isDerivedFromTrimmedCurve) { static_cast(newVals[GeoId]) ->setRange(firstParam, point1Param); @@ -3330,8 +3381,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 @@ -3451,6 +3504,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); } 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); +}