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
871 stars 297 forks source link

[Sofa.Core] A helper class for consistent component naming #2631

Closed alxbilger closed 2 years ago

alxbilger commented 2 years ago

Naming components was not consistent:

This is now unified using the singleton sofa::core::NameHelper. It applies the XML method: the class name + a counter

[ci-depends-on https://github.com/sofa-framework/SofaPython3/pull/228]

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

sofabot commented 2 years ago

[ci-depends-on] detected during build #1.

To unlock the merge button, you must

alxbilger commented 2 years ago

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

sofabot commented 2 years ago

[ci-depends-on] detected during build #2.

To unlock the merge button, you must

alxbilger commented 2 years ago

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

sofabot commented 2 years ago

[ci-depends-on] detected during build #3.

To unlock the merge button, you must

hugtalbot commented 2 years ago

Link name resolver to the simulation structure (when reloading, same IDs will be kept)

sofabot commented 2 years ago

[ci-depends-on] detected during build #4.

To unlock the merge button, you must

alxbilger commented 2 years ago

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

sofabot commented 2 years ago

[ci-depends-on] detected during build #5.

To unlock the merge button, you must

alxbilger commented 2 years ago

The problem with the approach to have a name helper in each Node is that the Node must exist before using it. And it's not the case in the XML loader. A name is required before the object creation, and no Node is available at that time. Therefore, I used a single name helper inside the XML loading for all the components. In this case, the counter suffix does not depend on the Node. A problem with this approach (other than it's not consistent), Nodes' counters are not updated...

alxbilger commented 2 years ago

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

sofabot commented 2 years ago

[ci-depends-on] detected during build #6.

To unlock the merge button, you must

alxbilger commented 2 years ago

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

sofabot commented 2 years ago

[ci-depends-on] detected during build #7.

To unlock the merge button, you must

sofabot commented 2 years ago

[ci-depends-on] detected during build #8.

To unlock the merge button, you must

alxbilger commented 2 years ago

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

sofabot commented 2 years ago

[ci-depends-on] detected during build #9.

To unlock the merge button, you must

sofabot commented 2 years ago

[ci-depends-on] detected during build #10.

To unlock the merge button, you must

fredroy commented 2 years ago

Everything has been done (from reviews & stuff), I set it to ready and if nobody disagrees, I will merge it at the end of the day.

EulalieCoevoet commented 2 years ago

Hi @fredroy, @damienmarchal, @hugtalbot, @alxbilger,

Can this be reopened for discussion ? My pros for reverting this or keeping the python naming instead of xml:

alxbilger commented 2 years ago

How does it break the scenes? In my opinion, it breaks only if you refer to a component that you did not name, relying only on the component class name. I don't think it's a good practice. If you refer to a component, you have to give it a name. But maybe we missed other cases. Same thing when you rely on a warning message to detect duplicate names. This warning is here only when the user actually provided two equal names. But a user can put two same components in the same Node without naming them and it's a valid situation. In this case, no warning. I am open to discuss about removing the 0 suffix, because in most cases, the component is the only one in its Node. But I would like to keep the following suffixes (1, 2, 3 etc). Note that removing the 0 would not discourage users to rely on the component class name of an unnamed component.

EulalieCoevoet commented 2 years ago

If it's a bad practice to use the default name why giving one? I guess the real cleaning is to remove the default name then. I don't think that the argument "let's put an annoying default name" so that people are discourage to use it is valid.

alxbilger commented 2 years ago

If it's a bad practice to use the default name why giving one? I guess the real cleaning is to remove the default name then.

That's a fair question. To me, it's cosmetic because we display the component name in the tree in the GUI, and for legacy reasons (one of the many implicit mechanism, which may confuse the users).

I don't think that the argument "let's put an annoying default name" so that people are discourage to use it is valid.

I meant if we want to discourage this practice, we probably need to warn the user. Doing nothing will certainly not discourage the practice.

In any case, your issue will be discussed during one of the next dev meetings. Meanwhile, @fredroy @damienmarchal you can put your opinion here. Even if we think it's a bad practice, it does not mean that we will keep this situation as it is. Breaking code without support is not wanted. Thanks for sharing your issue.

fredroy commented 2 years ago

If it's a bad practice to use the default name why giving one? I guess the real cleaning is to remove the default name then.

That's a fair question. To me, it's cosmetic because we display the component name in the tree in the GUI, and for legacy reasons (one of the many implicit mechanism, which may confuse the users).

I don't think that the argument "let's put an annoying default name" so that people are discourage to use it is valid.

I meant if we want to discourage this practice, we probably need to warn the user. Doing nothing will certainly not discourage the practice.

In any case, your issue will be discussed during one of the next dev meetings. Meanwhile, @fredroy @damienmarchal you can put your opinion here. Even if we think it's a bad practice, it does not mean that we will keep this situation as it is. Breaking code without support is not wanted. Thanks for sharing your issue.

We discussed about the goal of this PR quite a lot (it was created quite a long time ago now), and I guess we quite agreed that we should at least fix the lack of consistency in the gen. default name (xml vs python); default names being generated since... the start of SOFA. I try personally to not have any default names in my scenes, especially if I use those components in the scene (Link, Data), as I am a XML old coot (and names are the only way to link stuff). So as @alxbilger has said, those "random names" is (or should be) merely for GUI purposes. And I am not really a python3 dev but I would think that the pydev should always use directly the setPath(),setLinkPath() directly with the the object you intend to link with. In the end, maybe one way to make it a discouraged practice would be to warn the user when a name has been created and that it is not really recommended, like we do with aliases or missing RequiredPlugins. It will be annoying at the beginning but still...

As for the question which convention to use (i.e xml vs python), I think it was just decided orally between devs at a sofa-dev meeting, but I suppose we can change that easily; I said at that time XML just because I have a XML bias, that's all 🧐

EulalieCoevoet commented 2 years ago

I agree that at least we should have a warning.

Just, when you think about reusable code like STLIB, or what we have in SoftRobots too for instance, which is a good practice to avoid dirty copy paste. It means that we should give a name to every component right? Since the user cannot, and may need to refer to any component.