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

[Core] Convert warning to error in object factory #3404

Closed alxbilger closed 1 year ago

alxbilger commented 1 year ago

I converted the warning to an error in the following code of the ObjectFactor:

        if (isUserTemplateNameInTemplateList)
        {
            msg_warning(object.get()) << "Requested template '" << usertemplatename << "' "
                                      << "is not compatible with the current context. "
                                      << "Falling back to the first compatible template: '"
                                      << object->getTemplateName() << "'.";
        }
        else
        {
            msg_warning(object.get()) << "Requested template '" << usertemplatename << "' "
                                      << "cannot be found in the list of available templates [" << ss.str() << "]. "
                                      << "Falling back to default template: '"
                                      << object->getTemplateName() << "'.";
        }

To me, this situation is to be taken into account more seriously than a warning.

As failing template deduction is now an error, it leads to a lot of failing tests. I fixed them as well.

I added unit tests for OglModel. I factorize the parse code from the 3 masses.


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

damienmarchal commented 1 year ago

Looks ok to me.

EDIT: @alxbilger want to remove the new errors.

alxbilger commented 1 year ago

[ci-build][with-scene-tests]

alxbilger commented 1 year ago

@fredroy Was there a mechanism to support DiagonalMass template="Vec3d"? Or does it only fall back to DiagonalMass template="Vec3d,Vec3d" because it is the default template?

fredroy commented 1 year ago

@fredroy Was there a mechanism to support DiagonalMass template="Vec3d"? Or does it only fall back to DiagonalMass template="Vec3d,Vec3d" because it is the default template?

Nope, there is a check to see if the 2nd template is the previous MassType (removed) but there is no mechanism to implicitly suppose the geometry type to be like the data type of the mass (which is like 99.999% of the time actually) Do you think it would be a good idea to do it ? (aka if no 2nd template, then implictely suppose it is the same as the the 1st one)

alxbilger commented 1 year ago

@fredroy I prefer not adding another implicit complex mechanism for templates. If there are two templates, the user must set 2 templates. If the user set only one, there is an error message containing the list of all available templates. It should give a hint about the fact that there are two templates to provide.

alxbilger commented 1 year ago

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

sofabot commented 1 year ago

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

To unlock the merge button, you must

sofabot commented 1 year ago

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

To unlock the merge button, you must

sofabot commented 1 year ago

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

To unlock the merge button, you must

alxbilger commented 1 year ago

Some image plugin scenes fail because the template used is not compiled in the standard set. It needs the full set. Only the standard set is compiled on the CI.

Decision to take: 1) The failing scenes are ignored on the CI 2) The failing scenes are modified to use a templated available in the standard set 3) The CI compiled the full set

I am in favor of the third option, but we already talked about it in https://github.com/sofa-framework/ci/pull/15. It seems to take too much time at the compilation.

fredroy commented 1 year ago

Some image plugin scenes fail because the template used is not compiled in the standard set. It needs the full set. Only the standard set is compiled on the CI.

Decision to take:

1. The failing scenes are ignored on the CI

2. The failing scenes are modified to use a templated available in the standard set

3. The CI compiled the full set

I am in favor of the third option, but we already talked about it in sofa-framework/ci#15. It seems to take too much time at the compilation.

I think there is a 4th solution, adding the ImageB type in the "standard" set, as it is seems quite important if lots of scenes need it after all

alxbilger commented 1 year ago

@fredroy Excellent suggestion. I'll do that. Thanks

sofabot commented 1 year ago

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

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

sofabot commented 1 year ago

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

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

sofabot commented 1 year ago

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

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

sofabot commented 1 year ago

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

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

alxbilger commented 1 year ago

@fredroy I applied your suggestion, but there are remining failing scenes. I suggest to ignore them on the CI.

sofabot commented 1 year ago

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

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

sofabot commented 1 year ago

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

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

alxbilger commented 1 year ago

@hugtalbot Can you remind me why you switch this pull request back to wip?

fredroy commented 1 year ago

If I remember, just because there are conflicts to solve

sofabot commented 1 year ago

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

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

alxbilger commented 1 year ago

[ci-build][with-scene-tests]

sofabot commented 1 year ago

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

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

alxbilger commented 1 year ago

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

fredroy commented 1 year ago

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

fredroy commented 1 year ago

Conflicts were solved, tests are OK, and it has been long enough this PR is opened;ready to merge