Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SofaMiscFem] Fix FastTetrahedralCorotationalFF topology change #554

Merged
merged 6 commits into from
Jan 22, 2018
Merged

[SofaMiscFem] Fix FastTetrahedralCorotationalFF topology change #554

merged 6 commits into from
Jan 22, 2018

Conversation

epernod
Copy link
Contributor

@epernod epernod commented Jan 3, 2018 •
edited by guparan
Loading

Some changes to be able to handle tetra removal.

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.

@epernod
[SofaMiscFEM] ADD: FEM draw method in FastTetrahedralCorotationalFF, …
db7a8d4 
…same code as in TetrahedralCorotationalFEM
@epernod
[SofaMiscFEM] FIX: in FastTetrahedralCorotationalFF, remove the id st…
83d711f 
…ored in PointInfo and EdgeInfo which refer to their topological element id. Those ids were not updated during topological changes. Use directly the topology arrays.
@epernod
[SofaMiscFEM] FIX: FastTetrahedralCorotationalFF topologyChange. Poin…
d73c7fb 
…Info and EdgeInfo are not well updated during topological changes.
@epernod epernod added the pr: fix Fix a bug label Jan 3, 2018
@epernod epernod added the pr: status wip Development in the pull-request is still in progress label Jan 4, 2018
@epernod
[SofaMiscFEM] UPDATE: FastTetrahedralCorotationalFEM, remove the Edge…
2e80f57 
…RestInformation and PointRestInformation classes as they were only storing a Mat3x3. Use directly PointData and EdgeData templated with Mat3x3.
@epernod
[SofaMiscFEM] ADD: new scene for FastTetrahedralCorotationalFEM, and …
b061a51 
…rename the old one as _validation as it compares to TetrahedralCororationalFEM.
@epernod
Copy link
Contributor Author

epernod commented Jan 4, 2018

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

@epernod epernod added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Jan 4, 2018
@hugtalbot hugtalbot changed the title Fix FastTetrahedralCorotationalFF topology change [SofaMiscFem] Fix FastTetrahedralCorotationalFF topology change Jan 8, 2018
@damienmarchal damienmarchal added this to the v17.12 milestone Jan 10, 2018
@guparan
Copy link
Contributor

guparan commented Jan 14, 2018

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

@damienmarchal
Copy link
Contributor

Currently this PR makes forcefield/fastTetrahedralCorotationalFEM crash one the CI.

damienmarchal
damienmarchal reviewed Jan 17, 2018
View reviewed changes
modules/SofaMiscFem/FastTetrahedralCorotationalForceField.inl
@@ -155,6 +155,11 @@ template <class DataTypes> FastTetrahedralCorotationalForceField<DataTypes>::Fas
, f_youngModulus(initData(&f_youngModulus,(Real)1000.,"youngModulus","Young modulus in Hooke's law"))
, lambda(0)
, mu(0)
, f_drawing(initData(&f_drawing, true, "drawing", " draw the forcefield if true"))
Copy link
Contributor

@damienmarchal damienmarchal Jan 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rise some question to me about the general consistency of drawing in Sofa...but this is far
outside the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the code that is in TetrahedralCorotationalForcefield to be consistent. Is it a good way to do it, this is another discussion...

damienmarchal
damienmarchal reviewed Jan 17, 2018
View reviewed changes
modules/SofaMiscFem/FastTetrahedralCorotationalForceField.inl
@@ -155,6 +155,11 @@ template <class DataTypes> FastTetrahedralCorotationalForceField<DataTypes>::Fas
, f_youngModulus(initData(&f_youngModulus,(Real)1000.,"youngModulus","Young modulus in Hooke's law"))
, lambda(0)
, mu(0)
, f_drawing(initData(&f_drawing, true, "drawing", " draw the forcefield if true"))
, drawColor1(initData(&drawColor1, defaulttype::Vec4f(0.0f, 0.0f, 1.0f, 1.0f), "drawColor1", " draw color for faces 1"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not a vector of color instead of 4 data ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same answer as above.

@epernod
Copy link
Contributor Author

epernod commented Jan 18, 2018

Ok I'll check that crash for next dev meeting.

@guparan
Copy link
Contributor

guparan commented Jan 22, 2018 •
edited
Loading

The scene is not crashing, it timeouts. Obviously lowering the number of CG iterations avoids the timeout but does not solve the problem...
I checked the scene before this PR and it is as slow as after. So why wasn't it timeout-ing before?

@guparan
Copy link
Contributor

guparan commented Jan 22, 2018

Ok found it: the scene-tests are not performing in default mode (100 iterations with a timeout of 60s for a Debug build) because FastTetrahedronCorotationalForceField.scn is listed with custom values in examples/.scene-tests config file.
Renaming the scene according to b061a51 should calm down the CI ;-)

@guparan
[SofaMiscFEM] Propagate renaming in .scene-tests config file
8164efd 
FastTetrahedronCorotationalForceField.scn had custom values for scene testing.
@guparan
Copy link
Contributor

guparan commented Jan 22, 2018 •
edited
Loading

[ci-build][with-scene-tests] (please CI don't die on me)

@guparan guparan added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Jan 22, 2018
@guparan guparan merged commit 2ceee36 into sofa-framework:master Jan 22, 2018
@epernod epernod deleted the fixFastTetrahedralCorotationalFF branch September 20, 2019 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damienmarchal damienmarchal damienmarchal left review comments

@hugtalbot hugtalbot Awaiting requested review from hugtalbot

@hdeling hdeling Awaiting requested review from hdeling

Assignees
No one assigned
Labels
pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Milestone
v17.12
Development

Successfully merging this pull request may close these issues.
3 participants
@epernod @guparan @damienmarchal