[SofaPython] General SofaPython cleaning

[matthieu-nesme]

EDITED: Damien

Currently the SofaPython plugin is in a very poor shapes with two serious problems. The first one is a problem for developper with a lot of duplicated or invalid or in-elegant code which impact maintainability of our code base. The second problem is the lack of consistency in the way warning and errors are reported. In sofa some component rise en exception while others prints a message and return None which is very problematic to users.

In this PR we addressed these two issues through a major cleaning set of changes:

• [SofaPython]:

  • refactor the way to get get Sofa object from their PyObject equivalent (hundred of changes).
    • add dedicated easing function (unwrap, get, get_baseobject)
    • replace wherever possible copy-pasted code with these function.
  • refactor the way we report error from binding (hundred of changes) (API BREAK).
    • returning NULL with a correctly set exception type as it is the way of handling error in python.
    • when returning NULL it is mandatory to preceed the return with an exception type & message specification. This can be done either with PyExc_SetString() or Py_BaddArgument(),
    • when returning after a failing Py_ParseTuple is an exception, as the function sets itself a very descriptive and standardized error message.
    • add macros (SP_PYERR_SETSTRING_INVALIDTYPE and SP_PYERR_SETSTRING_OUTOFBOUND) to report consistant message with PyErr_SetString
    • add SP_CLASS_METHOD_DOC() and SP_CLASS_METHOD_KW_DOC() to declare method with their docstring (if you don't know what are docstring read: http://sametmax.com/les-docstrings/)
  • add new bindings:
    • BaseState::getSize
    • BaseMapping::applyJT
    • BaseMapping::applyDJT
    • Data::getCounter
    • Data::isDirty
    • Node::getMass
    • Node::getForceField
    • STLExporter
    • Sofa.unload
    • Sofa.path
    • Sofa.getAvailableComponents
    • VisualModel::updateVisual
    • VisualModel::initVisual
  • add in PythonEnvironment::getPythonCallingPointAsFileInfo() a function that return a string with the python file/line info to add report them in msg_*
  • adds 'override' keyword when needed (more info => http://en.cppreference.com/w/cpp/language/override)
  • adds tests on PythonScriptController
  • adds tests on SofaModule
  • adds a exception handler in python so that the un-catched exceptions are sended as msg_error instead of being printed on the console.
  • cosmetic changes like replacing 'using namespace a;' with the precise 'using a:TheClass', removing in-consistant line spacing.
  • replace "extern C" with static

• [Flexible/python] Fix the examples & the tests to take into account the changes in SofaPython

• [Compliant/python] Fix the examples & the tests to take into account the changes in SofaPython

To simplify the submitter's life (and don't waste their time) some extras (read not really relevant) changes are also added to the PR:

• remove a virtual in-heritance on BaseController and plugin/optitracknet.
• adds a << operator to Message.h to handle file:line information (now in PR: #314 )
• made some changes in STLExporter to stay consistant with Sofa guidelines.

There is still some work todo (if you have free time to offer): Eg: in Compliant there is still patterns like:

     SP_MESSAGE_ERROR( "_Compliant_getAssembledImplicitMatrix: wrong arguments" );
     return NULL;

The SP_MESSAGE_ERROR is probably not needed as it duplicate the one provided by the python exception.

While in

   SP_MESSAGE_ERROR( "_Compliant_getAssembledImplicitMatrix: first argument is not a BaseNode" );
  PyErr_BadArgument();
  return NULL;

PyExc_SetString() should replace the message error & the bad argument.

More generally there is still a lot of SPMESSAGE() instead of msg_ and there is a lot of method that haven't their docstring.

---

This PR:

• [ ] builds with SUCCESS for all platforms on the CI.
• [ ] does not generate new warnings.
• [ ] does not generate new unit test failures.
• [ ] does not generate new scene test failures.
• [ ] does not break API compatibility.
• [ ] is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

[matthieu-nesme]

I just cherry-picked Anatoscope's changes related to SofaPython (excl. PythonScriptController that comes in #283). It seems huge, but it is simply:

• mainly cleaning-up,
• adding bindings,
• refactoring python-tests.

More cleanings are coming, and waiting for #283 and #286.

[damienmarchal]

@matthieu-nesme I like this PR a lot :)

[bcarrez]

It looks huge, for sure ;-)

[maxime-tournier]

A little update:

• added wrap/unwrap mechanism: there are wrapping traits in PythonToSofa.inl specifying wrapped type (PySPtr or PyPtr) for a given type (defaults to PyPtr)
• wrapping traits specializations for Base, BaseData and BaseLink-derived classes. Base-derived classes are wrapped as PySPtr<Base>, BaseData-derived as PyPtr<BaseData>, etc. this should cover most cases.
• unwrap<T>(py_obj) first recovers the wrapped type and the object pointer, then dynamic_casts<T*>
• unwrap_self<T>(py_obj) does the same but with static_cast (this is for self arguments, python enforces that self is an instance of this type).

Extension code should look like this now:

Node* node = sofa::py::unwrap_self<Node>(self);
BaseObject* obj = sofa::py::unwrap<BaseObject>(py_obj); if(!obj) { ... }

[hugtalbot]

Nice work guys, but such a work is more dedicated to a branch and pull-request little by little the features ! here the work is un-readable for other devs.

[damienmarchal]

@matthieu-nesme and @maxime-tournier do you agree if we cut this PR in four on a per files basis instead of per commit ? It could be:

• Sofa base
• SofaPython
• Flexible
• Compliant

[bcarrez]

[ci-build]

[damienmarchal]

@hugtalbot To me this PR is a prefect example of where we could "relax/ease" the merging process.

From the submitter point of view we are "giving" to the community more than 50h of work and then we are requested to do more to split the PR. At some point we need to be a bit more pragmatic, having a "industrial grade" reviewing process is great, but do we have the budget for that ?

In this PR, mathieu, maxime and I contributed...if it breaks something in master, which is a development branch anyway, ...we will be there to fix it a-posteriori.

But you are right, this PR should have a better description and this will be done when it will be taggued to review :)

[maxime-tournier]

@damienmarchal ok for splitting, this is getting too big anyways :-)

[damienmarchal]

Ok, I will do that this evening.

[matthieu-nesme]

I am not sure it is doable as things must depend on each other.

[damienmarchal]

[ci-build]

[damienmarchal]

@hugtalbot, @guparan
I really would like to take the easy path of merging the whole PR.

This PR can break compatibility...but to me this important to allow breaking cleaning (and to have that done in a single step is far easier (in term of extra management work) than doing that in multiple small PR.

[hugtalbot]

[ci-build]

[hugtalbot]

sorry for the delay @damienmarchal , I merge it today as soon as the build is done

[hugtalbot]

hey @damienmarchal is it normal that the test testCreateObjectDataConversionWarning is failing since your last commit ?

[damienmarchal]

@hugtalbot No it is not..sorry for that.. I will fix the conflict and the failing test.

[damienmarchal]

@hugtalbot I fixed the test, is was not updated to handle the change between repr and str as well as the commit.

[hugtalbot]

@damienmarchal sure ? it's look like the test is still failing, isn't it ?

[damienmarchal]

My bad (again). I forgot to push to upstream and not origin.

[hugtalbot]

[ci-build]