Skip to content

Commit

Permalink
[Sketcher] Fix some issues in trim
Browse files Browse the repository at this point in the history 
Fixes fails at cases where one (or both) of the remaining segments is (are)
degenerate. This existed pre-refactor.

Fixes cases where there are some constraints on internal geometry that do not
get deleted cleanly.

Also fixes a memory leak.
  • Loading branch information
@AjinkyaDahale
AjinkyaDahale committed Jan 16, 2025
1 parent e4f906c commit 226d247
Showing 1 changed file with 47 additions and 24 deletions.
71 changes: 47 additions & 24 deletions src/Mod/Sketcher/App/SketchObject.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3585,6 +3585,12 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
return ((point1 - point2).Length() < 500 * Precision::Confusion());
};

auto areParamsWithinApproximation = [](double param1, double param2) {
// From testing: 500x (or 0.000050) is needed in order to not falsely distinguish points
// calculated with seekTrimPoints
return (std::abs(param1 - param2) < Precision::PApproximation());
};

// returns true if the point defined by (GeoId1, pos1) can be considered to be coincident with
// point.
auto isPointAtPosition =
Expand All @@ -3611,11 +3617,15 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
* and also make sure that the PointOnObject constraint is deleted.
*/
std::unique_ptr<Constraint> newConstr;
if (!constr->involvesGeoId(cuttingGeoId)
|| !constr->involvesGeoIdAndPosId(GeoId, PointPos::none)) {
return newConstr;
}
switch (constr->Type) {
case PointOnObject: {
// we might want to transform this (and the new point-on-object constraints) into a coincidence
if (constr->First == cuttingGeoId
&& isPointAtPosition(constr->First, constr->FirstPos, cutPointVec)) {
// At this stage of the check the point has to be an end of `cuttingGeoId` on the edge of `GeoId`.
if (isPointAtPosition(constr->First, constr->FirstPos, cutPointVec)) {
// This concerns the start portion of the trim
newConstr.reset(constr->copy());
newConstr->Type = Sketcher::Coincident;
Expand All @@ -3628,10 +3638,6 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
case Perpendicular: {
// These may have to be turned into endpoint-to-endpoint or endpoint-to-edge
// TODO: could there be tangent/perpendicular constraints not involving the trim that are modified below?
if (!constr->involvesGeoId(cuttingGeoId)
|| !constr->involvesGeoIdAndPosId(GeoId, PointPos::none)) {
break;
}
newConstr.reset(constr->copy());
newConstr->substituteIndexAndPos(GeoId, PointPos::none, newGeoId, newPosId);
// make sure the first position is a point
Expand Down Expand Up @@ -3670,10 +3676,6 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
// Points at the intersection
std::vector<Base::Vector3d> cutPoints(2);

// Remove internal geometry beforehand so cuttingGeoIds[0] and cuttingGeoIds[1] do not change
deleteUnusedInternalGeometryAndUpdateGeoId(GeoId);
geo = getGeometry(GeoId);

// Using SketchObject wrapper, as Part2DObject version returns GeoId = -1 when intersection not
// found, which is wrong for a GeoId (axis). seekTrimPoints returns:
// - For a parameter associated with "point" between an intersection and the end point
Expand All @@ -3695,34 +3697,44 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
return 0;
}

size_t numUndefs = std::count(cuttingGeoIds.begin(), cuttingGeoIds.end(), GeoEnum::GeoUndef);

if (numUndefs == 0 && arePointsWithinPrecision(cutPoints[0], cutPoints[1])) {
// If both points are detected and are coincident, deletion is the only option.
delGeometry(GeoId);
return 0;
}

const auto* geoAsCurve = static_cast<const Part::GeomCurve*>(geo);
double firstParam = geoAsCurve->getFirstParameter();
double lastParam = geoAsCurve->getLastParameter();
double pointParam, cut0Param, cut1Param;
double pointParam, cut0Param{firstParam}, cut1Param{lastParam};
bool allParamsFound = getIntersectionParameter(geo, point, pointParam)
&& getIntersectionParameter(geo, cutPoints[0], cut0Param)
&& getIntersectionParameter(geo, cutPoints[1], cut1Param);
if (!allParamsFound) {
return -1;
}

if (!isClosedCurve(geo) && areParamsWithinApproximation(firstParam, cut0Param)) {
cuttingGeoIds[0] = GeoEnum::GeoUndef;
}

if (!isClosedCurve(geo) && areParamsWithinApproximation(lastParam, cut1Param)) {
cuttingGeoIds[1] = GeoEnum::GeoUndef;
}

size_t numUndefs = std::count(cuttingGeoIds.begin(), cuttingGeoIds.end(), GeoEnum::GeoUndef);

if (numUndefs == 0 && arePointsWithinPrecision(cutPoints[0], cutPoints[1])) {
// If both points are detected and are coincident, deletion is the only option.
delGeometry(GeoId);
return 0;
}

bool startPointRemains = (cuttingGeoIds[0] != GeoEnum::GeoUndef);
bool endPointRemains = (cuttingGeoIds[1] != GeoEnum::GeoUndef);
std::vector<std::pair<double, double>> paramsOfNewGeos(2 - numUndefs, {firstParam, lastParam});
std::vector<int> newIds;
std::vector<Part::Geometry*> newGeos;
std::vector<const Part::Geometry*> newGeosAsConsts;
// TODO: This should be needed, but for some reason we already get both curves as construction
// when needed bool oldGeoIsConstruction = GeometryFacade::getConstruction(static_cast<const
// Part::GeomCurve*>(geo));
// when needed.

// bool oldGeoIsConstruction =
// GeometryFacade::getConstruction(static_cast<const Part::GeomCurve*>(geo));

if (isClosedCurve(geo) && !paramsOfNewGeos.empty()) {
startPointRemains = false;
Expand Down Expand Up @@ -3789,7 +3801,6 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
// Make centers coincident
if (newIds.size() > 1) {
Constraint* joint = new Constraint();
joint = new Constraint();
joint->Type = Coincident;
joint->First = newIds.front();
joint->FirstPos = PointPos::mid;
Expand Down Expand Up @@ -3840,6 +3851,11 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
isPoint2ConstrainedOnGeoId2 = true;
continue;
}
// We have already transferred all constraints on endpoints to the new pieces.
// If there is still any left, this means one of the remaining pieces was degenerate.
if (!con->involvesGeoIdAndPosId(GeoId, PointPos::none)) {
continue;
}
// constraint has not yet been changed
deriveConstraintsForPieces(GeoId, newIds, newGeosAsConsts, con, newConstraints);
}
Expand Down Expand Up @@ -3893,6 +3909,7 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)

auto vals = getInternalGeometry();
auto newVals(vals);
GeometryFacade::copyId(geo, newGeos.front());
newVals[GeoId] = newGeos.front();
if (newGeos.size() > 1) {
newVals.push_back(newGeos.back());
Expand All @@ -3914,6 +3931,13 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
changeConstraintAfterDeletingGeo(cons, deletedGeoId);
}
}
newConstraints.erase(std::remove_if(newConstraints.begin(),
newConstraints.end(),
[](const auto& constr) {
return constr->Type == ConstraintType::None;
}),
newConstraints.end());
delGeometries(geoIdsToBeDeleted.begin(), geoIdsToBeDeleted.end());
addConstraints(newConstraints);

if (noRecomputes) {
Expand Down Expand Up @@ -4039,8 +4063,7 @@ bool SketchObject::deriveConstraintsForPieces(const int oldId,
case Diameter:
case Equal: {
// FIXME: This sounds good by itself, but results in redundant constraints when equality is applied between newIds
transferToAll = geo->is<Part::GeomCircle>()
|| geo->is<Part::GeomArcOfCircle>();
transferToAll = geo->is<Part::GeomCircle>() || geo->is<Part::GeomArcOfCircle>();
break;
}
default:
Expand Down

0 comments on commit 226d247

Please sign in to comment.