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
Modify model appending algorithm #2218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @florent-lamiraux for working on this feature.
I made some remarks to enhance the integration of this PR into Pinocchio.
@jcarpent : I will handle your comments after fixing the tests. |
Modify robot append algorithm. In the new version, when appending model B to model A on frame F, joints are copied in the following order - joints of model A up to the parent of F, - all the descendents of parent of F, - the remaining joints of A. Modify test-cpp-model to check the new behavior. Improve documentation of Model.
I addressed the above comments, modified test-cpp-model to test the new behavior, and improved the documentation |
{ | ||
AJointsAfterB.push_back(jid); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the compact organization of the list of joints, I would expect that you can copy all the joints of model A until the first joint whose id id_insert
matches:
id_frame_parent < id_insert
: the joint is after theframe.parent
where modelB is appended.modelA.parents[id_insert] < id_frame_parent
: the joint parent is beforeframe.parent
.
id_insert
is to be added after model B.
If that is true, the code would arguably be easier to read because more atomic (e.g. I would not have to check the doc of model.support
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I'm not specifically asking you to change. I'm done with the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge this PR as it seems fine, even if it can be improved. Let's postpone it as a future work.
This modification follows #2203.
In the new version, when appending model B to model A on frame F, joints are copied in the
following order