New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BREAKING][SofaBaseMechanics][SofaMiscForceField] Homogeneization of mass components in SOFA #637

Merged
merged 43 commits into from Jul 16, 2018

Conversation

@hugtalbot
Copy link
Contributor

hugtalbot commented Apr 13, 2018

The purpose of this PR is to unify the mass components in SOFA (UniformMass, DiagonalMass and MeshMatrixMass):

  • unify the naming of data for mass information (vertexMass, massDensity and totalMass)
  • unify their initialization process
  • apply the coding rules (especially regarding the variable prefix d_ and m_) and use the msg API
  • add the feature of dataTracker on the data for mass information
  • add support for additional CUDA templates for the MeshMatrixMass (that should always be preferred as default mass)

This should ease the understanding of new users at first look.
All tests and checks added should also make these codes more robust.

A script is added with this PR to automatically update all your scene files (.xml and .scn).

I invite to be really prudent before merging this PR, since it affect basically all simulations.

Note: @hdeling I removed unused functions (numericalIntegrationOrder, numericalIntegrationMethod, integrationMethod) in MeshMatrixMass that I think were used before the "plugin.HighOrder" plugin was created. Can you confirm this?

Suggestion for future work:

  • rename MeshMatrixMass into SparseMass
  • move MeshMatrixMass with DiagonalMass
  • make DiagonalMass depends from MeshMatrixMass (since it's a simplified -lumped- derived approach from MeshMatrixMass) : this would clearly factorize the code and make it more robust

PS : bisous


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

hugtalbot added some commits Jan 21, 2018

CHANGE dataname from mass to vertexMass in UniformMass
The data mass was used to describe the single mass value of
each particle or vertex of the object. For a better understanding
and an unification of syntax between mass component, this is now
called vertexMass.
CHANGE initialization process and datanames in UniformMass
Unify the data names :
- vertexMass corresponds to a single real value defining
the mass set at each vertex (previously called mass)
- totalMass corresponds to the total mass of the object
(previously called totalmass)

Implement a clean and rigorous initialization process:
first the vertexMass is checked. If set by the user, it
will be used for initialization and totalMass is computed
from it.

If vertexMass is undefined or wrongly set, the totalMass
will be used. If it is set by the user, the totalMass will
be used for initialization and vertexMass is computed from it.
If totalMass is undefined or wrongly set, the default value
of totalMass = 1.0 is used, and vertexMass is computed
accordingly.

New tests added and file for rigid case updated with the
new name vertexMass instead of mass.
Unify variable name in UniformMass (mass becomes vertexMass)
Unify the variable name :
- d_vertexMass corresponds to a single real value defining
the mass set at each vertex (previously called d_mass)
Unify data name in UniformMass (mass becomes vertexMass)
Unify the data name :
mass becomes vertexMass, this will affect all scenes and tests
Unify the initialization process in DiagonalMass
Implement a clean and rigorous initialization process:
first the vertexMass is checked. If set by the user, it
will be used for initialization, massDensity and totalMass
are computed from it.

Second, the massDensity is checked. If set by the user, it
will be used for initialization, vertexMass and totalMass are
computed from it.

If other infos are undefined or wrongly set, the totalMass
will be used. If it is set by the user, the totalMass will
be used for initialization and vertexMass is computed from it.
If totalMass is undefined or wrongly set, the default value
of totalMass = 1.0 is used, and vertexMass is computed
accordingly.

New tests added for different initialization cases
Clean the variable names and comments in MeshMatrixMass
Update the variable names according to coding rules
Implement reinit for update, handleEvent and fix msg API
Now reinit update all mass info if value modified by the user
The handleEvent function now checks if value is dirty to make a reinit
The message API is now updated
New mechanism of init and update of mass information Uniform-MeshMatr…
…ixMass

- Back to the original vesion of Mass.h without any definition of data
- The mass information (vertexMass, massDensity and totalMass) is back
into the components : Uniform, Diagonal and MeshMatrixMass
- Now use an homogeneous and rigourous initiatlization, check if dirty
and update process for mass information keeping all consistent
- use DataTracker for checking if dirty
Clean and Fix in MeshMatrixMass
- remove data made for integration for Bezier now in a plugin
- fix initiation of topological information : pb of indice
- add get functions for mass information
- improve messaging
UPDATE DiagonalMass with the same process for initialization
As for MeshMatrixMass the process is unified :
- initTopologyHandlers
- massInitialization:
  - all get and set functions for the mass informations
  - all check and initialization process for each data

hugtalbot added some commits May 24, 2018

FIX test and compilation for Rigid case in UniformMass
This commit fixes the test damping.py in SofaTest, failing before
due to the missing computation of recalc() for inertiaMatrix

The commit also simplifies the code by refactoring the init
for Rigid and other cases
@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented May 24, 2018

Note for @sofa-framework/reviewers
The PR now builds and tests run.
This PR might be really breaking, and your careful review and feedback is more than welcome. I am available for any question.

@hugtalbot hugtalbot self-assigned this May 24, 2018

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented May 25, 2018

Hello Hugo,

GG for this PR.
As you announced it, this PR is severaly breaking...either at API level as well as in the user interface (as it breaks users scenes). You provide a script to update scene but it is only covering .scn (not .pyscn or .psl) so to me we need other ways to warns user how to manually update their scene. In the past we were overriding the 'parse' method for each object that have data field's name change and found the solution very nice as it 1) warn user to update their scene, 2) provide a minimal compatibility layer if they don't.

Example of what we did in OglLabel:

    void OglLabel::parse(BaseObjectDescription *arg)
{
    // BACKWARD COMPATIBILITY April 2017
    const char* value = arg->getAttribute("color") ;
    if(value==nullptr || strcmp(value, "contrast")){
        VisualModel::parse(arg);
        return ;
    }

    arg->setAttribute("selectContrastingColor", "true");
    arg->removeAttribute("color") ;

    VisualModel::parse(arg);

    /// A send the message after the parsing of the base class so that the "name" of the component
    /// is correctly reported in the message.
    msg_deprecated() << "Attribute color='contrast' is deprecated since Sofa 17.06.  " << msgendl
                     << "Using deprecated attributes may result in lower performance or un-expected behaviors" << msgendl
                     << "To remove this message you need to update your scene by replacing color='contrast' with "
                        " selectConstrastingColor='true'" ;
}

Now about the API breaks....I will try your branch on top of our plugins to see if it compile or not.

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented May 25, 2018

@olivier-goury and @EulalieCoevoet this PR can be severly breaking so don't hesitate to review it and give feedback to Hugo.

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented May 28, 2018

@damienmarchal I just did the change for warning when using old attribute names are used. Let me know how it works for you guys

/// @name Read and write access functions in mass information
/// @{
void setMass(const MassType& d_vertexMass);
const MassType& getMass() const { return d_vertexMass.getValue(); }

This comment has been minimized.

@EulalieCoevoet

EulalieCoevoet May 29, 2018

Contributor

I would change getMass() for getVertexMass() for more consistency.

This comment has been minimized.

@EulalieCoevoet

EulalieCoevoet May 29, 2018

Contributor

Nice changes by the way! :)

