personalrobotics / aikido

Artificial Intelligence for Kinematics, Dynamics, and Optimization

https://personalrobotics.github.io/aikido/

BSD 3-Clause "New" or "Revised" License

213 stars 30 forks source link

Simplify Robot API #593

Closed aditya-vk closed 2 years ago

aditya-vk commented 3 years ago

This PR simplifies the API to construct and plan with new robots. This changes the software design as well as moves a significant amount of code to setup robots from librobots to aikido in a general fashion. Link to design document..

The code has been tested downstream with modified libherb/herb3_demos when registering subrobots with a factory pattern. The WIP is to replace with templates.

Before creating a pull request

[x] Document new methods and classes
[x] Format code with make format

Before merging a pull request

[x] Set version target by selecting a milestone on the right side
[x] Summarize this change in CHANGELOG.md
[ ] Add unit test(s) for this change

aditya-vk commented 3 years ago

Assigning to @egordon as parts of this PR need to be modified (or even a complete rewrite might be warranted) to reflect the requirements from food manipulation project. @egordon please feel free to assign to someone else if needed? I might not have cycles to see this through. Thank you!

egordon commented 2 years ago

Note: needed a DART version update for cloneMetaskeleton. This version should still be well behind for both bionic and focal.

Should be ready for a review concurrent with me adding this interface to libada.

Some issues that should be addressed in later PRs:

Why the distinction between the DART planners and the non-DART planners? In general, I think the planning interface could be simplified.
Ensure the TrajectoryExecutor joints actually match the managed degrees of freedom in the Robot's Metaskeleton. This fix can be implemented fairly easily after merging the Executor overhaul in https://github.com/personalrobotics/aikido/pull/602
In the same vein, we should add the ability to register arbitrary executors. And RosRobot should gain the ability to manage controllers.

codecov[bot] commented 2 years ago

Codecov Report

Merging #593 (9f794e3) into master (856b60b) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 9f794e3 differs from pull request most recent head 456bc95. Consider uploading reports for the commit 456bc95 to get more accurate results
@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
Coverage   76.38%   76.38%           
=======================================
Files         200      200           
Lines        7030     7030           
=======================================
Hits         5370     5370           
Misses       1660     1660           

Impacted Files	Coverage Δ
...rc/constraint/dart/InverseKinematicsSampleable.cpp	`85.26% <ø> (ø)`

amalnanavati commented 2 years ago

Does this need a corresponding libada PR? Libada currently uses ConcreteRobot, so as soon as this code gets merged in, libada will break.

amalnanavati commented 2 years ago

Does this need a corresponding libada PR? Libada currently uses ConcreteRobot, so as soon as this code gets merged in, libada will break.

EDIT: NVM, I see the corresponding libada issue: https://github.com/personalrobotics/libada/pull/64

amalnanavati commented 2 years ago

Looked at the new commits, and they look good