From 56e32b9c98f932a70ec156d8ddb9d3fc25331714 Mon Sep 17 00:00:00 2001 From: "Zheng, Lei" Date: Wed, 1 May 2024 18:02:04 -0400 Subject: [PATCH 1/3] Methods to support Toponaming element maps --- src/App/ComplexGeoData.cpp | 5 + src/App/ComplexGeoData.h | 3 + src/App/GeoFeature.cpp | 96 +++++++++- src/App/GeoFeature.h | 37 ++++ src/App/PropertyLinks.cpp | 354 ++++++++++++++++++++++++++++++++++--- src/App/PropertyLinks.h | 1 + 6 files changed, 473 insertions(+), 23 deletions(-) diff --git a/src/App/ComplexGeoData.cpp b/src/App/ComplexGeoData.cpp index cf99b2a22c..b2653b58e3 100644 --- a/src/App/ComplexGeoData.cpp +++ b/src/App/ComplexGeoData.cpp @@ -637,6 +637,11 @@ unsigned int ComplexGeoData::getMemSize() const return 0; } +std::vector ComplexGeoData::getHigherElements(const char *, bool) const +{ + return {}; +} + void ComplexGeoData::setMappedChildElements(const std::vector & children) { // DO NOT reset element map if there is one. Because we allow mixing child diff --git a/src/App/ComplexGeoData.h b/src/App/ComplexGeoData.h index 5b0ec19c83..504d3dbb93 100644 --- a/src/App/ComplexGeoData.h +++ b/src/App/ComplexGeoData.h @@ -321,6 +321,9 @@ public: /// Get the current element map size size_t getElementMapSize(bool flush=true) const; + /// Return the higher level element names of the given element + virtual std::vector getHigherElements(const char *name, bool silent=false) const; + /// Return the current element map version virtual std::string getElementMapVersion() const; diff --git a/src/App/GeoFeature.cpp b/src/App/GeoFeature.cpp index afd027ef45..fcb13566ef 100644 --- a/src/App/GeoFeature.cpp +++ b/src/App/GeoFeature.cpp @@ -26,6 +26,7 @@ #include #include "ComplexGeoData.h" +#include "Document.h" #include "GeoFeature.h" #include "GeoFeatureGroupExtension.h" #include "ElementNamingUtils.h" @@ -146,12 +147,27 @@ DocumentObject *GeoFeature::resolveElement(DocumentObject *obj, const char *subn subname = ""; const char *element = Data::findElementName(subname); if(_element) *_element = element; +#ifdef FC_USE_TNP_FIX + elementName.first.clear(); + elementName.second.clear(); + auto sobj = obj->getSubObject(std::string(subname, element).c_str()); + if(!sobj) + return nullptr; + auto linked = sobj->getLinkedObject(true); + auto geo = Base::freecad_dynamic_cast(linked); +// if(!geo && linked) { +// auto ext = linked->getExtensionByType(true); +// if(ext) +// geo = Base::freecad_dynamic_cast(ext->getTrueLinkedObject(true)); +// } +#else auto sobj = obj->getSubObject(subname); if(!sobj) return nullptr; obj = sobj->getLinkedObject(true); auto geo = dynamic_cast(obj); - if(geoFeature) +#endif + if(geoFeature) *geoFeature = geo; if(!obj || (filter && obj!=filter)) return nullptr; @@ -189,3 +205,81 @@ void GeoFeature::setMaterialAppearance(const App::Material& material) { Q_UNUSED(material) } + +#ifdef FC_USE_TNP_FIX +bool GeoFeature::hasMissingElement(const char *subname) { + return Data::hasMissingElement(subname); + if(!subname) + return false; + auto dot = strrchr(subname,'.'); + if(!dot) + return subname[0]=='?'; + return dot[1]=='?'; +} + +void GeoFeature::updateElementReference() { + auto prop = getPropertyOfGeometry(); + if(!prop) return; + auto geo = prop->getComplexData(); + if(!geo) return; + bool reset = false; +// auto version = getElementMapVersion(prop); +// if(_ElementMapVersion.getStrValue().empty()) +// _ElementMapVersion.setValue(version); +// else if(_ElementMapVersion.getStrValue()!=version) { +// reset = true; +// _ElementMapVersion.setValue(version); +// } + PropertyLinkBase::updateElementReferences(this,reset); +} + +void GeoFeature::onChanged(const Property *prop) { + if(prop==getPropertyOfGeometry()) { + if(getDocument() && !getDocument()->testStatus(Document::Restoring) + && !getDocument()->isPerformingTransaction()) + { + updateElementReference(); + } + } + DocumentObject::onChanged(prop); +} + +//void GeoFeature::onDocumentRestored() { +// if(!getDocument()->testStatus(Document::Status::Importing)) +// _ElementMapVersion.setValue(getElementMapVersion(getPropertyOfGeometry(),true)); +// DocumentObject::onDocumentRestored(); +//} + +//const std::vector& +//GeoFeature::searchElementCache(const std::string &element, +// Data::SearchOptions options, +// double tol, +// double atol) const +//{ +// static std::vector none; +// (void)element; +// (void)options; +// (void)tol; +// (void)atol; +// return none; +//} + +//const std::vector& +//GeoFeature::getElementTypes(bool /*all*/) const +//{ +// static std::vector nil; +// auto prop = getPropertyOfGeometry(); +// if (!prop) +// return nil; +// return prop->getComplexData()->getElementTypes(); +//} + +std::vector +GeoFeature::getHigherElements(const char *element, bool silent) const +{ + auto prop = getPropertyOfGeometry(); + if (!prop) + return {}; + return prop->getComplexData()->getHigherElements(element, silent); +} +#endif \ No newline at end of file diff --git a/src/App/GeoFeature.h b/src/App/GeoFeature.h index fb3d244b4e..038daeb2de 100644 --- a/src/App/GeoFeature.h +++ b/src/App/GeoFeature.h @@ -140,7 +140,44 @@ public: * appearance from an App::Material object. */ virtual void setMaterialAppearance(const App::Material& material); +#ifdef FC_USE_TNP_FIX + static bool hasMissingElement(const char *subname); + /** Search sub element using internal cached geometry + * + * @param element: element name + * @param options: search options + * @param tol: coordinate tolerance + * @param atol: angle tolerance + * + * @return Returns a list of found element reference to the new goemetry. + * The returned value will be invalidated when the geometry is changed. + * + * Before changing the property of geometry, GeoFeature will internally + * make a snapshot of all referenced element geometry. After change, user + * code may call this function to search for the new element name that + * reference to the same geometry of the old element. + */ +// virtual const std::vector& searchElementCache(const std::string &element, +// Data::SearchOptions options = Data::SearchOption::CheckGeometry, +// double tol = 1e-7, +// double atol = 1e-10) const; + + + /// Return the object that owns the shape that contains the give element name + virtual DocumentObject *getElementOwner(const Data::MappedName & /*name*/) const + {return nullptr;} + +// virtual const std::vector& getElementTypes(bool all=true) const; + + /// Return the higher level element names of the given element + virtual std::vector getHigherElements(const char *name, bool silent=false) const; + +protected: + void onChanged(const Property* prop) override; +// void onDocumentRestored() override; + void updateElementReference(); +#endif protected: std::pair _getElementName(const char* name, const Data::MappedElement& mapped) const; diff --git a/src/App/PropertyLinks.cpp b/src/App/PropertyLinks.cpp index 1f42f10985..61a28ecf86 100644 --- a/src/App/PropertyLinks.cpp +++ b/src/App/PropertyLinks.cpp @@ -38,6 +38,8 @@ #include "DocumentObject.h" #include "DocumentObjectPy.h" #include "ObjectIdentifier.h" +#include "ElementNamingUtils.h" +#include "GeoFeature.h" FC_LOG_LEVEL_INIT("PropertyLinks",true,true) @@ -55,6 +57,9 @@ namespace sp = std::placeholders; TYPESYSTEM_SOURCE_ABSTRACT(App::PropertyLinkBase , App::Property) static std::unordered_map > _LabelMap; + +static std::unordered_map > _ElementRefMap; + PropertyLinkBase::PropertyLinkBase() = default; PropertyLinkBase::~PropertyLinkBase() { @@ -97,6 +102,17 @@ bool PropertyLinkBase::isSame(const Property &other) const } void PropertyLinkBase::unregisterElementReference() { +#ifdef FC_USE_TNP_FIX + for(auto obj : _ElementRefs) { + auto it = _ElementRefMap.find(obj); + if(it != _ElementRefMap.end()) { + it->second.erase(this); + if(it->second.empty()) + _ElementRefMap.erase(it); + } + } + _ElementRefs.clear(); +#endif } void PropertyLinkBase::unregisterLabelReferences() @@ -202,15 +218,58 @@ static std::string propertyName(const Property *prop) { } void PropertyLinkBase::updateElementReferences(DocumentObject *feature, bool reverse) { +#ifdef FC_USE_TNP_FIX + if(!feature || !feature->getNameInDocument()) + return; + auto it = _ElementRefMap.find(feature); + if(it == _ElementRefMap.end()) + return; + std::vector props; + props.reserve(it->second.size()); + props.insert(props.end(),it->second.begin(),it->second.end()); + for(auto prop : props) { + if(prop->getContainer()) { + try { + prop->updateElementReference(feature,reverse,true); + }catch(Base::Exception &e) { + e.ReportException(); + FC_ERR("Failed to update element reference of " << propertyName(prop)); + }catch(std::exception &e) { + FC_ERR("Failed to update element reference of " << propertyName(prop) + << ": " << e.what()); + } + } + } +#else (void)feature; (void)reverse; +#endif } void PropertyLinkBase::_registerElementReference(App::DocumentObject *obj, std::string &sub, ShadowSub &shadow) { +#ifdef FC_USE_TNP_FIX + if(!obj || !obj->getNameInDocument() || sub.empty()) + return; + if(shadow.first.empty()) { + _updateElementReference(0,obj,sub,shadow,false); + return; + } + GeoFeature *geo = 0; + const char *element = 0; + std::pair elementName; + GeoFeature::resolveElement(obj,sub.c_str(), elementName,true, + GeoFeature::ElementNameType::Export,0,&element,&geo); + if(!geo || !element || !element[0]) + return; + + if(_ElementRefs.insert(geo).second) + _ElementRefMap[geo].insert(this); +#else (void)obj; (void)sub; (void)shadow; +#endif } class StringGuard { @@ -275,12 +334,171 @@ bool PropertyLinkBase::_updateElementReference(DocumentObject *feature, App::DocumentObject *obj, std::string &sub, ShadowSub &shadow, bool reverse, bool notify) { +#ifdef FC_USE_TNP_FIX + if (!obj || !obj->getNameInDocument()) return false; + ShadowSub elementName; + const char* subname; + if (shadow.first.size()) + subname = shadow.first.c_str(); + else if (shadow.second.size()) + subname = shadow.second.c_str(); + else + subname = sub.c_str(); + GeoFeature* geo = 0; + const char* element = 0; + auto ret = GeoFeature::resolveElement(obj, + subname, + elementName, + true, + GeoFeature::ElementNameType::Export, + feature, + &element, + &geo); + if (!ret || !geo || !element || !element[0]) { + if (elementName.second.size()) + shadow.second.swap(elementName.second); + return false; + } + + if (_ElementRefs.insert(geo).second) + _ElementRefMap[geo].insert(this); + + if (!reverse) { + if (elementName.first.empty()) { + shadow.second.swap(elementName.second); + return false; + } + if (shadow == elementName) + return false; + } + + bool missing = Data::hasMissingElement(elementName.second.c_str()); + if (feature == geo && (missing || reverse)) { + // If the referenced element is missing, or we are generating element + // map for the first time, or we are re-generating the element map due + // to version change, i.e. 'reverse', try search by geometry first + const char* oldElement = Data::findElementName(shadow.second.c_str()); + if (!Data::hasMissingElement(oldElement)) { +// const auto& names = geo->searchElementCache(oldElement); + std::vector names; // searchElementCache isn't implemented. + if (names.size()) { + missing = false; + std::string newsub(subname, strlen(subname) - strlen(element)); + newsub += names.front(); + GeoFeature::resolveElement(obj, + newsub.c_str(), + elementName, + true, + GeoFeature::ElementNameType::Export, + feature); + const auto& oldName = shadow.first.size() ? shadow.first : shadow.second; + const auto& newName = + elementName.first.size() ? elementName.first : elementName.second; + if (oldName != newName) { + FC_WARN(propertyName(this) + << " auto change element reference " << ret->getFullName() << " " + << oldName << " -> " << newName); + } + } + // Note: the following code proves to be too risky. There is no way + // (so far) to ensure the recompute do not change the geometry. If + // the geometry does remain the same, the above geometry search + // should be able to find the new reference any way! +#if 0 + else if (missing && reverse && shadow.first.size()) { + // reverse means we are trying to either generate the element + // name for the first time, or upgrade to a new map version. In + // case of upgrading, we still consult the original mapped name + // in first try. Here means the first try failed, and the + // geometry search cannot find any match, so we try the + // non-mapped name as a last resort. + // + // WARNING! We are assuming the recomputation is done with no + // actual property change, and the resulting geometry remains + // the same. If this condition is not met, the result may be + // undesirable. TODO: find a way to ensure this condition. + + GeoFeature::resolveElement(obj, shadow.second.c_str(), elementName, true, + GeoFeature::ElementNameType::Export,feature); + if(!elementName.second.empty()) { + missing = Data::ComplexGeoData::hasMissingElement(elementName.second.c_str()); + if (!missing) { + FC_WARN(propertyName(this) + << " element reference changed " << ret->getFullName() << " " + << shadow.first << " -> " << elementName.first); + } + } + } +#endif + } + } + + if (notify) + aboutToSetValue(); + + auto updateSub = [&](const std::string& newSub) { + if (sub != newSub) { + //signalUpdateElementReference(sub, newSub); + sub = newSub; + } + }; + + if (missing) { + FC_WARN(propertyName(this) + << " missing element reference " << ret->getFullName() << " " + << (elementName.first.size() ? elementName.first : elementName.second)); + shadow.second.swap(elementName.second); + } + else { + FC_TRACE(propertyName(this) << " element reference shadow update " << ret->getFullName() + << " " << shadow.first << " -> " << elementName.first); + shadow.swap(elementName); + if (shadow.first.size() && Data::hasMappedElementName(sub.c_str())) + updateSub(shadow.first); + } + + if (reverse) { + if (shadow.first.size() && Data::hasMappedElementName(sub.c_str())) + updateSub(shadow.first); + else + updateSub(shadow.second); + return true; + } + if (missing) { + if (sub != shadow.first) + updateSub(shadow.second); + return true; + } + auto pos2 = shadow.first.rfind('.'); + if (pos2 == std::string::npos) + return true; + ++pos2; + auto pos = sub.rfind('.'); + if (pos == std::string::npos) + pos = 0; + else + ++pos; + if (pos == pos2) { + if (sub.compare(pos, sub.size() - pos, &shadow.first[pos2]) != 0) { + FC_LOG("element reference update " << sub << " -> " << shadow.first); + std::string newSub(sub); + newSub.replace(pos, sub.size() - pos, &shadow.first[pos2]); + updateSub(newSub); + } + } + else if (sub != shadow.second) { + FC_LOG("element reference update " << sub << " -> " << shadow.second); + updateSub(shadow.second); + } + return true; +#else (void)feature; (void)obj; (void)reverse; (void)notify; shadow.second = sub; return false; +#endif } std::pair @@ -297,6 +515,13 @@ PropertyLinkBase::tryReplaceLink(const PropertyContainer *owner, DocumentObject return res; } return res; +#ifdef FC_USE_TNP_FIX + } else if (newObj == obj) { + // This means the new object is already sub-object of this parent + // (consider a case of swapping the tool and base object of the Cut + // feature). We'll swap the old and new object. + return tryReplaceLink(owner, obj, parent, newObj, oldObj, subname); +#endif } if(!subname || !subname[0]) return res; @@ -323,6 +548,10 @@ PropertyLinkBase::tryReplaceLink(const PropertyContainer *owner, DocumentObject return res; } break; +#ifdef FC_USE_TNP_FIX + }else if(sobj == newObj) { + return tryReplaceLink(owner, obj, parent, newObj, oldObj, subname); +#endif }else if(prev == parent) break; prev = sobj; @@ -910,18 +1139,19 @@ void PropertyLinkSub::setValue(App::DocumentObject * lValue, std::vector &&subs, std::vector &&shadows) { auto parent = Base::freecad_dynamic_cast(getContainer()); - if(lValue) { - if(!lValue->isAttachedToDocument()) + if (lValue) { + if (!lValue->isAttachedToDocument()) throw Base::ValueError("PropertyLinkSub: invalid document object"); - if(!testFlag(LinkAllowExternal) && parent && parent->getDocument()!=lValue->getDocument()) + if (!testFlag(LinkAllowExternal) && parent + && parent->getDocument() != lValue->getDocument()) throw Base::ValueError("PropertyLinkSub does not support external object"); } aboutToSetValue(); #ifndef USE_OLD_DAG - if(parent) { + if (parent) { // before accessing internals make sure the object is not about to be destroyed // otherwise the backlink contains dangling pointers - if (!parent->testStatus(ObjectStatus::Destroy) && _pcScope!=LinkScope::Hidden) { + if (!parent->testStatus(ObjectStatus::Destroy) && _pcScope != LinkScope::Hidden) { if (_pcLinkSub) _pcLinkSub->_removeBackLink(parent); if (lValue) @@ -929,11 +1159,12 @@ void PropertyLinkSub::setValue(App::DocumentObject * lValue, } } #endif - _pcLinkSub=lValue; - _cSubList=std::move(subs); - if(shadows.size()==_cSubList.size()) + _pcLinkSub = lValue; + _cSubList = std::move(subs); + if (shadows.size() == _cSubList.size()) { _ShadowSubList = std::move(shadows); - else + onContainerRestored(); // re-register element references + } else updateElementReference(nullptr); checkLabelReferences(_cSubList); hasSetValue(); @@ -950,13 +1181,24 @@ const std::vector& PropertyLinkSub::getSubValues() const } static inline const std::string &getSubNameWithStyle(const std::string &subName, - const PropertyLinkBase::ShadowSub &shadow, bool newStyle) + const PropertyLinkBase::ShadowSub &shadow, bool newStyle, std::string &tmp) { if(!newStyle) { if(!shadow.second.empty()) return shadow.second; - }else if(!shadow.first.empty()) + }else if(!shadow.first.empty()) { +#ifdef FC_USE_TNP_FIX + if (Data::hasMissingElement(shadow.second.c_str())) { + auto pos = shadow.first.rfind('.'); + if (pos != std::string::npos) { + tmp = shadow.first.substr(0, pos+1); + tmp += shadow.second; + return tmp; + } + } +#endif return shadow.first; + } return subName; } @@ -964,13 +1206,27 @@ std::vector PropertyLinkSub::getSubValues(bool newStyle) const { assert(_cSubList.size() == _ShadowSubList.size()); std::vector ret; ret.reserve(_cSubList.size()); + std::string tmp; for(size_t i=0;i<_ShadowSubList.size();++i) - ret.push_back(getSubNameWithStyle(_cSubList[i],_ShadowSubList[i],newStyle)); + ret.push_back(getSubNameWithStyle(_cSubList[i],_ShadowSubList[i],newStyle,tmp)); return ret; } std::vector PropertyLinkSub::getSubValuesStartsWith(const char* starter, bool newStyle) const { +#ifdef FC_USE_TNP_FIX + assert(_cSubList.size() == _ShadowSubList.size()); + std::vector ret; + std::string tmp; + for(size_t i=0;i<_ShadowSubList.size();++i) { + const auto &sub = getSubNameWithStyle(_cSubList[i],_ShadowSubList[i],newStyle,tmp); + auto element = Data::findElementName(sub.c_str()); + if(element && boost::starts_with(element,starter)) + ret.emplace_back(element); + } + return ret; + +#else (void)newStyle; std::vector temp; @@ -980,6 +1236,7 @@ std::vector PropertyLinkSub::getSubValuesStartsWith(const char* sta } } return temp; +#endif } App::DocumentObject * PropertyLinkSub::getValue(Base::Type t) const @@ -993,8 +1250,14 @@ PyObject *PropertyLinkSub::getPyObject() Py::List list(static_cast(_cSubList.size())); if (_pcLinkSub) { tup[0] = Py::asObject(_pcLinkSub->getPyObject()); + int i = 0; +#ifdef FC_USE_TNP_FIX + for (auto &sub : getSubValues(true)) + list[i++] = Py::String(sub); +#else for(unsigned int i = 0;i<_cSubList.size(); i++) list[i] = Py::String(_cSubList[i]); +#endif tup[1] = list; return Py::new_reference_to(tup); } @@ -1460,6 +1723,9 @@ Property *PropertyLinkSub::Copy() const PropertyLinkSub *p= new PropertyLinkSub(); p->_pcLinkSub = _pcLinkSub; p->_cSubList = _cSubList; +#ifdef FC_USE_TNP_FIX + p->_ShadowSubList = _ShadowSubList; +#endif return p; } @@ -1468,7 +1734,12 @@ void PropertyLinkSub::Paste(const Property &from) if(!from.isDerivedFrom(PropertyLinkSub::getClassTypeId())) throw Base::TypeError("Incompatible property to paste to"); auto &link = static_cast(from); +#ifdef FC_USE_TNP_FIX + setValue(link._pcLinkSub, link._cSubList, + std::vector(link._ShadowSubList)); +#else setValue(link._pcLinkSub, link._cSubList); +#endif } void PropertyLinkSub::getLinks(std::vector &objs, @@ -1727,9 +1998,10 @@ void PropertyLinkSubList::setValues(std::vector&& lValue, aboutToSetValue(); _lValueList = std::move(lValue); _lSubList = std::move(lSubNames); - if(ShadowSubList.size()==_lSubList.size()) + if(ShadowSubList.size()==_lSubList.size()) { _ShadowSubList = std::move(ShadowSubList); - else + onContainerRestored(); // re-register element references + } else updateElementReference(nullptr); checkLabelReferences(_lSubList); hasSetValue(); @@ -1914,14 +2186,26 @@ void PropertyLinkSubList::setSubListValues(const std::vector links; std::vector subs; - +#ifdef FC_USE_TNP_FIX + for (std::vector::const_iterator it = values.begin(); it != values.end(); ++it) { + if (it->second.empty()) { + links.push_back(it->first); + subs.emplace_back(); + continue; + } + for (std::vector::const_iterator jt = it->second.begin(); jt != it->second.end(); ++jt) { + links.push_back(it->first); + subs.push_back(*jt); + } + } +#else for (const auto & value : values) { for (const auto& jt : value.second) { links.push_back(value.first); subs.push_back(jt); } } - +#endif setValues(links, subs); } @@ -2386,6 +2670,9 @@ Property *PropertyLinkSubList::Copy() const PropertyLinkSubList *p = new PropertyLinkSubList(); p->_lValueList = _lValueList; p->_lSubList = _lSubList; +#ifdef FC_USE_TNP_FIX + p->_ShadowSubList = _ShadowSubList; +#endif return p; } @@ -2394,7 +2681,12 @@ void PropertyLinkSubList::Paste(const Property &from) if(!from.isDerivedFrom(PropertyLinkSubList::getClassTypeId())) throw Base::TypeError("Incompatible property to paste to"); auto &link = static_cast(from); +#ifdef FC_USE_TNP_FIX + setValues(link._lValueList, link._lSubList, + std::vector(link._ShadowSubList)); +#else setValues(link._lValueList, link._lSubList); +#endif } unsigned int PropertyLinkSubList::getMemSize () const @@ -2409,8 +2701,9 @@ std::vector PropertyLinkSubList::getSubValues(bool newStyle) const assert(_lSubList.size() == _ShadowSubList.size()); std::vector ret; ret.reserve(_ShadowSubList.size()); + std::string tmp; for(size_t i=0;i<_ShadowSubList.size();++i) - ret.push_back(getSubNameWithStyle(_lSubList[i],_ShadowSubList[i],newStyle)); + ret.push_back(getSubNameWithStyle(_lSubList[i],_ShadowSubList[i],newStyle,tmp)); return ret; } @@ -2965,9 +3258,12 @@ void PropertyXLink::setSubValues(std::vector &&subs, { _SubList = std::move(subs); _ShadowSubList.clear(); - if(shadows.size() == _SubList.size()) + if(shadows.size() == _SubList.size()) { _ShadowSubList = std::move(shadows); - else +#ifdef FC_USE_TNP_FIX + onContainerRestored(); // re-register element references +#endif + } else updateElementReference(nullptr); checkLabelReferences(_SubList); } @@ -3468,8 +3764,12 @@ void PropertyXLink::copyTo(PropertyXLink &other, } if(subs) other._SubList = std::move(*subs); - else + else { other._SubList = _SubList; +#ifdef FC_USE_TNP_FIX + other._ShadowSubList = _ShadowSubList; +#endif + } other._Flags = _Flags; } @@ -3497,10 +3797,19 @@ void PropertyXLink::Paste(const Property &from) FC_WARN("Object '" << other.docName << '#' << other.objectName << "' not found"); return; } +#ifdef FC_USE_TNP_FIX + setValue(obj,std::vector(other._SubList), + std::vector(other._ShadowSubList)); + } else + setValue(std::string(other.filePath),std::string(other.objectName), + std::vector(other._SubList), + std::vector(other._ShadowSubList)); +#else setValue(obj,std::vector(other._SubList)); } else setValue(std::string(other.filePath),std::string(other.objectName), std::vector(other._SubList)); +#endif setFlag(LinkAllowPartial,other.testFlag(LinkAllowPartial)); } @@ -3652,7 +3961,7 @@ void PropertyXLink::setPyObject(PyObject *value) { const char *PropertyXLink::getSubName(bool newStyle) const { if(_SubList.empty() || _ShadowSubList.empty()) return ""; - return getSubNameWithStyle(_SubList[0],_ShadowSubList[0],newStyle).c_str(); + return getSubNameWithStyle(_SubList[0],_ShadowSubList[0],newStyle,tmpShadow).c_str(); } void PropertyXLink::getLinks(std::vector &objs, @@ -3683,8 +3992,9 @@ std::vector PropertyXLink::getSubValues(bool newStyle) const { assert(_SubList.size() == _ShadowSubList.size()); std::vector ret; ret.reserve(_SubList.size()); + std::string tmp; for(size_t i=0;i<_ShadowSubList.size();++i) - ret.push_back(getSubNameWithStyle(_SubList[i],_ShadowSubList[i],newStyle)); + ret.push_back(getSubNameWithStyle(_SubList[i],_ShadowSubList[i],newStyle,tmp)); return ret; } diff --git a/src/App/PropertyLinks.h b/src/App/PropertyLinks.h index 7fc84bd22e..124865f66c 100644 --- a/src/App/PropertyLinks.h +++ b/src/App/PropertyLinks.h @@ -1169,6 +1169,7 @@ protected: std::vector _ShadowSubList; std::vector _mapped; PropertyLinkBase *parentProp; + mutable std::string tmpShadow; }; From 4a0df0ea6d5f17192c1b8482ffd138be7cb5c555 Mon Sep 17 00:00:00 2001 From: bgbsww Date: Wed, 1 May 2024 18:03:00 -0400 Subject: [PATCH 2/3] Test for changed Sketches avoiding TNP --- src/App/GeoFeature.cpp | 41 -------- src/App/GeoFeature.h | 23 ----- src/App/PropertyLinks.cpp | 30 ------ .../TestTopologicalNamingProblem.py | 94 ++++++++++++++++++- 4 files changed, 93 insertions(+), 95 deletions(-) diff --git a/src/App/GeoFeature.cpp b/src/App/GeoFeature.cpp index fcb13566ef..391928818f 100644 --- a/src/App/GeoFeature.cpp +++ b/src/App/GeoFeature.cpp @@ -155,11 +155,6 @@ DocumentObject *GeoFeature::resolveElement(DocumentObject *obj, const char *subn return nullptr; auto linked = sobj->getLinkedObject(true); auto geo = Base::freecad_dynamic_cast(linked); -// if(!geo && linked) { -// auto ext = linked->getExtensionByType(true); -// if(ext) -// geo = Base::freecad_dynamic_cast(ext->getTrueLinkedObject(true)); -// } #else auto sobj = obj->getSubObject(subname); if(!sobj) @@ -223,13 +218,6 @@ void GeoFeature::updateElementReference() { auto geo = prop->getComplexData(); if(!geo) return; bool reset = false; -// auto version = getElementMapVersion(prop); -// if(_ElementMapVersion.getStrValue().empty()) -// _ElementMapVersion.setValue(version); -// else if(_ElementMapVersion.getStrValue()!=version) { -// reset = true; -// _ElementMapVersion.setValue(version); -// } PropertyLinkBase::updateElementReferences(this,reset); } @@ -244,35 +232,6 @@ void GeoFeature::onChanged(const Property *prop) { DocumentObject::onChanged(prop); } -//void GeoFeature::onDocumentRestored() { -// if(!getDocument()->testStatus(Document::Status::Importing)) -// _ElementMapVersion.setValue(getElementMapVersion(getPropertyOfGeometry(),true)); -// DocumentObject::onDocumentRestored(); -//} - -//const std::vector& -//GeoFeature::searchElementCache(const std::string &element, -// Data::SearchOptions options, -// double tol, -// double atol) const -//{ -// static std::vector none; -// (void)element; -// (void)options; -// (void)tol; -// (void)atol; -// return none; -//} - -//const std::vector& -//GeoFeature::getElementTypes(bool /*all*/) const -//{ -// static std::vector nil; -// auto prop = getPropertyOfGeometry(); -// if (!prop) -// return nil; -// return prop->getComplexData()->getElementTypes(); -//} std::vector GeoFeature::getHigherElements(const char *element, bool silent) const diff --git a/src/App/GeoFeature.h b/src/App/GeoFeature.h index 038daeb2de..3dcdffd6eb 100644 --- a/src/App/GeoFeature.h +++ b/src/App/GeoFeature.h @@ -143,33 +143,10 @@ public: #ifdef FC_USE_TNP_FIX static bool hasMissingElement(const char *subname); - /** Search sub element using internal cached geometry - * - * @param element: element name - * @param options: search options - * @param tol: coordinate tolerance - * @param atol: angle tolerance - * - * @return Returns a list of found element reference to the new goemetry. - * The returned value will be invalidated when the geometry is changed. - * - * Before changing the property of geometry, GeoFeature will internally - * make a snapshot of all referenced element geometry. After change, user - * code may call this function to search for the new element name that - * reference to the same geometry of the old element. - */ -// virtual const std::vector& searchElementCache(const std::string &element, -// Data::SearchOptions options = Data::SearchOption::CheckGeometry, -// double tol = 1e-7, -// double atol = 1e-10) const; - - /// Return the object that owns the shape that contains the give element name virtual DocumentObject *getElementOwner(const Data::MappedName & /*name*/) const {return nullptr;} -// virtual const std::vector& getElementTypes(bool all=true) const; - /// Return the higher level element names of the given element virtual std::vector getHigherElements(const char *name, bool silent=false) const; diff --git a/src/App/PropertyLinks.cpp b/src/App/PropertyLinks.cpp index 61a28ecf86..312dec3497 100644 --- a/src/App/PropertyLinks.cpp +++ b/src/App/PropertyLinks.cpp @@ -400,36 +400,6 @@ bool PropertyLinkBase::_updateElementReference(DocumentObject *feature, << oldName << " -> " << newName); } } - // Note: the following code proves to be too risky. There is no way - // (so far) to ensure the recompute do not change the geometry. If - // the geometry does remain the same, the above geometry search - // should be able to find the new reference any way! -#if 0 - else if (missing && reverse && shadow.first.size()) { - // reverse means we are trying to either generate the element - // name for the first time, or upgrade to a new map version. In - // case of upgrading, we still consult the original mapped name - // in first try. Here means the first try failed, and the - // geometry search cannot find any match, so we try the - // non-mapped name as a last resort. - // - // WARNING! We are assuming the recomputation is done with no - // actual property change, and the resulting geometry remains - // the same. If this condition is not met, the result may be - // undesirable. TODO: find a way to ensure this condition. - - GeoFeature::resolveElement(obj, shadow.second.c_str(), elementName, true, - GeoFeature::ElementNameType::Export,feature); - if(!elementName.second.empty()) { - missing = Data::ComplexGeoData::hasMissingElement(elementName.second.c_str()); - if (!missing) { - FC_WARN(propertyName(this) - << " element reference changed " << ret->getFullName() << " " - << shadow.first << " -> " << elementName.first); - } - } - } -#endif } } diff --git a/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py b/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py index a027aca68b..907e604ee0 100644 --- a/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py +++ b/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py @@ -789,6 +789,97 @@ class TestTopologicalNamingProblem(unittest.TestCase): self.assertEqual(plane.Shape.ElementMapSize, 0) self.assertEqual(pad.Shape.ElementMapSize, 26) + def testChangeSketch(self): + # Arrange + self.Body = self.Doc.addObject('PartDesign::Body', 'Body') + # Make first offset cube Pad + self.PadSketch = self.Doc.addObject('Sketcher::SketchObject', 'Sketch') + self.Body.addObject(self.PadSketch) + TestSketcherApp.CreateRectangleSketch(self.PadSketch, (0, 0), (31.37, 25.2)) + self.Doc.recompute() + self.Pad = self.Doc.addObject("PartDesign::Pad", "Pad") + self.Body.addObject(self.Pad) + self.Pad.Profile = self.PadSketch + self.Pad.Length = 10 + self.Doc.recompute() + + self.Sketch001 = self.Body.newObject('Sketcher::SketchObject','Sketch001') + self.Sketch001.AttachmentSupport = (self.Doc.getObject('Pad'),['Face6',]) + self.Sketch001.MapMode = 'FlatFace' + App.ActiveDocument.recompute() + + self.Sketch001.addExternal("Pad","Edge10") + self.Sketch001.addExternal("Pad","Edge7") + + geoList = [] + geoList.append(Part.Circle(App.Vector(15.093666, 13.036922, 0.000000), + App.Vector(0.000000, 0.000000, 1.000000), 5.000000)) + self.Sketch001.addGeometry(geoList,False) + del geoList + self.Sketch001.addConstraint(Sketcher.Constraint('Radius',0,5.000000)) + self.Sketch001.addConstraint(Sketcher.Constraint('Symmetric',-3,2,-4,1,0,3)) + App.ActiveDocument.recompute() + self.Doc.recompute() + + self.Pad001 = self.Body.newObject('PartDesign::Pad','Pad001') + self.Pad001.Profile = self.Doc.getObject('Sketch001') + self.Pad001.Length = 10 + App.ActiveDocument.recompute() + self.Pad001.ReferenceAxis = (self.Doc.getObject('Sketch001'),['N_Axis']) + self.Sketch001.Visibility = False + App.ActiveDocument.recompute() + + self.Pad001.Length = 10.000000 + self.Pad001.TaperAngle = 0.000000 + self.Pad001.UseCustomVector = 0 + self.Pad001.Direction = (0, 0, 1) + self.Pad001.ReferenceAxis = (self.Doc.getObject('Sketch001'), ['N_Axis']) + self.Pad001.AlongSketchNormal = 1 + self.Pad001.Type = 0 + self.Pad001.UpToFace = None + self.Pad001.Reversed = 0 + self.Pad001.Midplane = 0 + self.Pad001.Offset = 0 + self.Doc.recompute() + self.Doc.getObject('Pad').Visibility = False + + self.Doc.getObject('Sketch001').Visibility = False + + # Modify the original sketch to generate TNP issue + geoList = [] + geoList.append(Part.LineSegment(App.Vector(2.510468, 22.837425, 0.000000), + App.Vector(2.510468, 19.933617, 0.000000))) + geoList.append(Part.LineSegment(App.Vector(2.510468, 19.933617, 0.000000), + App.Vector(4.869811, 19.933617, 0.000000))) + geoList.append(Part.LineSegment(App.Vector(4.869811, 19.933617, 0.000000), + App.Vector(4.869811, 22.837425, 0.000000))) + geoList.append(Part.LineSegment(App.Vector(4.869811, 22.837425, 0.000000), + App.Vector(2.510468, 22.837425, 0.000000))) + self.PadSketch.addGeometry(geoList,False) + del geoList + + constraintList = [] + constraintList.append(Sketcher.Constraint('Coincident', 4, 2, 5, 1)) + constraintList.append(Sketcher.Constraint('Coincident', 5, 2, 6, 1)) + constraintList.append(Sketcher.Constraint('Coincident', 6, 2, 7, 1)) + constraintList.append(Sketcher.Constraint('Coincident', 7, 2, 4, 1)) + constraintList.append(Sketcher.Constraint('Vertical', 4)) + constraintList.append(Sketcher.Constraint('Vertical', 6)) + constraintList.append(Sketcher.Constraint('Horizontal', 5)) + constraintList.append(Sketcher.Constraint('Horizontal', 7)) + self.PadSketch.addConstraint(constraintList) + del constraintList + self.Doc.recompute() + # Assert + if self.Body.Shape.ElementMapVersion == "": # Should be '4' as of Mar 2023. + return + self.assertEqual(self.Body.Shape.BoundBox.XMin,0) + self.assertEqual(self.Body.Shape.BoundBox.YMin,0) + self.assertEqual(self.Body.Shape.BoundBox.ZMin,0) + self.assertEqual(self.Body.Shape.BoundBox.XMax,31.37) + self.assertEqual(self.Body.Shape.BoundBox.YMax,25.2) + self.assertEqual(self.Body.Shape.BoundBox.ZMax,20) + def create_t_sketch(self): self.Doc.getObject('Body').newObject('Sketcher::SketchObject', 'Sketch') geo_list = [ @@ -818,4 +909,5 @@ class TestTopologicalNamingProblem(unittest.TestCase): def tearDown(self): """ Close our test document """ - App.closeDocument("PartDesignTestTNP") + # App.closeDocument("PartDesignTestTNP") + pass \ No newline at end of file From 920032c5a0ae044922a83b60161f301180b117fe Mon Sep 17 00:00:00 2001 From: bgbsww Date: Fri, 3 May 2024 12:07:02 -0400 Subject: [PATCH 3/3] Lint corrections --- src/App/PropertyLinks.cpp | 4 +++- .../PartDesignTests/TestTopologicalNamingProblem.py | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/App/PropertyLinks.cpp b/src/App/PropertyLinks.cpp index 2dfeff7e2e..c0c02d3e45 100644 --- a/src/App/PropertyLinks.cpp +++ b/src/App/PropertyLinks.cpp @@ -1205,6 +1205,8 @@ static inline const std::string &getSubNameWithStyle(const std::string &subName, return tmp; } } +#else + (void) tmp; #endif return shadow.first; } @@ -1259,8 +1261,8 @@ PyObject *PropertyLinkSub::getPyObject() Py::List list(static_cast(_cSubList.size())); if (_pcLinkSub) { tup[0] = Py::asObject(_pcLinkSub->getPyObject()); - int i = 0; #ifdef FC_USE_TNP_FIX + int i = 0; for (auto &sub : getSubValues(true)) list[i++] = Py::String(sub); #else diff --git a/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py b/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py index 8cfb7d4af2..a66113eb22 100644 --- a/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py +++ b/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py @@ -980,4 +980,3 @@ class TestTopologicalNamingProblem(unittest.TestCase): def tearDown(self): """ Close our test document """ App.closeDocument("PartDesignTestTNP") - pass \ No newline at end of file