This comment has been minimized.

@hugtalbot

hugtalbot May 29, 2018

Author Contributor

@EulalieCoevoet Ok, why not, however since getMass() is a function inherited from BaseContext, maybe a good point would be to keep the getMass() function that would call the getVertexMass() (few classes use this call to getMass from BaseContext : e.g. CenterOfMassMapping)

This comment has been minimized.

@hugtalbot

hugtalbot May 29, 2018

Author Contributor

see my last commit

Update the getMass function into getVertexMass
For more coherency, the getMass function (inherited from BaseObject)
has been renamed into getVertexMass. However, since the function
getMass is inherited from BaseObject, the function is kept and
redirect to getVertexMass
@guparan

This comment has been minimized.

Copy link
Member

guparan commented Jun 6, 2018

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

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Jun 7, 2018

Ok so it seems to be ready, let's just wait the feedback from @damienmarchal regarding the current incompatibility problem at Defrost.

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Jun 7, 2018

Hi Hugo,

The incompatibility I was talking about few days ago were because of issimo changes integrated in master.
So I don't think there is a need to wait for merge.

DM.

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Jul 12, 2018

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

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Jul 16, 2018

any can merge ? CI looks fine

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Jul 16, 2018

Get ready for the troubles ? Fire on 10 mintues :=)

@damienmarchal damienmarchal merged commit 68cc242 into sofa-framework:master Jul 16, 2018

11 of 14 checks passed

Jk2/ubuntu_clang-3.8_options Build queued.
Details
Jk2/ubuntu_gcc-5.4_default Build queued.
Details
mac_clang-3.4_options Build queued.
Details
Dashboard Builds triggered.
Details
Jk2/Dashboard Builds triggered.
Details
Jk2/Scene tests Triggered in latest build.
Details
Jk2/centos_clang-3.4_options Build OK. FIXME: 1 unit tests, 0 scene tests
Details
Jk2/mac_clang-3.5_options Build OK. FIXME: 52 unit tests, 13 scene tests
Details
Jk2/ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit tests, 1 scene tests
Details
Jk2/windows7_VS-2015_options_amd64 Build OK. FIXME: 0 unit tests, 0 scene tests
Details
Scene tests Triggered in latest build.
Details
centos_clang-3.4_options OK (tests ignored, see details)
Details
ubuntu_gcc-5.4_options OK (tests ignored, see details)
Details
windows7_VS-2015_options_amd64 OK (tests ignored, see details)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment