sofa icon indicating copy to clipboard operation
sofa copied to clipboard

[SofaCore] Reorganize ObjectFactory so it becomes possible to refactor it in a smooth way

Open damienmarchal opened this issue 3 years ago • 5 comments

Refactoring ObjectFactory without braking to much sofa is rather complex. To ease in the process I splitted the ObjectFactory class in three entity.

sofa::core::future::ObjectFactory and sofa::core::future::ObjectFactoryInstance are the new versions while sofa::core::ObjectFactory is a compatibility layer exposing the old API for transitionning.

For the moment, 100% of the old API is reproduced and emits deprecation warning. As the code change are rather important it is easier to read the source code instead of the diff.

sofa::core::future::ObjectFactory is implementing the factory mecanisme without any static in it. sofa::core::future::ObjectFactoryInstance is a single-instance factory accessible through static method.


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

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

damienmarchal avatar Feb 19 '22 16:02 damienmarchal

[ci-build][with-all-tests]

guparan avatar Feb 22 '22 15:02 guparan

Hey @damienmarchal

what is the scope of this PR? Is it part of the "refactoring the whole class introspection system" as you mentioned in your 1087's reply?

anyway, I see here two topics in one PR:

  • introduction of the future namespace : I understand that it is for keeping compatibility with previous codes, but that's the purpose of the dev branch to include new features, isn't?
  • refactoring of the ObjectFactory : why not just updating the existing ObjectFactory ?

hugtalbot avatar Feb 25 '22 10:02 hugtalbot

CI is also failing: ObjectFactory.h:45:89: error: no type named 'BaseObjectDescription' in namespace 'sofa::core::objectmodel'

hugtalbot avatar Feb 25 '22 10:02 hugtalbot

@hugtalbot

  • it is not related to #1087,
  • it is more to prepare the evolution from the class that mix in a single object a static/singleton part of the object and a part that shouldn't be one. It was often said in discussion that such design was highly questionnable.
  • i prefer to have such move before implementing the import/namespace in the factory demonstrated in https://github.com/sofa-framework/sofa/pull/2512

About the use of the "future" namespace I use it to clearly show that there is two "ObjectFactory". The one with the old API (and it fully backward compatible) and the one with the new API. I could of course have mixed everything (the old and the new API) in a single one object but I found it quite convenient to clearly see the objects future::ObjectFatory and ObjectFactoryInstance in their final state and well separated from the "compatbility" layer in ObjectFactory.
I could have use a different class name for future::ObjectFactory like NewObjectFactory but this is would have been behaving exactly like having a namespace glued to the class name (actually I see this a namespace hidden in the classname so people don't notice it is one ;)) I also consider using the approach of SofaNG to make deprecation but this only work if we move the new version of the object to be in a different namespace that the one we want to deprecate. In the present case I wasn't sure we really want to move sofa::core;;ObjectFactory somewhere less, if you find an appropriate suggestion i would be very welcome as I would found that easier than using the "extra" namespace. In NG we often do such a thing but this is done like that:

  1. move the new API to a new namespace (eg: sofa::type::)
  2. make a deprecation layer (eg: sofa::defaulttypes::) In the ObjectFactory cas I'm not sure there is a need to have the new api moved in a new namespace (I see no problem in having it in sofa::core)

EDIT: and "future" this is nothing related to master branch. As being in master branch doesn't tells how deprecated API should be implemented on top of the new one during the transitional period.

damienmarchal avatar Feb 28 '22 17:02 damienmarchal

issue with the config.h / macro _API I guess :

error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl sofa::core::future::ObjectFactoryInstance::ObjectCreator<class sofa::simulation::DefaultAnimationLoop>::ObjectCreator<class sofa::simulation::DefaultAnimationLoop>(void)" ```

hugtalbot avatar Mar 02 '22 10:03 hugtalbot