Skip to content

Conversation

@fredroy
Copy link
Contributor

@fredroy fredroy commented Dec 23, 2025

Based on

Add the ability to have multiple collision pipeline in a scene, instead of one monolithic pipeline (presented at the STC20)
This allows a lot of things:

  • group collision model with {alarm,contact} distances (can mitigate some tunneling-effect artifacts during collisions)
  • different response for a group of models (not tested)
  • mix different type of intersections
  • easily add custom pipelines
  • maybe more

It adds a warning if no collision model are handled by any pipeline.

This PR also adds a not-an-alias for CollisionPipeline similar to BruteForceDetection, but in this case I would not deprecate it and would be more seen as a convenience for most users, as the whole Multi + Sub collision pipeline is quite verbose in the end. And the former CollisionPipeline was doing a lot of things implicitly.

EDIT: with this, PipelineImpl is not used anymore (and has ... a dubious implementation by the way)

[with-all-tests]


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).

@fredroy fredroy added pr: status to review To notify reviewers to review this pull-request pr: new feature Implement a new feature pr: highlighted in next release Highlight this contribution in the notes of the upcoming release topic for next dev-meeting PR to be discussed in sofa-dev meeting labels Dec 23, 2025
@fredroy
Copy link
Contributor Author

fredroy commented Dec 24, 2025

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

@hugtalbot hugtalbot added this to the v26.06 milestone Jan 5, 2026
Comment on lines 70 to 75
m_subCollisionPipeline = sofa::core::objectmodel::New<SubCollisionPipeline>();
m_subCollisionPipeline->d_depth.setParent(&this->d_depth);
m_subCollisionPipeline->l_broadPhaseDetection.set(this->broadPhaseDetection);
m_subCollisionPipeline->l_narrowPhaseDetection.set(this->narrowPhaseDetection);
m_multiCollisionPipeline = sofa::core::objectmodel::New<MultiCollisionPipeline>();
m_multiCollisionPipeline->l_subCollisionPipelines.add(m_subCollisionPipeline.get());
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure to exactly get what you intend to do here. It seems like you are expecting that if a CollisionPipeline is in the scene, then you expect to find the rest of the components and then you internally create a MultisubCollision + SubCollision. But what if we create another SubcollisionPipeline in the scene ? How t link it to the one created automatically ?

If it is for compat purpose, why don't you just keep the old behavior ? Why forcing to instantiate a multiCollisionPipeline inside ? I suppose that it is to avoid duplicating code. Wouldn't it be cleaner to make CollisionPipeline inherits from SubCollisionPipeline ? Or at least add some static methods that does the job ?

{
if (pipelineCollisionModels.find(cm) == pipelineCollisionModels.end())
{
msg_warning() << "CollisionModel " << cm->getName() << " is not handled by any SubCollisionPipeline.";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add, regarding my first comment, that this message will become very confusing for used it the old CollisionPipeline uses a multi one inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

YEah, but the message will be prefixed with "[WARNING] [MultiCollisionPipeline(unnamed)]" which will be confusing for someone using an oold scene that doesn't contain any MultiCollisionPipeline.

Anyway, I really think that the original CollisionPipeline is just a special case of SubCollisionPipeline that is alone and doesn't belong to any multi one. The inheritance seems more appropriate for me.

subPipeline->computeCollisionReset();
}

// re-order pipelines by order of distance
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially it was in the case of if two sub collision pipelines were testing on the same collision models, to avoid doing twice/thrice whatever the same collision tests. But not sure if useful actually

@hugtalbot hugtalbot removed the topic for next dev-meeting PR to be discussed in sofa-dev meeting label Jan 13, 2026
namespace sofa::component::collision::detection::algorithm
{

class SOFA_COMPONENT_COLLISION_DETECTION_ALGORITHM_API AbstractSubCollisionPipeline : public sofa::core::objectmodel::BaseObject
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to document this class? Also the main virtual functions. Thanks

this->d_componentState.setValue(sofa::core::objectmodel::ComponentState::Valid);
}

doInit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call doInit when validity==false?

Copy link
Contributor Author

@fredroy fredroy Jan 22, 2026

Choose a reason for hiding this comment

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

A bit of a stretch but maybe the (in)-validity could be solved in the inheriting class 🤔

Comment on lines 77 to 78
this->addSlave(m_subCollisionPipeline);
this->addSlave(m_multiCollisionPipeline);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm that they should be slaves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are supposed to be totally controlled by the "master" (aka MultiCollisionPipeline) and thus should be not be used by the common visitor and such

@fredroy fredroy removed the pr: status to review To notify reviewers to review this pull-request label Jan 22, 2026
@fredroy fredroy added the pr: status wip Development in the pull-request is still in progress label Jan 22, 2026
@fredroy fredroy force-pushed the add_multi_pipeline branch from 9518785 to add0b08 Compare January 22, 2026 04:08
@fredroy
Copy link
Contributor Author

fredroy commented Jan 22, 2026

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

@fredroy fredroy added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Jan 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: highlighted in next release Highlight this contribution in the notes of the upcoming release pr: new feature Implement a new feature pr: status to review To notify reviewers to review this pull-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants