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

[Mass,MechanicalLoad] Use enumeration to clarify the code #4918

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

[Mass,MechanicalLoad] Use enumeration to clarify the code #4918

wants to merge 33 commits into from

Conversation

hugtalbot
Copy link
Contributor

@hugtalbot hugtalbot commented Aug 13, 2024 •
edited
Loading

Based on #3927
Needs #4917 to make all tests pass successfully.

This PR aims at :

  • Use enumeration to clarify the code for initialization methods in ConstantForceField and UniformMass
  • some code cleanings : starting init as Valid, avoid calling isComponentValid while init(), check isComponentValid prior to run-time functions

Diff : hugtalbot/sofa@b7c7293...hugtalbot:9371ffc1731840b2d100eb7deaa754775c98c6c7

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

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

hugtalbot and others added 23 commits April 22, 2024 13:54
@hugtalbot @fredroy
[Mass] Replace doUpdateInternal by callback: UniformMass
8daef1a
@hugtalbot @fredroy
fix initial view in example scene using InteractiveCamera
96908f7
@hugtalbot @fredroy
add call to componentState in accFromF and buildMassMatrix
a0be3f2
@hugtalbot @fredroy
Apply suggestions from code review
572579a
@alxbilger @fredroy
Use function isComponentStateValid
afd56eb
@alxbilger @fredroy
remove empty override
a700c53
@alxbilger @fredroy
Fix typo in comment
c55918f
@alxbilger @fredroy
remove useless comment
bdcfe71
@hugtalbot @fredroy
take reviews into account: separate callbacks, add boolean switch
9efc3ff
@hugtalbot @fredroy
minor clean
a56d0ea
@hugtalbot @fredroy
additional changes in UniformMass
8cd9231
@hugtalbot @fredroy
update associated test
23ffce1
@hugtalbot @fredroy
Work on init and callback to get a clean state after init, fix test
0126565
@hugtalbot @fredroy
Fix tests attempt 1
01955c4
@hugtalbot @fredroy
fix createRegularGridScene using UniformMass
3e2c984
@hugtalbot @fredroy
fix scene without specifying a mass
102aeaa
@hugtalbot @fredroy
Remove uses of mass data replaced with vertexMass
4301271
@hugtalbot @fredroy
Update Sofa/Component/Mass/src/sofa/component/mass/UniformMass.inl
b20193f
@hugtalbot
merge with master 20240812
cc557ed
@hugtalbot
minor change on ConstantFF comment and config.h.in
f6ff564
@hugtalbot
remove reinit, add comments and fix typo in UM
f72c957
@hugtalbot
simplify code regarding SingleStateAccessor
e4fc67f
@hugtalbot
[Mass,MechanicalLoad] Use enumeration to clarify the code
3b1cc06
@hugtalbot hugtalbot added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request pr: based on previous PR PR based on a previous PR, therefore to be merged ONLY subsequently labels Aug 13, 2024
@fredroy
Copy link
Contributor

fredroy commented Aug 13, 2024

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

@hugtalbot hugtalbot requested a review from fredroy August 13, 2024 13:46
@hugtalbot
Copy link
Contributor Author

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

hugtalbot
hugtalbot commented Aug 14, 2024
View reviewed changes
Sofa/Component/MechanicalLoad/src/sofa/component/mechanicalload/ConstantForceField.inl
Comment on lines +161 to +163
/// Trigger callbacks to update data (see constructor)
if(!this->isComponentStateValid())
msg_error() << "Initialization process is invalid";
Copy link
Contributor Author

@hugtalbot hugtalbot Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This change makes all tests using the ConstantForceField fail due to a bug in SingleStateAccessor solved in #4917 . To solve the tests, merge #4917 first and rebase. Then, the PR should be fine

@hugtalbot hugtalbot requested a review from fredroy August 14, 2024 20:31
@fredroy
Copy link
Contributor

fredroy commented Aug 19, 2024

This change makes all tests using the ConstantForceField fail due to a bug in SingleStateAccessor solved in #4917 . To solve the tests, merge #4917 first and rebase. Then, the PR should be fine

#4917 has been integrated (not rebased due to anterior conflicts) and there are still errors because of
No input force has been set. Please define one of both Data: forces or totalForce 🥲

I guess there is an error of logics (if, else, etc) somewhere

@bakpaul bakpaul added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bakpaul bakpaul bakpaul left review comments

@fredroy fredroy Awaiting requested review from fredroy

Assignees
No one assigned
Labels
enhancement About a possible enhancement pr: based on previous PR PR based on a previous PR, therefore to be merged ONLY subsequently pr: status wip Development in the pull-request is still in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hugtalbot @fredroy @bakpaul @alxbilger