[SofaCore] Add virtual getPathName function in Base

[marques-bruno]

Both BaseObject and BaseNode have a getPathName function, but those can't be called from a Base ptr without dereferencing with toBaseNode / toBaseObject.

---

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.

[marques-bruno]

This is ugly, I don't like it much but I can't put a virtual pure here since BaseContext also inherits Base, which means more than 1 final overrider for Node (inherits BaseNode & BaseContext...)

[marques-bruno]

This has nothing to do with the PR, but I need this to be overridable for custom Data in SofaQtQuick

[damienmarchal]

Hello @marques-bruno

My assumption is that, initially, Base was supposed to be more the Base class for "everything" and not only for scenegraph entries. This is why the initial design was that there is no "path" concept at this level.

My current approach to handle this kind of scenario was to implement an easing function like that:

std::string sofa::helper::objectmodel::getPathName(Base* o)
{
        BaseNode *node = dynamic_cast<BaseNode>(o);
        if(node)
            return node->getPathName();
        BaseObject *object = dynamic_cast<BaseNode>(o);
        if(object)
            return object->getPathName();
        ...
}

To avoid having if/else and dynamic cast at every call and thus easing the readability on the caller's.
But I agree with your that factoring them out in Base would make sense. Actually some refactoring of BaseNode/Node/DAGNode/BaseContext would also make sense given how hard these are interleaved and unclear to most of the developpers.

[marques-bruno]

Hello @marques-bruno

My assumption is that, initially, Base was supposed to be more the Base class for "everything" and not only for scenegraph entries. This is why the initial design was that there is no "path" concept at this level.

My current approach to handle this kind of scenario was to implement an easing function like that:

std::string sofa::helper::objectmodel::getPathName(Base* o)
{
        BaseNode *node = dynamic_cast<BaseNode>(o);
        if(node)
            return node->getPathName();
        BaseObject *object = dynamic_cast<BaseNode>(o);
        if(object)
            return object->getPathName();
        ...
}

To avoid having if/else and dynamic cast at every call and thus easing the readability on the caller's.
But I agree with your that factoring them out in Base would make sense. Actually some refactoring of BaseNode/Node/DAGNode/BaseContext would also make sense given how hard these are interleaved and unclear to most of the developpers.

Let's get rid of BaseContext! ;)
More seriously, I was also using easing functions before, but I noticed that I was often just duplicating the code whenever I needed it, and felt like having it in Base made more sense, but I must say that I would have preferred a virtual pure method there.. your alternative (adding in SofaCore an easing function in the sofa::core::objectmodel or sofa::helper::objectmodel scope) makes sense to me, solves the problem of having an extra function to override when inheriting Base.
Do you know if there is an improvement in perfs when using toBaseNode() / toBaseObject() instead of a dynamic_cast here?

I would not bet too much on performance improvement without making small benchmark. But in a function that basically returns strings build by concatenating ... I would say this is probably very small.

In addition it has to be noted that using a getPathName() explingn function with if/else has a linear complexity in the amount of possible types (number of if/else) while a virtual getPathName in Base has a constant cost. But again...the calling costs is probably very small compared to what the function actually do :)

[damienmarchal]

[ci-build]