diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index e04352df98..45b78faad2 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -320,14 +320,19 @@ int SketchObject::setDatum(int ConstrId, double Datum) // copy the list std::vector newVals(vals); - // clone the changed Constraint - Constraint *constNew = vals[ConstrId]->clone(); - constNew->setValue(Datum); - newVals[ConstrId] = constNew; - this->Constraints.setValues(newVals); - delete constNew; + + for(size_t i=0; iclone(); + + if((int)i == ConstrId) { + newVals[i]->setValue(Datum); + } + } + + this->Constraints.setValues(std::move(newVals)); int err = solve(); + if (err) this->Constraints.setValues(vals); @@ -345,14 +350,20 @@ int SketchObject::setDriving(int ConstrId, bool isdriving) // copy the list std::vector newVals(vals); + // clone the changed Constraint - Constraint *constNew = vals[ConstrId]->clone(); - constNew->isDriving = isdriving; - newVals[ConstrId] = constNew; - this->Constraints.setValues(newVals); + for(size_t i=0; iclone(); + + if((int)i == ConstrId) { + newVals[i]->isDriving = isdriving; + } + } + + this->Constraints.setValues(std::move(newVals)); + if (!isdriving) setExpression(Constraints.createPath(ConstrId), boost::shared_ptr()); - delete constNew; if(noRecomputes) // if we do not have a recompute, the sketch must be solved to update the DoF of the solver solve(); @@ -396,14 +407,23 @@ int SketchObject::toggleDriving(int ConstrId) // copy the list std::vector newVals(vals); - // clone the changed Constraint - Constraint *constNew = vals[ConstrId]->clone(); - constNew->isDriving = !constNew->isDriving; - newVals[ConstrId] = constNew; - this->Constraints.setValues(newVals); - if (!constNew->isDriving) + + bool isdriving = newVals[ConstrId]->isDriving; + + // deep copy + for(size_t i=0; iclone(); + + if((int)i == ConstrId) { + newVals[i]->isDriving = !newVals[i]->isDriving; + } + } + + this->Constraints.setValues(std::move(newVals)); + + if (!isdriving) setExpression(Constraints.createPath(ConstrId), boost::shared_ptr()); - delete constNew; + if(noRecomputes) // if we do not have a recompute, the sketch must be solved to update the DoF of the solver solve(); @@ -436,13 +456,17 @@ int SketchObject::setActive(int ConstrId, bool isactive) // copy the list std::vector newVals(vals); - // clone the changed Constraint - Constraint *constNew = vals[ConstrId]->clone(); - constNew->isActive = isactive; - newVals[ConstrId] = constNew; - this->Constraints.setValues(newVals); - delete constNew; + // deep copy + for(size_t i=0; iclone(); + + if((int)i == ConstrId) { + newVals[i]->isActive = isactive; + } + } + + this->Constraints.setValues(std::move(newVals)); if(noRecomputes) // if we do not have a recompute, the sketch must be solved to update the DoF of the solver solve(); @@ -471,13 +495,17 @@ int SketchObject::toggleActive(int ConstrId) // copy the list std::vector newVals(vals); - // clone the changed Constraint - Constraint *constNew = vals[ConstrId]->clone(); - constNew->isActive = !constNew->isActive; - newVals[ConstrId] = constNew; - this->Constraints.setValues(newVals); - delete constNew; + // deep copy + for(size_t i=0; iclone(); + + if((int)i == ConstrId) { + newVals[i]->isActive = !newVals[i]->isActive;; + } + } + + this->Constraints.setValues(std::move(newVals)); if(noRecomputes) // if we do not have a recompute, the sketch must be solved to update the DoF of the solver solve(); @@ -492,27 +520,22 @@ int SketchObject::setDatumsDriving(bool isdriving) const std::vector &vals = this->Constraints.getValues(); std::vector newVals(vals); - std::vector< Constraint * > tbd; // list of dynamically allocated memory that need to be deleted; + for(size_t i=0; iclone(); - for (size_t i=0; iclone(); - constNew->isDriving = isdriving; - newVals[i] = constNew; - tbd.push_back(constNew); - } + if (!testDrivingChange(i, isdriving)) + newVals[i]->isDriving = isdriving; } - this->Constraints.setValues(newVals); - for (size_t i = 0; i < newVals.size(); i++) { - if (!isdriving && newVals[i]->isDimensional()) + this->Constraints.setValues(std::move(newVals)); // we already did a deep copy of all pointers, so avoid one in setValues + + const std::vector &uvals = this->Constraints.getValues(); // newVals is a shell now + + for (size_t i = 0; i < uvals.size(); i++) { + if (!isdriving && uvals[i]->isDimensional()) setExpression(Constraints.createPath(i), boost::shared_ptr()); } - for (auto &t : tbd) - delete t; - if (noRecomputes) // if we do not have a recompute, the sketch must be solved to update the DoF of the solver solve(); @@ -562,14 +585,16 @@ int SketchObject::setVirtualSpace(int ConstrId, bool isinvirtualspace) // copy the list std::vector newVals(vals); - // clone the changed Constraint - Constraint *constNew = vals[ConstrId]->clone(); - constNew->isInVirtualSpace = isinvirtualspace; - newVals[ConstrId] = constNew; + // deep copy + for(size_t i=0; iclone(); - this->Constraints.setValues(newVals); + if((int)i == ConstrId) { + newVals[i]->isInVirtualSpace = isinvirtualspace; + } + } - delete constNew; + this->Constraints.setValues(std::move(newVals)); return 0; } @@ -595,14 +620,16 @@ int SketchObject::toggleVirtualSpace(int ConstrId) // copy the list std::vector newVals(vals); - // clone the changed Constraint - Constraint *constNew = vals[ConstrId]->clone(); - constNew->isInVirtualSpace = !constNew->isInVirtualSpace; - newVals[ConstrId] = constNew; + // deep copy + for(size_t i=0; iclone(); - this->Constraints.setValues(newVals); + if((int)i == ConstrId) { + newVals[i]->isInVirtualSpace = !newVals[i]->isInVirtualSpace; + } + } - delete constNew; + this->Constraints.setValues(std::move(newVals)); return 0; } @@ -824,24 +851,25 @@ int SketchObject::addGeometry(const std::vector &geoList, bool { const std::vector< Part::Geometry * > &vals = getInternalGeometry(); - std::vector< Part::Geometry * > newVals(vals); - std::vector< Part::Geometry * > copies; - copies.reserve(geoList.size()); - for (std::vector::const_iterator it = geoList.begin(); it != geoList.end(); ++it) { - Part::Geometry* copy = (*it)->copy(); + std::vector< Part::Geometry * > newVals; + + newVals.reserve(vals.size()+geoList.size()); + + for( auto & v : vals) + newVals.push_back(v->clone()); + + for( auto & v : geoList) { + Part::Geometry* copy = v->copy(); + if(construction && copy->getTypeId() != Part::GeomPoint::getClassTypeId()) { copy->Construction = construction; } - copies.push_back(copy); + newVals.push_back(copy); } - newVals.insert(newVals.end(), copies.begin(), copies.end()); // On setting geometry the onChanged method will call acceptGeometry(), thereby updating constraint geometry indices and rebuilding the vertex index - Geometry.setValues(newVals); - for (std::vector::iterator it = copies.begin(); it != copies.end(); ++it) - delete *it; - + Geometry.setValues(std::move(newVals)); return Geometry.getSize()-1; } @@ -850,16 +878,22 @@ int SketchObject::addGeometry(const Part::Geometry *geo, bool construction/*=fal { const std::vector< Part::Geometry * > &vals = getInternalGeometry(); - std::vector< Part::Geometry * > newVals(vals); + std::vector< Part::Geometry * > newVals; + + newVals.reserve(vals.size()+1); + + for( auto & v : vals) + newVals.push_back(v->clone()); + Part::Geometry *geoNew = geo->copy(); if(geoNew->getTypeId() != Part::GeomPoint::getClassTypeId()) geoNew->Construction = construction; newVals.push_back(geoNew); + // On setting geometry the onChanged method will call acceptGeometry(), thereby updating constraint geometry indices and rebuilding the vertex index - Geometry.setValues(newVals); - delete geoNew; + Geometry.setValues(std::move(newVals)); return Geometry.getSize()-1; } @@ -973,15 +1007,20 @@ int SketchObject::toggleConstruction(int GeoId) std::vector< Part::Geometry * > newVals(vals); - Part::Geometry *geoNew = newVals[GeoId]->clone(); - geoNew->Construction = !geoNew->Construction; - newVals[GeoId]=geoNew; + // deep copy + for(size_t i=0; iclone(); + + if((int)i == GeoId) { + newVals[i]->Construction = !newVals[i]->Construction; + } + } // There is not actual intertransaction going on here, however for a toggle neither the geometry indices nor the vertices need to be updated // so this is a convenient way of preventing it. { Base::StateLocker lock(internaltransaction, true); - this->Geometry.setValues(newVals); + this->Geometry.setValues(std::move(newVals)); } solverNeedsUpdate=true; @@ -999,15 +1038,20 @@ int SketchObject::setConstruction(int GeoId, bool on) std::vector< Part::Geometry * > newVals(vals); - Part::Geometry *geoNew = newVals[GeoId]->clone(); - geoNew->Construction = on; - newVals[GeoId]=geoNew; + // deep copy + for(size_t i=0; iclone(); + + if((int)i == GeoId) { + newVals[i]->Construction = on; + } + } // There is not actual intertransaction going on here, however for a toggle neither the geometry indices nor the vertices need to be updated // so this is a convenient way of preventing it. { Base::StateLocker lock(internaltransaction, true); - this->Geometry.setValues(newVals); + this->Geometry.setValues(std::move(newVals)); } solverNeedsUpdate=true; @@ -1019,26 +1063,27 @@ int SketchObject::addConstraints(const std::vector &ConstraintList { const std::vector< Constraint * > &vals = this->Constraints.getValues(); - std::vector< Constraint * > newVals(vals); - newVals.insert(newVals.end(), ConstraintList.begin(), ConstraintList.end()); + std::vector< Constraint * > newVals; - //test if tangent constraints have been added; AutoLockTangency. - std::vector< Constraint * > tbd;//list of temporary copies that need to be deleted - for(std::size_t i = newVals.size()-ConstraintList.size(); iType == Tangent || newVals[i]->Type == Perpendicular ){ - Constraint *constNew = newVals[i]->clone(); - AutoLockTangencyAndPerpty(constNew); - tbd.push_back(constNew); - newVals[i] = constNew; + newVals.reserve(vals.size() + ConstraintList.size()); + + // deep copy originals + for(auto &v : vals) { + newVals.push_back(v->clone()); + } + + // deep copy new ones + for(auto &v : ConstraintList) { + + auto & cnew = v; + newVals.push_back(v->clone()); + + if( cnew->Type == Tangent || cnew->Type == Perpendicular ){ + AutoLockTangencyAndPerpty(cnew); } } - this->Constraints.setValues(newVals); - - //clean up - delete temporary copies of constraints that were made to affect the constraints - for(std::size_t i=0; iConstraints.setValues(std::move(newVals)); return this->Constraints.getSize()-1; } @@ -1049,17 +1094,24 @@ int SketchObject::addCopyOfConstraints(const SketchObject &orig) const std::vector< Constraint * > &origvals = orig.Constraints.getValues(); - std::vector< Constraint * > newVals(vals); + std::vector< Constraint * > newVals; - for(std::size_t j = 0; jcopy()); + newVals.reserve(vals.size()+origvals.size()); - std::size_t valssize = vals.size(); + for( auto & v : vals ) + newVals.push_back(v->clone()); - this->Constraints.setValues(newVals); + for( auto & v : origvals ) + newVals.push_back(v->copy()); - for(std::size_t i = valssize, j = 0; iisDriving && newVals[i]->isDimensional()) { + this->Constraints.setValues(std::move(newVals)); + + auto & uvals = this->Constraints.getValues(); + + std::size_t uvalssize = uvals.size(); + + for(std::size_t i = uvalssize, j = 0; iisDriving && uvals[i]->isDimensional()) { App::ObjectIdentifier spath = orig.Constraints.createPath(j); @@ -1069,7 +1121,6 @@ int SketchObject::addCopyOfConstraints(const SketchObject &orig) App::ObjectIdentifier dpath = this->Constraints.createPath(i); setExpression(dpath, boost::shared_ptr(expr_info.expression->copy())); } - } } @@ -1080,15 +1131,24 @@ int SketchObject::addConstraint(const Constraint *constraint) { const std::vector< Constraint * > &vals = this->Constraints.getValues(); - std::vector< Constraint * > newVals(vals); + std::vector< Constraint * > newVals; + + newVals.reserve(vals.size()+1); + + // deep copy originals + for(auto &v : vals) { + newVals.push_back(v->clone()); + } + Constraint *constNew = constraint->clone(); if (constNew->Type == Tangent || constNew->Type == Perpendicular) AutoLockTangencyAndPerpty(constNew); - newVals.push_back(constNew); - this->Constraints.setValues(newVals); - delete constNew; + newVals.push_back(constNew); // add new constraint at the back + + this->Constraints.setValues(std::move(newVals)); + return this->Constraints.getSize()-1; } @@ -3050,10 +3110,19 @@ bool SketchObject::isCarbonCopyAllowed(App::Document *pDoc, App::DocumentObject int SketchObject::addSymmetric(const std::vector &geoIdList, int refGeoId, Sketcher::PointPos refPosId/*=Sketcher::none*/) { const std::vector< Part::Geometry * > &geovals = getInternalGeometry(); - std::vector< Part::Geometry * > newgeoVals(geovals); - const std::vector< Constraint * > &constrvals = this->Constraints.getValues(); - std::vector< Constraint * > newconstrVals(constrvals); + + std::vector< Part::Geometry * > newgeoVals; + std::vector< Constraint * > newconstrVals; + + newgeoVals.reserve(geovals.size()+geoIdList.size()); + newconstrVals.reserve(constrvals.size()); // At least these, probably more. + + for( auto & v : geovals ) + newgeoVals.push_back(v->clone()); + + for( auto & v : constrvals ) + newconstrVals.push_back(v->clone()); int cgeoid = getHighestCurveIndex()+1; @@ -3477,7 +3546,7 @@ int SketchObject::addSymmetric(const std::vector &geoIdList, int refGeoId, // Block acceptGeometry in OnChanged to avoid unnecessary checks and updates { Base::StateLocker lock(internaltransaction, true); - Geometry.setValues(newgeoVals); + Geometry.setValues(std::move(newgeoVals)); for (std::vector::const_iterator it = constrvals.begin(); it != constrvals.end(); ++it) { @@ -3574,7 +3643,7 @@ int SketchObject::addSymmetric(const std::vector &geoIdList, int refGeoId, } if( newconstrVals.size() > constrvals.size() ) - Constraints.setValues(newconstrVals); + Constraints.setValues(std::move(newconstrVals)); } @@ -3585,14 +3654,32 @@ int SketchObject::addSymmetric(const std::vector &geoIdList, int refGeoId, return Geometry.getSize()-1; } -int SketchObject::addCopy(const std::vector &geoIdList, const Base::Vector3d& displacement, bool moveonly /*=false*/, bool clone /*=false*/, int csize/*=2*/, int rsize/*=1*/, - bool constraindisplacement /*= false*/, double perpscale /*= 1.0*/) +int SketchObject::addCopy(const std::vector &geoIdList, const Base::Vector3d& displacement, bool moveonly /*=false*/, + bool clone /*=false*/, int csize/*=2*/, int rsize/*=1*/, bool constraindisplacement /*= false*/, + double perpscale /*= 1.0*/) { const std::vector< Part::Geometry * > &geovals = getInternalGeometry(); - std::vector< Part::Geometry * > newgeoVals(geovals); const std::vector< Constraint * > &constrvals = this->Constraints.getValues(); - std::vector< Constraint * > newconstrVals(constrvals); + + std::vector< Part::Geometry * > newgeoVals; + + std::vector< Constraint * > newconstrVals; + + if(moveonly) { + newgeoVals.reserve(geovals.size()); + newconstrVals.reserve(constrvals.size()); + } + else { + newgeoVals.reserve(geovals.size()+geoIdList.size()); + newconstrVals.reserve(constrvals.size()); // At least the same (moving), probably more (copying) but not easy to estimate. + } + + for(auto & v : geovals) + newgeoVals.push_back(v->clone()); + + for(auto & v : constrvals) + newconstrVals.push_back(v->clone()); std::vector newgeoIdList(geoIdList); @@ -3652,7 +3739,14 @@ int SketchObject::addCopy(const std::vector &geoIdList, const Base::Vector3 int index = 0; for (std::vector::const_iterator it = newgeoIdList.begin(); it != newgeoIdList.end(); ++it, index++) { const Part::Geometry *geo = getGeometry(*it); - Part::Geometry *geocopy = geo->copy(); // make a copy of the pointer for undo even if moving + + Part::Geometry *geocopy; + + // We have already cloned all geometry and constraints, we only need a copy if not moving + if(!moveonly) + geocopy = geo->copy(); // make a copy of the pointer for undo even if moving + else + geocopy = newgeoVals[*it]; // Handle Geometry if(geocopy->getTypeId() == Part::GeomLineSegment::getClassTypeId()){ @@ -3755,13 +3849,13 @@ int SketchObject::addCopy(const std::vector &geoIdList, const Base::Vector3 continue; } - if(!moveonly) { + if(!moveonly) { // we are copying newgeoVals.push_back(geocopy); geoIdMap.insert(std::make_pair(*it, cgeoid)); cgeoid++; } - else { - newgeoVals[index] = geocopy; + else { // We are moving + newgeoVals[*it] = geocopy; } } @@ -3998,10 +4092,10 @@ int SketchObject::addCopy(const std::vector &geoIdList, const Base::Vector3 // Block acceptGeometry in OnChanged to avoid unnecessary checks and updates { Base::StateLocker lock(internaltransaction, true); - Geometry.setValues(newgeoVals); + Geometry.setValues(std::move(newgeoVals)); if( newconstrVals.size() > constrvals.size() ) - Constraints.setValues(newconstrVals); + Constraints.setValues(std::move(newconstrVals)); } // we inhibited update, so we trigger it now @@ -5071,6 +5165,7 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m } } catch (const Base::Exception& e) { + delete bspline; Base::Console().Error("%s\n", e.what()); return false; } @@ -5147,11 +5242,11 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m } } else { // it is a bspline geometry, but not a controlpoint or knot - newcVals.push_back(*it); + newcVals.push_back((*it)->clone()); } } else { - newcVals.push_back(*it); + newcVals.push_back((*it)->clone()); } } @@ -5159,14 +5254,23 @@ bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int m std::vector< Part::Geometry * > newVals(vals); - newVals[GeoId] = bspline; + // deep copy + for(size_t i=0; iclone(); + } + } // Block acceptGeometry in OnChanged to avoid unnecessary checks and updates { Base::StateLocker lock(internaltransaction, true); - Geometry.setValues(newVals); + Geometry.setValues(std::move(newVals)); - this->Constraints.setValues(newcVals); + this->Constraints.setValues(std::move(newcVals)); } // Trigger update now // Update geometry indices and rebuild vertexindex now via onChanged, so that ViewProvider::UpdateData is triggered. @@ -5214,9 +5318,17 @@ int SketchObject::carbonCopy(App::DocumentObject * pObj, bool construction) const std::vector< Sketcher::Constraint * > &cvals = Constraints.getValues(); - std::vector< Part::Geometry * > newVals(vals); + const std::vector< Part::Geometry * > &svals = psObj->getInternalGeometry(); - std::vector< Constraint * > newcVals(cvals); + const std::vector< Sketcher::Constraint * > &scvals = psObj->Constraints.getValues(); + + std::vector< Part::Geometry * > newVals; + + std::vector< Constraint * > newcVals; + + newVals.reserve(vals.size()+svals.size()); + + newcVals.reserve(cvals.size()+scvals.size()); int nextgeoid = vals.size(); @@ -5224,10 +5336,6 @@ int SketchObject::carbonCopy(App::DocumentObject * pObj, bool construction) int nextcid = cvals.size(); - const std::vector< Part::Geometry * > &svals = psObj->getInternalGeometry(); - - const std::vector< Sketcher::Constraint * > &scvals = psObj->Constraints.getValues(); - if(psObj->ExternalGeometry.getSize()>0) { std::vector Objects = ExternalGeometry.getValues(); std::vector SubElements = ExternalGeometry.getSubValues(); @@ -5277,6 +5385,12 @@ int SketchObject::carbonCopy(App::DocumentObject * pObj, bool construction) solverNeedsUpdate=true; } + for( auto &v : vals) + newVals.push_back(v->clone()); + + for( auto &v : cvals) + newcVals.push_back(v->clone()); + for (std::vector::const_iterator it=svals.begin(); it != svals.end(); ++it){ Part::Geometry *geoNew = (*it)->copy(); if(construction && geoNew->getTypeId() != Part::GeomPoint::getClassTypeId()) { @@ -5307,15 +5421,15 @@ int SketchObject::carbonCopy(App::DocumentObject * pObj, bool construction) // Block acceptGeometry in OnChanged to avoid unnecessary checks and updates { Base::StateLocker lock(internaltransaction, true); - Geometry.setValues(newVals); - this->Constraints.setValues(newcVals); + Geometry.setValues(std::move(newVals)); + this->Constraints.setValues(std::move(newcVals)); } // we trigger now the update (before dealing with expressions) // Update geometry indices and rebuild vertexindex now via onChanged, so that ViewProvider::UpdateData is triggered. Geometry.touch(); int sourceid = 0; - for (std::vector< Sketcher::Constraint * >::const_iterator it= scvals.begin(); it != scvals.end(); ++it,nextcid++,sourceid++) { + for (std::vector< Sketcher::Constraint * >::const_iterator it= scvals.begin(); it != scvals.end(); ++it, nextcid++, sourceid++) { if ((*it)->Type == Sketcher::Distance || (*it)->Type == Sketcher::Radius || @@ -5486,9 +5600,7 @@ int SketchObject::delAllExternal() } solverNeedsUpdate=true; - Constraints.setValues(newConstraints); - for (Constraint* it : newConstraints) - delete it; + Constraints.setValues(std::move(newConstraints)); acceptGeometry(); // This may need to be refactored into OnChanged for ExternalGeometry return 0; } @@ -5671,9 +5783,7 @@ void SketchObject::validateExternalLinks(void) } } - Constraints.setValues(newConstraints); - for (Constraint* it : newConstraints) - delete it; + Constraints.setValues(std::move(newConstraints)); i--; // we deleted an item, so the next one took its place } } @@ -6972,28 +7082,25 @@ int SketchObject::changeConstraintsLocking(bool bLock) std::vector< Constraint * > newVals(vals);//modifiable copy of pointers array - std::vector< Constraint * > tbd;//list of temporary Constraint copies that need to be deleted later + // deep copy + for(size_t i=0; iclone(); - for(std::size_t i = 0; iType == Tangent || newVals[i]->Type == Perpendicular ){ //create a constraint copy, affect it, replace the pointer cntToBeAffected++; - Constraint *constNew = newVals[i]->clone(); - bool ret = AutoLockTangencyAndPerpty(constNew, /*bForce=*/true, bLock); - if (ret) cntSuccess++; - tbd.push_back(constNew); - newVals[i] = constNew; + + bool ret = AutoLockTangencyAndPerpty(newVals[i], /*bForce=*/true, bLock); + + if (ret) + cntSuccess++; + Base::Console().Log("Constraint%i will be affected\n", i+1); } } - this->Constraints.setValues(newVals); - - //clean up - delete temporary copies of constraints that were made to affect the constraints - for(std::size_t i=0; iConstraints.setValues(std::move(newVals)); Base::Console().Log("ChangeConstraintsLocking: affected %i of %i tangent/perp constraints\n", cntSuccess, cntToBeAffected);