From 24c22d3231a4c88d5d29c995b2f3fd7a1fb75ba7 Mon Sep 17 00:00:00 2001 From: Damien Marchal Date: Sat, 29 Aug 2020 08:49:19 +0200 Subject: [PATCH 1/3] [SofaPython3] Remove -at-best- the use of this infamous string conversion while creating an object. --- Plugin/src/SofaPython3/DataHelper.cpp | 32 +++- Plugin/src/SofaPython3/DataHelper.h | 177 +++++++----------- .../SofaPython3/Sofa/Core/Binding_Node.cpp | 21 ++- 3 files changed, 110 insertions(+), 120 deletions(-) diff --git a/Plugin/src/SofaPython3/DataHelper.cpp b/Plugin/src/SofaPython3/DataHelper.cpp index 3e73f0db..e418542e 100644 --- a/Plugin/src/SofaPython3/DataHelper.cpp +++ b/Plugin/src/SofaPython3/DataHelper.cpp @@ -45,25 +45,48 @@ std::string toSofaParsableString(const py::handle& p) if(py::isinstance(p) || py::isinstance(p)) { std::stringstream tmp; - for(auto pa : p){ + for(auto pa : p) + { tmp << toSofaParsableString(pa) << " "; } return tmp.str(); } //TODO(dmarchal) This conversion to string is so bad. if(py::isinstance(p)) + { return py::str(p); + } + return py::repr(p); } -/// RVO optimized function. Don't care about copy on the return code. void fillBaseObjectdescription(sofa::core::objectmodel::BaseObjectDescription& desc, - const py::dict& dict) + const py::dict& dict) + { + for(auto kv : dict) + { + desc.setAttribute(py::str(kv.first), toSofaParsableString(kv.second)); + } + + return; + } + +void processKwargsForObjectCreation(const py::dict dict, + py::list& parametersToLink, + py::list& parametersToCopy, + sofa::core::objectmodel::BaseObjectDescription& parametersAsString) { + auto t = py::detail::get_type_handle(typeid(BaseData), false); for(auto kv : dict) { - desc.setAttribute(py::str(kv.first), toSofaParsableString(kv.second)); + if (py::isinstance(kv.second, t)) + parametersToLink.append(kv.first); + else if (py::isinstance(kv.second)) + parametersAsString.setAttribute(py::str(kv.first), py::cast(kv.second)); + else + parametersToCopy.append(kv.first); } + return; } PythonTrampoline::~PythonTrampoline(){} @@ -76,6 +99,7 @@ void PythonTrampoline::setInstance(py::object s) pyobject = std::shared_ptr( s.ptr(), [](PyObject* ob) { + SOFA_UNUSED(ob); // runSofa Sofa/tests/pyfiles/ScriptController.py => CRASH // Py_DECREF(ob); }); diff --git a/Plugin/src/SofaPython3/DataHelper.h b/Plugin/src/SofaPython3/DataHelper.h index a73388c9..710a92ed 100644 --- a/Plugin/src/SofaPython3/DataHelper.h +++ b/Plugin/src/SofaPython3/DataHelper.h @@ -42,119 +42,63 @@ along with sofaqtquick. If not, see . ////////////////////////// FORWARD DECLARATION /////////////////////////// namespace sofa { - namespace defaulttype { - class AbstractTypeInfo; - } - namespace core { - namespace objectmodel { - class BaseData; - - - class SOFAPYTHON3_API PrefabLink - { - public: - PrefabLink() {} - PrefabLink(const Base::SPtr& targetBase) { m_targetBase = targetBase; } - PrefabLink(BaseLink* targetLink) { m_targetBase = targetLink->getLinkedBase(); } - PrefabLink(const std::string& targetPath) { m_targetPath = targetPath; } - - const Base::SPtr& getTargetBase() const { return m_targetBase; } - void setTargetBase(const Base::SPtr& targetBase) { m_targetBase = targetBase; } - - const std::string& getTargetPath() const { return m_targetPath; } - void setTargetPath(const std::string& targetPath) { m_targetPath = targetPath; } - - friend std::ostream& operator << ( std::ostream& out, const PrefabLink& l) - { - if (l.getTargetBase()) - { - auto bn = l.getTargetBase()->toBaseNode(); - auto bo = l.getTargetBase()->toBaseObject(); - out << "@" + (bn ? bn->getPathName() : bo->getPathName()); - } - out << l.getTargetPath(); - return out; - } - - friend std::istream& operator >> ( std::istream& in, PrefabLink& l) - { - std::string s; - in >> s; - l.setTargetPath(s); - return in; - } - - private: - Base::SPtr m_targetBase { nullptr }; - std::string m_targetPath {""}; - }; - - class SOFAPYTHON3_API DataLink : public Data - { - typedef Data Inherit; - - DataLink( const std::string& helpMsg="", bool isDisplayed=true, bool isReadOnly=false ) - : Inherit(helpMsg, isDisplayed, isReadOnly) - { - } - - DataLink( const std::string& value, const std::string& helpMsg="", bool isDisplayed=true, bool isReadOnly=false ) - : Inherit(value, helpMsg, isDisplayed, isReadOnly) - { - } - - explicit DataLink(const BaseData::BaseInitData& init) - : Inherit(init) - { - } - - const PrefabLink& getValue() const - { - updateIfDirty(); - if (m_value.getValue().getTargetBase()) return m_value.getValue(); - - auto self = const_cast(this); - - Base* dst = nullptr; - this->getOwner()->findLinkDest(dst, self->m_value.getValue().getTargetPath(), nullptr); - if (dst) { - auto edit = self->m_value.beginEdit(); - edit->setTargetBase(dst); - edit->setTargetPath(""); - self->m_value.endEdit(); - } - return m_value.getValue(); - } - - std::string getValueString() const - { - const auto& ptr = getValue(); - if (ptr.getTargetBase()) - { - auto bn = ptr.getTargetBase()->toBaseNode(); - auto bo = ptr.getTargetBase()->toBaseObject(); - return "@" + (bn ? bn->getPathName() : bo->getPathName()); - } - return ptr.getTargetPath(); - } - - - bool read(const std::string& value) - { - Base* dst; - auto data = m_value.beginEdit(); - if (this->getOwner()->findLinkDest(dst, value, nullptr) && dst != nullptr) - data->setTargetBase(dst); - else { - data->setTargetBase(nullptr); - data->setTargetPath(value); - } - return true; - } - }; +namespace defaulttype { +class AbstractTypeInfo; +} +namespace core { +namespace objectmodel { +class BaseData; + + +class SOFAPYTHON3_API PrefabLink +{ +public: + PrefabLink() {} + PrefabLink(const Base::SPtr& targetBase) { m_targetBase = targetBase; } + PrefabLink(BaseLink* targetLink) { m_targetBase = targetLink->getLinkedBase(); } + PrefabLink(const std::string& targetPath) { m_targetPath = targetPath; } + const Base::SPtr& getTargetBase() const { return m_targetBase; } + void setTargetBase(const Base::SPtr& targetBase) { m_targetBase = targetBase; } + + const std::string& getTargetPath() const { return m_targetPath; } + void setTargetPath(const std::string& targetPath) { m_targetPath = targetPath; } + + friend std::ostream& operator << ( std::ostream& out, const PrefabLink& l) + { + if (l.getTargetBase()) + { + auto bn = l.getTargetBase()->toBaseNode(); + auto bo = l.getTargetBase()->toBaseObject(); + out << "@" + (bn ? bn->getPathName() : bo->getPathName()); } + out << l.getTargetPath(); + return out; + } + + friend std::istream& operator >> ( std::istream& in, PrefabLink& l) + { + std::string s; + in >> s; + l.setTargetPath(s); + return in; } + +private: + Base::SPtr m_targetBase { nullptr }; + std::string m_targetPath {""}; +}; +} +} +namespace defaulttype +{ +template <> +struct DataTypeName +{ + static const char* name() { return "PrefabLink"; } +}; + +} } /////////////////////////////// DECLARATION ////////////////////////////// @@ -192,7 +136,7 @@ template class py_shared_ptr : public sofa::core::sptr SOFAPYTHON3_API void setItem2D(py::array a, py::slice slice, py::object o); SOFAPYTHON3_API void setItem2D(py::array a, const py::slice& slice, - const py::slice& slice1, py::object o); + const py::slice& slice1, py::object o); SOFAPYTHON3_API void setItem1D(py::array a, py::slice slice, py::object o); SOFAPYTHON3_API void setItem(py::array a, py::slice slice, py::object value); @@ -216,11 +160,16 @@ void SOFAPYTHON3_API copyFromListScalar(BaseData& d, const AbstractTypeInfo& nfo std::string SOFAPYTHON3_API toSofaParsableString(const py::handle& p); -//py::object SOFAPYTHON3_API dataToPython(BaseData* d); - /// RVO optimized function. Don't care about copy on the return code. void SOFAPYTHON3_API fillBaseObjectdescription(sofa::core::objectmodel::BaseObjectDescription& desc, - const py::dict& dict); + const py::dict& dict); + +/// Split the content of the dictionnary 'dict' in three set. +/// On containing the data to parent, one containing the data to copy and on containing the data to parse in the BaseObjectDescription +void SOFAPYTHON3_API processKwargsForObjectCreation(const py::dict dict, + py::list& parametersToLink, + py::list& parametersToCopy, + sofa::core::objectmodel::BaseObjectDescription& parametersAsString); template void copyScalar(BaseData* a, const AbstractTypeInfo& nfo, py::array_t src) diff --git a/bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp b/bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp index d2a94f03..effa78f0 100644 --- a/bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp +++ b/bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp @@ -166,7 +166,9 @@ py::object addObjectKwargs(Node* self, const std::string& type, const py::kwargs /// Prepare the description to hold the different python attributes as data field's /// arguments then create the object. BaseObjectDescription desc {type.c_str(), type.c_str()}; - fillBaseObjectdescription(desc, kwargs); + py::list parametersToCopy; + py::list parametersToLink; + processKwargsForObjectCreation(kwargs, parametersToLink, parametersToCopy, desc); auto object = ObjectFactory::getInstance()->createObject(self, &desc); /// After calling createObject the returned value can be either a nullptr @@ -195,7 +197,13 @@ py::object addObjectKwargs(Node* self, const std::string& type, const py::kwargs { BaseData* d = object->findData(py::cast(a.first)); if(d) + { + if (parametersToLink.contains(a.first)) + d->setParent(a.second.cast()); + else if(parametersToCopy.contains(a.first)) + PythonFactory::fromPython(d, py::cast(a.second)); d->setPersistent(true); + } } return PythonFactory::toPython(object.get()); } @@ -258,7 +266,10 @@ py::object addChildKwargs(Node* self, const std::string& name, const py::kwargs& if (sofapython3::isProtectedKeyword(name)) throw py::value_error("addChild: Cannot call addChild with name " + name + ": Protected keyword"); BaseObjectDescription desc (name.c_str()); - fillBaseObjectdescription(desc,kwargs); + py::list parametersToCopy; + py::list parametersToLink; + processKwargsForObjectCreation(kwargs, parametersToLink, parametersToCopy, desc); + auto node=simpleapi::createChild(self, desc); checkParamUsage(desc); @@ -266,7 +277,13 @@ py::object addChildKwargs(Node* self, const std::string& name, const py::kwargs& { BaseData* d = node->findData(py::cast(a.first)); if(d) + { + if (parametersToLink.contains(a.first)) + d->setParent(a.second.cast()); + else if(parametersToCopy.contains(a.first)) + PythonFactory::fromPython(d, py::cast(a.second)); d->setPersistent(true); + } } return py::cast(node); From 61899a993f12c6acc721b0474631f9a77f9f9121 Mon Sep 17 00:00:00 2001 From: Damien Marchal Date: Mon, 31 Aug 2020 23:24:40 +0200 Subject: [PATCH 2/3] [bindings/Sofa] Return type_error when trying to create an object with invalid argument. (this was the previous behavior) --- bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp b/bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp index effa78f0..dab438b9 100644 --- a/bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp +++ b/bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp @@ -157,17 +157,20 @@ py::object addObject(Node& self, BaseObject* object) /// Implement the addObject function. py::object addObjectKwargs(Node* self, const std::string& type, const py::kwargs& kwargs) { + /// Check that the name of the component is not one of the protected keywords (like: children, nodes,and others...) if (kwargs.contains("name")) { std::string name = py::str(kwargs["name"]); if (sofapython3::isProtectedKeyword(name)) throw py::value_error("addObject: Cannot call addObject with name " + name + ": Protected keyword"); } + /// Prepare the description to hold the different python attributes as data field's /// arguments then create the object. BaseObjectDescription desc {type.c_str(), type.c_str()}; py::list parametersToCopy; py::list parametersToLink; + processKwargsForObjectCreation(kwargs, parametersToLink, parametersToCopy, desc); auto object = ObjectFactory::getInstance()->createObject(self, &desc); @@ -203,18 +206,20 @@ py::object addObjectKwargs(Node* self, const std::string& type, const py::kwargs else if(parametersToCopy.contains(a.first)) PythonFactory::fromPython(d, py::cast(a.second)); d->setPersistent(true); + }else + { + throw py::type_error("Unknown Attribute: '"+py::cast(a.first)+"'"); } } return PythonFactory::toPython(object.get()); } -/// Implement the addObject function. +/// Implement the add(callable, xxx) function. py::object addKwargs(Node* self, const py::object& callable, const py::kwargs& kwargs) { if(py::isinstance(callable)) { BaseObject* obj = py::cast(callable); - self->addObject(obj); return py::cast(obj); } From b4b7caeea96abfb3def6edbfabc76fdb73b8da09 Mon Sep 17 00:00:00 2001 From: Damien Marchal Date: Mon, 31 Aug 2020 23:30:40 +0200 Subject: [PATCH 3/3] [bindings/Sofa/tests] Change few tests in ForceField and Node. The PR is breaking existing code. But the previously working code was, despite being conveniant was not valid and it was using a "flat" array as a multi-dimensionnal structure. It was not noticed before because the lists were converted into a 1D array then into a big string. Two options were possibles: a) not allowing this kind of code (using a 1D array to fill a 2D structure) b) implement kind of de-flattening a python lists into the corresponding multidimmensional matrix For the simplicity I choose option (a), so I updated the tests to use the 'right' syntax. --- bindings/Sofa/tests/Core/ForceField.py | 2 +- bindings/Sofa/tests/Simulation/Node.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bindings/Sofa/tests/Core/ForceField.py b/bindings/Sofa/tests/Core/ForceField.py index 12cf3d85..ab1a26bb 100644 --- a/bindings/Sofa/tests/Core/ForceField.py +++ b/bindings/Sofa/tests/Core/ForceField.py @@ -30,7 +30,7 @@ def createParticle(node, node_name, use_implicit_scheme, use_iterative_solver): p = node.addChild(node_name) createIntegrationScheme(p, use_implicit_scheme) createSolver(p, use_iterative_solver) - dofs = p.addObject('MechanicalObject', name="MO", position=[0, 0, 0]) + dofs = p.addObject('MechanicalObject', name="MO", position=[[0, 0, 0]]) p.addObject('UniformMass', totalMass=1.0) print ("dofs.rest_position " + str(dofs.rest_position.value)) diff --git a/bindings/Sofa/tests/Simulation/Node.py b/bindings/Sofa/tests/Simulation/Node.py index 84745a0b..5c452985 100644 --- a/bindings/Sofa/tests/Simulation/Node.py +++ b/bindings/Sofa/tests/Simulation/Node.py @@ -45,7 +45,7 @@ def test_GetAttr(self): def test_init(self): root = Sofa.Core.Node("rootNode") c = root.addChild("child1") - c = c.addObject("MechanicalObject", name="MO", position=[0.0,1.0,2.0]*100) + c = c.addObject("MechanicalObject", name="MO", position=[[0.0,1.0,2.0]]*100) root.init() print("TYPE: "+str(len(c.position.value))) self.assertEqual(len(c.position.value), 100)