[Collision] Introduce multi-staged collision pipeline #5841
Conversation
fredroy
added
pr: status to review
fredroy
force-pushed
the
add_multi_pipeline
branch
from
9d2bf0b to
bbabba1
Compare
|
[ci-build][with-all-tests] |
fredroy
force-pushed
the
add_multi_pipeline
branch
from
bbabba1 to
fed32d2
Compare
| 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()); |
There was a problem hiding this comment.
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 ?
...ection/Algorithm/src/sofa/component/collision/detection/algorithm/MultiCollisionPipeline.cpp
Show resolved
Hide resolved
| { | ||
| if (pipelineCollisionModels.find(cm) == pipelineCollisionModels.end()) | ||
| { | ||
| msg_warning() << "CollisionModel " << cm->getName() << " is not handled by any SubCollisionPipeline."; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Nope, as the CollisionPipeline will fetch all collision models, and set in the link.
https://github.com/fredroy/sofa/blob/fed32d2df4806edaf37f36fd34e47ad24f6c6472/Sofa/Component/Collision/Detection/Algorithm/src/sofa/component/collision/detection/algorithm/CollisionPipeline.cpp#L89
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
...etection/Algorithm/src/sofa/component/collision/detection/algorithm/MultiCollisionPipeline.h
Outdated
Show resolved
Hide resolved
…t/collision/detection/algorithm/MultiCollisionPipeline.h Co-authored-by: Paul Baksic <30337881+bakpaul@users.noreply.github.com>
3 participants
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:
It adds a warning if no collision model are handled by any pipeline.
This PR also adds a not-an-alias for
CollisionPipelinesimilar toBruteForceDetection, 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.[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