Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply new registration mechanism #141

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bakpaul
Copy link
Collaborator

@bakpaul bakpaul commented Nov 29, 2024

This applies the new factory registration mechanism as introduced by [All] ObjectFactory: Explicit components registration #4429

In the process I've encountered a problem of consistency of namespace. I needed all of the registering methods to be in the Cosserat namespace. But a lot of SOFA namespace are used. This is bad practice, only the Cosserat namespace should be used in this plugin.

@bakpaul
Copy link
Collaborator Author

bakpaul commented Dec 2, 2024

Please test it on your side before merging: while testing it on my side using the examples, I've had a lots of warning because of multiple registration in the factory. This might be due to multiple loading of the library. Problem is that the example are using some custom headers and so on, I preferred to have your feedback before taking a closer look.

Copy link
Collaborator

@adagolodjo adagolodjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it stands, I think the code is good. But, you're right, I'll try to compile it on my machine and test scenes before a possible merge.

Copy link
Member

@alxbilger alxbilger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, @adagolodjo could you take the opportunity to review all the Component descriptions? I see a lot of copy/paste.

@@ -35,61 +35,60 @@

//#define SOFTROBOTS_CONSTRAINT_QPSLIDINGCONSTRAINT_NOEXTERN

namespace sofa::component::constraintset
{
namespace sofa::component::constraintset {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
namespace sofa::component::constraintset {
namespace sofa::component::constraintset
{


//////////////////////////////////////////// FACTORY //////////////////////////////////////////////
// Registering the component
// see: http://wiki.sofa-framework.org/wiki/ObjectFactory
// 1-RegisterObject("description") + .add<> : Register the component
// 2-.add<>(true) : Set default template

int QPSlidingConstraintClass = RegisterObject("Simulate cable actuation.")
.add< QPSlidingConstraint<sofa::defaulttype::Vec3Types> >(true)
void registerQPSlidingConstraint(sofa::core::ObjectFactory* factory)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment just before must be removed. Also in other files

.add< QPSlidingConstraint<sofa::defaulttype::Vec3Types> >(true)
void registerQPSlidingConstraint(sofa::core::ObjectFactory* factory)
{
factory->registerObjects(sofa::core::ObjectRegistrationData("Simulate cable actuation.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adagolodjo Can you give a better description? Here, it's copy/paste.

.add< ProjectionEngine<Vec3Types> >(true);
void registerProjectionEngine(sofa::core::ObjectFactory *factory) {
factory->registerObjects(
sofa::core::ObjectRegistrationData("TODO-ProjectionEngine")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adagolodjo Can you give a better description?

@@ -32,164 +32,171 @@

#include <sofa/core/ObjectFactory.h>

namespace sofa::component::forcefield
{
namespace sofa::component::forcefield {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the changes in this files. Why so much changes? Indentation maybe?

Comment on lines +41 to +44
#ifdef COSSERAT_USES_SOFTROBOTS
extern void registerQPSlidingConstraint(sofa::core::ObjectFactory* factory);
extern void registerCosseratActuatorConstraint(sofa::core::ObjectFactory* factory);
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be moved in a Cosserat extension (in a later PR)

extern void registerCosseratActuatorConstraint(sofa::core::ObjectFactory* factory);
#endif

extern void registerCosseratNeedleSlidingConstraint(sofa::core::ObjectFactory* factory);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is extern keyword needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants