kindred/create
1
1
Fork 0
Code Issues 37 Pull Requests Actions Packages Projects 9 Releases 2 Wiki Activity

[Sketcher] Adjust constraint changes when trimming

Browse Source 
1. No longer applying equality constraints between new (circular) pieces since
they may cause issues.
2. Only transfer equality with a different curve to one of the pieces.
3. Re-added certain constraints (that applied to both ends of the original
curve) that were incorrectly excluded from modification/deletion at a certain
step.
4. Use C++20 `std::erase_if()` in trim
This commit is contained in:
Ajinkya Dahale
2025-03-09 21:07:09 +05:30
committed by Benjamin Nauck
parent 2727550e90
commit 41e1b647ae
1 changed files with 38 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

77
src/Mod/Sketcher/App/SketchObject.cpp
View File

@@ -22,6 +22,7 @@
***************************************************************************/
#include "PreCompiled.h"
#include <algorithm>
#ifndef _PreComp_
#include <cmath>
#include <limits>
@@ -3624,6 +3625,7 @@ std::unique_ptr<Constraint> transformPreexistingConstraintForTrim(const SketchOb
* is set to Sketcher::Tangent, that the secondPos1 is captured from the PointOnObject,
* and also make sure that the PointOnObject constraint is deleted.
*/
// TODO: Symmetric and distance constraints (sometimes together) can be changed to something
std::unique_ptr<Constraint> newConstr;
if (!constr->involvesGeoId(cuttingGeoId)
|| !constr->involvesGeoIdAndPosId(GeoId, PointPos::none)) {
@@ -3635,7 +3637,7 @@ std::unique_ptr<Constraint> transformPreexistingConstraintForTrim(const SketchOb
// coincidence At this stage of the check the point has to be an end of `cuttingGeoId`
// on the edge of `GeoId`.
if (isPointAtPosition(obj, constr->First, constr->FirstPos, cutPointVec)) {
// This concerns the start portion of the trim
// We already know the point-on-object is on the whole of GeoId
newConstr.reset(constr->copy());
newConstr->Type = Sketcher::Coincident;
newConstr->Second = newGeoId;
@@ -3924,34 +3926,23 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
// remove the constraints that we want to manually transfer
// We could transfer beforehand but in case of exception that transfer is permanent
if (!isClosedCurve(geoAsCurve)) {
if (cuttingGeoIds[0] != GeoEnum::GeoUndef) {
idsOfOldConstraints.erase(
std::remove_if(
idsOfOldConstraints.begin(),
idsOfOldConstraints.end(),
[&GeoId, &allConstraints](const auto& i) {
return (allConstraints[i]->involvesGeoIdAndPosId(GeoId, PointPos::start));
}),
idsOfOldConstraints.end());
}
if (cuttingGeoIds[1] != GeoEnum::GeoUndef) {
idsOfOldConstraints.erase(
std::remove_if(idsOfOldConstraints.begin(),
idsOfOldConstraints.end(),
[&GeoId, &allConstraints](const auto& i) {
return (allConstraints[i]->involvesGeoIdAndPosId(GeoId,
PointPos::end));
}),
idsOfOldConstraints.end());
}
std::erase_if(idsOfOldConstraints,
[&GeoId, &allConstraints, &cuttingGeoIds](const auto& i) {
auto* constr = allConstraints[i];
bool involvesStart =
constr->involvesGeoIdAndPosId(GeoId, PointPos::start);
bool involvesEnd = constr->involvesGeoIdAndPosId(GeoId, PointPos::end);
bool keepStart = cuttingGeoIds[0] != GeoEnum::GeoUndef;
bool keepEnd = cuttingGeoIds[1] != GeoEnum::GeoUndef;
bool involvesBothButNotBothKept =
involvesStart && involvesEnd && !(keepStart && keepEnd);
return !involvesBothButNotBothKept
&& ((involvesStart && keepStart) || (involvesEnd && keepEnd));
});
}
idsOfOldConstraints.erase(
std::remove_if(idsOfOldConstraints.begin(),
idsOfOldConstraints.end(),
[&GeoId, &allConstraints](const auto& i) {
return (allConstraints[i]->involvesGeoIdAndPosId(GeoId, PointPos::mid));
}),
idsOfOldConstraints.end());
std::erase_if(idsOfOldConstraints, [&GeoId, &allConstraints](const auto& i) {
return (allConstraints[i]->involvesGeoIdAndPosId(GeoId, PointPos::mid));
});
createNewConstraintsForTrim(this,
GeoId,
@@ -3966,7 +3957,7 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
//****************************************//
// Constraints related to start/mid/end points of original
auto constrainAsEqual = [this](int GeoId1, int GeoId2) {
[[maybe_unused]] auto constrainAsEqual = [this](int GeoId1, int GeoId2) {
auto newConstr = std::make_unique<Sketcher::Constraint>();
// Build Constraints associated with new pair of arcs
@@ -3999,12 +3990,21 @@ int SketchObject::trim(int GeoId, const Base::Vector3d& point)
newConstraints.push_back(joint);
// Any radius etc. equality constraints here
constrainAsEqual(newIds.front(), newIds.back());
// TODO: There could be some form of equality between the constraints here. However, it
// may happen that this is imposed by an elaborate set of additional constraints. When
// that happens, this causes redundant constraints, and in worse cases (incorrect)
// complaints of over-constraint and solver failures.
// if (std::none_of(newConstraints.begin(), newConstraints.end(), [](const auto& constr)
// {
// return constr->Type == ConstraintType::Equal;
// })) {
// constrainAsEqual(newIds.front(), newIds.back());
// }
// TODO: ensure alignment as well?
}
}
// TODO: Implement this
replaceGeometries({GeoId}, newGeos);
if (noRecomputes) {
@@ -4016,12 +4016,9 @@ 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());
std::erase_if(newConstraints, [](const auto& constr) {
return constr->Type == ConstraintType::None;
});
delGeometries(geoIdsToBeDeleted.begin(), geoIdsToBeDeleted.end());
addConstraints(newConstraints);
@@ -4152,8 +4149,10 @@ bool SketchObject::deriveConstraintsForPieces(const int oldId,
case Radius:
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>();
// Only transfer to one of them (arbitrarily chosen here as the first)
Constraint* trans = con->copy();
trans->substituteIndex(oldId, newIds.front());
newConstraints.push_back(trans);
break;
}
default: