Skip to content

[Framework] Apply template method design pattern to Contact #5456

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.

Sign up for GitHub

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

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

[Framework] Apply template method design pattern to Contact #5456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
80eb692
feat(sed_sprint): apply template method to Contact
NiziL May 13, 2025
4e7e3c8
feat(sed_sprint): update child class BarycentricStickContact
NiziL May 13, 2025
1c0c22e
feat(sed_sprint): update child class BarycentricPenaltyContact
NiziL May 13, 2025
f2cf951
feat(sed_sprint): add forgotten comments
NiziL May 13, 2025
4924366
feat(sed_sprint): update child class BaseUnilateralContactResponse
NiziL May 13, 2025
80f161b
feat(sed_sprint): update child class RayContact
NiziL May 13, 2025
4bd3c9d
feat(sed_sprint): update child class StickContactConstraint
NiziL May 13, 2025
952c1b5
fix: RayContact getDetectionOutputs should not be doGet
NiziL May 13, 2025
32d91fc
fix: comments
NiziL May 13, 2025
839919f
feat(sed_sprint): update child class PersistentFrictionContact
NiziL May 14, 2025
79dbaf0
feat(sed_sprint): update child class CudaCollisionDistanceGrid
NiziL May 14, 2025
cd0795b
fix: add forgotten override
NiziL May 14, 2025
ac5ac59
fix: add forgotten override
NiziL May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions ...sponse/Contact/src/sofa/component/collision/response/contact/BarycentricPenalityContact.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,19 @@ class BarycentricPenalityContact : public core::collision::Contact
public:
void cleanup() override;

std::pair<core::CollisionModel*,core::CollisionModel*> getCollisionModels() override { return std::make_pair(model1,model2); }
void draw(const core::visual::VisualParams* vparams) override;

void setDetectionOutputs(OutputVector* outputs) override;
protected:
std::pair<core::CollisionModel*,core::CollisionModel*> doGetCollisionModels() override
{
return std::make_pair(model1,model2);
}

void createResponse(core::objectmodel::BaseContext* group) override;
void doSetDetectionOutputs(OutputVector* outputs) override;

void removeResponse() override;
void doCreateResponse(core::objectmodel::BaseContext* group) override;

void draw(const core::visual::VisualParams* vparams) override;
void doRemoveResponse() override;

};

Expand Down
6 changes: 3 additions & 3 deletions ...onse/Contact/src/sofa/component/collision/response/contact/BarycentricPenalityContact.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ void BarycentricPenalityContact<TCollisionModel1,TCollisionModel2,ResponseDataTy
}

template < class TCollisionModel1, class TCollisionModel2, class ResponseDataTypes >
void BarycentricPenalityContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::setDetectionOutputs(OutputVector* o)
void BarycentricPenalityContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::doSetDetectionOutputs(OutputVector* o)
{
TOutputVector& outputs = *static_cast<TOutputVector*>(o);
if (ff==nullptr)
Expand Down Expand Up @@ -188,7 +188,7 @@ void BarycentricPenalityContact<TCollisionModel1,TCollisionModel2,ResponseDataTy
}

