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.

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

Extend iDynTree::ModelLoader::loadReducedModelFromFullModel method to optionally take a given value for lumped models #1174

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

traversaro
Copy link
Member

Fix #495 .

Currently untested, I added functions instead of adding new default values to preserve ABI. Functions can be removed right before a stable release.

@GiulioRomualdi
Copy link
Member

Thank you @traversaro this is great!!

@traversaro
Copy link
Member Author

CI failures are unrelated, I did not tested the functions, but if you want to use the functionalities we can merge and do a release once you validated the function.

@traversaro
Copy link
Member Author

I added tests, this is ready for review.

Copy link
Member

@GiulioRomualdi GiulioRomualdi left a comment

Choose a reason for hiding this comment

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

That's amazing! After this get merged is it possible to release the software so we can use this feature in wbd and in the walking?

bool createReducedModel(const Model& fullModel,
const std::vector<std::string>& jointsInReducedModel,
Model& reducedModel,
const std::map<std::string, double>& removedJointPositions);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why you decided to use a map and not an unordered map?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I am old and I am just instintively tend to use stuff that was available in old versions on C++? Anyhow, let's use unordered, I think it make sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by 36956df, let's see if CI is happy.

@traversaro
Copy link
Member Author

Great, only macos test fail with:

24/68 Test #24: UnitTestModel .........................................Subprocess aborted***Exception:   0.01 sec
Checking simple model... 
Checking random chains...
Checking reduced model for random chain of size: 2
Checking reduced model for random chain of size: 17

valgrind tests on Linux work fine. @GiulioRomualdi realistically I can't debug this until next week as I do not hav a mac. We can disable the tests on mac and release, or start testing the method from this branch, let me know what you prefer.

@traversaro
Copy link
Member Author

Great, only macos test fail with:

24/68 Test #24: UnitTestModel .........................................Subprocess aborted***Exception:   0.01 sec
Checking simple model... 
Checking random chains...
Checking reduced model for random chain of size: 2
Checking reduced model for random chain of size: 17

valgrind tests on Linux work fine. @GiulioRomualdi realistically I can't debug this until next week as I do not hav a mac. We can disable the tests on mac and release, or start testing the method from this branch, let me know what you prefer.

Note that I noticed that no one was calling the checkReducedModel test and I added it in tests now, so I guess that could also be a macos failure that was always there, and nothing something introduced by this PR.

@traversaro
Copy link
Member Author

Note that I noticed that no one was calling the checkReducedModel test and I added it in tests now, so I guess that could also be a macos failure that was always there, and nothing something introduced by this PR.

Actually not: #1176 .

@traversaro
Copy link
Member Author

Great, only macos test fail with:

24/68 Test #24: UnitTestModel .........................................Subprocess aborted***Exception:   0.01 sec
Checking simple model... 
Checking random chains...
Checking reduced model for random chain of size: 2
Checking reduced model for random chain of size: 17

valgrind tests on Linux work fine. @GiulioRomualdi realistically I can't debug this until next week as I do not hav a mac. We can disable the tests on mac and release, or start testing the method from this branch, let me know what you prefer.

There was a bug in the test function, that was setting some position value even if the joint had 0 DOF, I fixed it with the check in https://github.com/robotology/idyntree/pull/1174/files#diff-f8a68ac51cda85238110c1087da7b9715dd2e0d44e16913fd07217c8c43d24e8R106 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants