search

sofa-framework / sofa

Real-time multi-physics simulation with an emphasis on medical simulation.
https://www.sofa-framework.org
GNU Lesser General Public License v2.1
923 stars 311 forks source link

Replace getMeshTopologyLink into getMeshTopology #1297

Open hugtalbot opened 4 years ago

hugtalbot commented 4 years ago

Two interface for both static and dynamic topologies but they are identitical !

  1. Looking for removing all getMeshTopologyLink with getMeshTopology
  2. Then remove getMeshTopologyLink safely

Suggested labels:

jnbrunet commented 4 years ago

Just in case someone have doubts:

BaseContext.cpp (line 172)

/// Mesh Topology (unified interface for both static and dynamic topologies)
core::topology::BaseMeshTopology* BaseContext::getMeshTopology(SearchDirection dir) const
{
    return this->get<sofa::core::topology::BaseMeshTopology>(dir);
}

core::topology::BaseMeshTopology* BaseContext::getMeshTopologyLink(SearchDirection dir) const
{
    return this->get<sofa::core::topology::BaseMeshTopology>(dir);
}
epernod commented 4 years ago

uh? not sure to understand this issue... GetMeshTopologyLink have been introduce to remove progressively GetMeshTopology hidden mechanism. See #744

jnbrunet commented 4 years ago

Hey @epernod ,

Look at both methods, they are exactly the same. In #744 , we are talking about how such methods should not be used, which I agree completely. But here, getMeshTopologyLink is not giving us an answer to this, it is simply an alias to getMeshTopology, which is quite confusing as we can find both of them in the source code, and it seems they are used for the same exact reason, find the first mesh topology in the current context (which is probably wrong since we can have more than one in the context).

epernod commented 4 years ago

yes, this is the work started in the other issue:

  1. Depreciate all getXXXMeshTopology for 19.12
  2. Use explicit SingleLink
  3. if easy will see to automatically add a SingleLink each time a getXXXMeshTopology is used for the in-between period.

2 and 3 have been done in most components through several PR. 1. is a little late in the planning... The idea was to keep GetMeshTopology for unset case but as a last solution, and not accessible through the API.

jnbrunet commented 4 years ago

I understand completely.

Here, we are not talking about bringing back getMeshTopology. We are saying that both getMeshTopologyLink and getMeshTopology are the same.

Following #744, both should be removed. Here we are simply talking about removing the duplicated one, even if at some point it should be also be removed and replaced by either a link or a new getTopologies() (plural) method that makes it clear we can have more than one topology in the context.

epernod commented 4 years ago

ok sorry, got it.

Yes I have duplicated this method in #1223. In the idea of deleting both.

The idea was to make a smooth transition period:

I let that unfinished. So as you want either we put the deprecated message or we replace directly both methods by a new one like "GetFirstEncounteredTopology" inside a breaking PR.

epernod commented 4 years ago

@jnbrunet @hugtalbot could we agree on the name of the method? I will have some time by the end of the month to work on that!

hugtalbot commented 4 years ago

alright, let's discuss this on Wed (it's a bit more than just a choice of name)

fredroy commented 3 years ago

It seems that this issue is stalled since then.