template < class TCollisionModel1, class TCollisionModel2, class ResponseDataTypes >
void BarycentricPenalityContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::createResponse(core::objectmodel::BaseContext* group)
void BarycentricPenalityContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::doCreateResponse(core::objectmodel::BaseContext* group)
{
if (ff!=nullptr)
{
Expand All @@ -207,7 +207,7 @@ void BarycentricPenalityContact<TCollisionModel1,TCollisionModel2,ResponseDataTy
}

template < class TCollisionModel1, class TCollisionModel2, class ResponseDataTypes >
void BarycentricPenalityContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::removeResponse()
void BarycentricPenalityContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::doRemoveResponse()
{
if (ff!=nullptr)
{
Expand Down
20 changes: 12 additions & 8 deletions .../Response/Contact/src/sofa/component/collision/response/contact/BarycentricStickContact.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,21 +74,25 @@ class BarycentricStickContact : public core::collision::Contact

void cleanup() override;

std::pair<core::CollisionModel*,core::CollisionModel*> getCollisionModels() override { return std::make_pair(model1,model2); }

void setDetectionOutputs(OutputVector* outputs) override;

void createResponse(core::objectmodel::BaseContext* group) override;

void removeResponse() override;

/// Return true if this contact should be kept alive, even if objects are no longer in collision
bool keepAlive() override { return d_keepAlive.getValue(); }

/// Control the keepAlive flag of the contact.
void setKeepAlive(bool val) override { d_keepAlive.setValue(val); }

void draw(const core::visual::VisualParams* vparams) override;

protected:
std::pair<core::CollisionModel*,core::CollisionModel*> doGetCollisionModels() override
{
return std::make_pair(model1,model2);
}

void doSetDetectionOutputs(OutputVector* outputs) override;

void doCreateResponse(core::objectmodel::BaseContext* group) override;

void doRemoveResponse() override;
};

} // namespace sofa::component::collision::response::contact
6 changes: 3 additions & 3 deletions ...esponse/Contact/src/sofa/component/collision/response/contact/BarycentricStickContact.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ void BarycentricStickContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes
}

template < class TCollisionModel1, class TCollisionModel2, class ResponseDataTypes >
void BarycentricStickContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::setDetectionOutputs(OutputVector* o)
void BarycentricStickContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::doSetDetectionOutputs(OutputVector* o)
{
if (o==nullptr) return;
TOutputVector& outputs = *static_cast<TOutputVector*>(o);
Expand Down Expand Up @@ -184,7 +184,7 @@ void BarycentricStickContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes
}

template < class TCollisionModel1, class TCollisionModel2, class ResponseDataTypes >
void BarycentricStickContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::createResponse(core::objectmodel::BaseContext* group)
void BarycentricStickContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::doCreateResponse(core::objectmodel::BaseContext* group)
{
if (ff!=nullptr)
{
Expand All @@ -203,7 +203,7 @@ void BarycentricStickContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes
}

template < class TCollisionModel1, class TCollisionModel2, class ResponseDataTypes >
void BarycentricStickContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::removeResponse()
void BarycentricStickContact<TCollisionModel1,TCollisionModel2,ResponseDataTypes>::doRemoveResponse()
{
if (ff!=nullptr)
{
Expand Down
19 changes: 13 additions & 6 deletions ...nse/Contact/src/sofa/component/collision/response/contact/BaseUnilateralContactResponse.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,26 @@ class BaseUnilateralContactResponse : public core::collision::Contact, public Co
BaseUnilateralContactResponse(CollisionModel1* model1, CollisionModel2* model2, Intersection* intersectionMethod);

~BaseUnilateralContactResponse() override;

public:
void cleanup() override;

std::pair<core::CollisionModel*,core::CollisionModel*> getCollisionModels() override { return std::make_pair(model1,model2); }
virtual ConstraintParameters getParameterFromDatas() const = 0;

void setDetectionOutputs(OutputVector* outputs) override;
virtual void setupConstraint(MechanicalState1 *,MechanicalState2 *) = 0;

void createResponse(core::objectmodel::BaseContext* group) override;
protected:
std::pair<core::CollisionModel*,core::CollisionModel*> doGetCollisionModels() override
{
return std::make_pair(model1,model2);
}

void removeResponse() override;
void doSetDetectionOutputs(OutputVector* outputs) override;

virtual ConstraintParameters getParameterFromDatas() const = 0;
virtual void setupConstraint(MechanicalState1 *,MechanicalState2 *) = 0;
void doCreateResponse(core::objectmodel::BaseContext* group) override;

void doRemoveResponse() override;

};

} // namespace sofa::component::collision::response::contact
6 changes: 3 additions & 3 deletions ...e/Contact/src/sofa/component/collision/response/contact/BaseUnilateralContactResponse.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ void BaseUnilateralContactResponse<TCollisionModel1, TCollisionModel2, Constrain


template <class TCollisionModel1, class TCollisionModel2, class ConstraintParameters, class ResponseDataTypes >
void BaseUnilateralContactResponse<TCollisionModel1, TCollisionModel2, ConstraintParameters, ResponseDataTypes>::setDetectionOutputs(OutputVector* o)
void BaseUnilateralContactResponse<TCollisionModel1, TCollisionModel2, ConstraintParameters, ResponseDataTypes>::doSetDetectionOutputs(OutputVector* o)
{
TOutputVector& outputs = *static_cast<TOutputVector*>(o);
// We need to remove duplicate contacts
Expand Down Expand Up @@ -198,7 +198,7 @@ void BaseUnilateralContactResponse<TCollisionModel1, TCollisionModel2, Constrain
}

template <class TCollisionModel1, class TCollisionModel2, class ConstraintParameters, class ResponseDataTypes >
void BaseUnilateralContactResponse<TCollisionModel1, TCollisionModel2, ConstraintParameters, ResponseDataTypes>::createResponse(core::objectmodel::BaseContext* group)
void BaseUnilateralContactResponse<TCollisionModel1, TCollisionModel2, ConstraintParameters, ResponseDataTypes>::doCreateResponse(core::objectmodel::BaseContext* group)
{

activateMappers();
Expand Down Expand Up @@ -238,7 +238,7 @@ void BaseUnilateralContactResponse<TCollisionModel1, TCollisionModel2, Constrain
}

template <class TCollisionModel1, class TCollisionModel2, class ConstraintParameters, class ResponseDataTypes >
void BaseUnilateralContactResponse<TCollisionModel1, TCollisionModel2, ConstraintParameters, ResponseDataTypes>::removeResponse()
void BaseUnilateralContactResponse<TCollisionModel1, TCollisionModel2, ConstraintParameters, ResponseDataTypes>::doRemoveResponse()
{
if (m_constraint)
{
Expand Down
21 changes: 15 additions & 6 deletions ...ent/Collision/Response/Contact/src/sofa/component/collision/response/contact/RayContact.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ class SOFA_COMPONENT_COLLISION_RESPONSE_CONTACT_API BaseRayContact : public core
public:
typedef collision::geometry::RayCollisionModel CollisionModel1;

const sofa::type::vector<core::collision::DetectionOutput*>& getDetectionOutputs() const
{
return collisions;
}

protected:
CollisionModel1* model1;
sofa::type::vector<core::collision::DetectionOutput*> collisions;
Expand All @@ -44,14 +49,12 @@ class SOFA_COMPONENT_COLLISION_RESPONSE_CONTACT_API BaseRayContact : public core
BaseRayContact(CollisionModel1* model1, core::collision::Intersection* instersectionMethod);

~BaseRayContact() override;
public:
const sofa::type::vector<core::collision::DetectionOutput*>& getDetectionOutputs() const { return collisions; }

void createResponse(core::objectmodel::BaseContext* /*group*/) override
void doCreateResponse(core::objectmodel::BaseContext* /*group*/) override
{
}

void removeResponse() override
void doRemoveResponse() override
{
}

Expand All @@ -68,13 +71,15 @@ class RayContact : public BaseRayContact
protected:
CollisionModel2* model2;
core::objectmodel::BaseContext* parent;

public:
RayContact(CollisionModel1* model1, CollisionModel2* model2, Intersection* intersectionMethod)
: BaseRayContact(model1, intersectionMethod), model2(model2)
{
}

void setDetectionOutputs(core::collision::DetectionOutputVector* outputs) override
protected:
void doSetDetectionOutputs(core::collision::DetectionOutputVector* outputs) override
{
OutputVector* o = static_cast<OutputVector*>(outputs);
//collisions = outputs;
Expand All @@ -83,7 +88,11 @@ class RayContact : public BaseRayContact
collisions[i] = &(*o)[i];
}

std::pair<core::CollisionModel*,core::CollisionModel*> getCollisionModels() override { return std::make_pair(model1,model2); }
std::pair<core::CollisionModel*,core::CollisionModel*> doGetCollisionModels() override
{
return std::make_pair(model1,model2);
}

};

} //namespace sofa::component::collision::response::contact
12 changes: 8 additions & 4 deletions ...n/Response/Contact/src/sofa/component/collision/response/contact/StickContactConstraint.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,17 @@ class StickContactConstraint : public core::collision::Contact, public ContactId

void cleanup() override;

std::pair<core::CollisionModel*,core::CollisionModel*> getCollisionModels() override { return std::make_pair(model1,model2); }
protected:
std::pair<core::CollisionModel*,core::CollisionModel*> doGetCollisionModels() override
{
return std::make_pair(model1,model2);
}

void setDetectionOutputs(OutputVector* outputs) override;
void doSetDetectionOutputs(OutputVector* outputs) override;

void createResponse(core::objectmodel::BaseContext* group) override;
void doCreateResponse(core::objectmodel::BaseContext* group) override;

void removeResponse() override;
void doRemoveResponse() override;
};


Expand Down
6 changes: 3 additions & 3 deletions ...Response/Contact/src/sofa/component/collision/response/contact/StickContactConstraint.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ void StickContactConstraint<TCollisionModel1,TCollisionModel2>::cleanup()


template < class TCollisionModel1, class TCollisionModel2 >
void StickContactConstraint<TCollisionModel1,TCollisionModel2>::setDetectionOutputs(OutputVector* o)
void StickContactConstraint<TCollisionModel1,TCollisionModel2>::doSetDetectionOutputs(OutputVector* o)
{
this->f_printLog.setValue(true);
msg_info() << "setDetectionOutputs(" << (o == nullptr ? -1 : (int)static_cast<TOutputVector*>(o)->size()) << ")";
Expand Down Expand Up @@ -178,7 +178,7 @@ void StickContactConstraint<TCollisionModel1,TCollisionModel2>::activateMappers(
}

template < class TCollisionModel1, class TCollisionModel2 >
void StickContactConstraint<TCollisionModel1,TCollisionModel2>::createResponse(core::objectmodel::BaseContext* group)
void StickContactConstraint<TCollisionModel1,TCollisionModel2>::doCreateResponse(core::objectmodel::BaseContext* group)
{
msg_info() << "->createResponse(" << group->getName() << ")";
if (!contacts.empty() || !keepAlive())
Expand Down Expand Up @@ -217,7 +217,7 @@ void StickContactConstraint<TCollisionModel1,TCollisionModel2>::createResponse(c
}

template < class TCollisionModel1, class TCollisionModel2 >
void StickContactConstraint<TCollisionModel1,TCollisionModel2>::removeResponse()
void StickContactConstraint<TCollisionModel1,TCollisionModel2>::doRemoveResponse()
{
msg_info() << "->removeResponse()";
if (m_constraint)
Expand Down
74 changes: 68 additions & 6 deletions Sofa/framework/Core/src/sofa/core/collision/Contact.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,65 @@ class SOFA_CORE_API Contact : public virtual objectmodel::BaseObject
Contact& operator=(const Contact& n) = delete;

public:
/// Get the pair of collision models which are in contact
virtual std::pair< core::CollisionModel*, core::CollisionModel* > getCollisionModels() = 0;
/**
* !!! WARNING since v25.12 !!!
*
* The template method pattern has been applied to this part of the API.
* This method calls the newly introduced method "doGetCollisionModels" internally,
* which is the method to override from now on.
*
* Get the pair of collision models which are in contact
*
**/
virtual std::pair< core::CollisionModel*, core::CollisionModel* > getCollisionModels() final
{
//TODO (SPRINT SED 2025): Component state mechamism
return this->doGetCollisionModels();
};

/// Set the generic description of a contact point
virtual void setDetectionOutputs(DetectionOutputVector* outputs) = 0;
/**
* !!! WARNING since v25.12 !!!
*
* The template method pattern has been applied to this part of the API.
* This method calls the newly introduced method "doSetDetectionOuputs" internally,
* which is the method to override from now on.
*
* Set the generic description of a contact point
*
**/
virtual void setDetectionOutputs(DetectionOutputVector* outputs) final
{
//TODO (SPRINT SED 2025): Component state mechamism
this->doSetDetectionOutputs(outputs);
};

virtual void createResponse(objectmodel::BaseContext* group) = 0;
/**
* !!! WARNING since v25.12 !!!
*
* The template method pattern has been applied to this part of the API.
* This method calls the newly introduced method "doCreateResponse" internally,
* which is the method to override from now on.
*
**/
virtual void createResponse(objectmodel::BaseContext* group) final
Copy link
Contributor

@damienmarchal damienmarchal May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure it makes sens to move from absrtact method to a virtual final. Shouldn't it be a non virtual one ? (same comment apply everywhere)

Copy link
Author

@NiziL NiziL May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm going to talk about this to the team during the daily meeting, might apply to all the SED Sprint PRs

Copy link
Author

@NiziL NiziL May 14, 2025

Choose a reason for hiding this comment

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

@damienmarchal Talked about it with the team, for us it's important to declare them as virtual final to ensure child class cannot shadow them.

Copy link
Contributor

@bakpaul bakpaul May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I confirm, putting virtual final will make sure no one override it

Copy link
Contributor

@alxbilger alxbilger May 14, 2025

Choose a reason for hiding this comment

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

@damienmarchal's point is pertinent but the context must be taken into account:
The function createResponse was virtual. There are chances that someone overloaded this function in a derived class. And there are two cases:

  1. The overload has the override keyword:
    • if the base class function is virtual final, it will trigger a compilation error because of a final method cannot be overloaded.
    • If the base class function is a non-virtual function, it will also trigger a compilation error because there is nothing to override.
  2. The overload has not the override keyword:
    • if the base class function is virtual final, it will trigger a compilation error because it tries to override a final function.
    • If the base class function is a non-virtual function, the function in the derived class will be valid. And that's not what we want. We want to notice the derived class that something changed and the code must be adapted. This case will not notice the developer of the derived class, and will introduce a bug.

Because of that, virtual final is not silly. However, this case should be rare because the override keyword should be widely used. So going with a non-virtual function should not be risky, and be the objective.

{
//TODO (SPRINT SED 2025): Component state mechamism
this->doCreateResponse(group);
};

virtual void removeResponse() = 0;
/**
* !!! WARNING since v25.12 !!!
*
* The template method pattern has been applied to this part of the API.
* This method calls the newly introduced method "doRemoveResponse" internally,
* which is the method to override from now on.
*
**/
virtual void removeResponse() final
{
//TODO (SPRINT SED 2025): Component state mechamism
this->doRemoveResponse();
};

/// Return true if this contact should be kept alive, even if objects are no longer in collision
virtual bool keepAlive() { return false; }
Expand Down Expand Up @@ -108,5 +158,17 @@ class SOFA_CORE_API Contact : public virtual objectmodel::BaseObject
return sofa::core::objectmodel::New<RealContact>(model1, model2, inter);
}

protected:
/// Get the pair of collision models which are in contact
virtual std::pair< core::CollisionModel*, core::CollisionModel* > doGetCollisionModels() = 0;

/// Set the generic description of a contact point
virtual void doSetDetectionOutputs(DetectionOutputVector* outputs) = 0;

virtual void doCreateResponse(objectmodel::BaseContext* group) = 0;

virtual void doRemoveResponse() = 0;


};
} // namespace sofa::core::collision
15 changes: 8 additions & 7 deletions applications/plugins/PersistentContact/src/PersistentContact/PersistentFrictionContact.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,20 +133,21 @@ class PersistentFrictionContact : public response::contact::FrictionContact<TCol

void cleanup();

/// Set the generic description of a contact point
void setDetectionOutputs(OutputVector* outputs);

void createResponse(core::objectmodel::BaseContext* group);

virtual void removeResponse();

void init();


#ifdef DEBUG_INACTIVE_CONTACTS
void draw(const core::visual::VisualParams* vparams);
#endif

protected:

/// Set the generic description of a contact point
void doSetDetectionOutputs(OutputVector* outputs) override;

void doCreateResponse(core::objectmodel::BaseContext* group) override;

void doRemoveResponse() override;

std::pair<bool,bool> findMappingOrUseMapper();

Expand Down
Loading
Loading