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

Revised planner profile interfaces #267

Conversation

marip8
Copy link
Contributor

@marip8 marip8 commented Jan 12, 2023

This PR provides new interfaces for motion planner waypoint, composite, and planner level profiles. Each profile produces a type-erased, planner-specific data object from a relevant input (an instruction for the waypoint profile, a composite instruction for the composite profile). The individual planners can look up profiles in the profile dictionary by a combination of namespace and name, and can cast them to the required type. Since the objects returned by the profiles are std::any type-erasers, any issues with incorrect casts will result in an exception that can be handled.

The addition of new planner profile interfaces supports an on-going larger refactor and simplification of the various motion planners class (started in #138). Additionally, the new profiles will be easier to store and retrieve in the profile dictionary since they will no longer be stored by class typeid.

This PR adds three new maps in the profile dictionary to hold these waypoints. Currently these maps are public members of the profile dictionary for convenience and to minimize API breakage. As motion planners are refactored to use these profile interfaces, the planners will retrieve profiles from these maps. Ultimately, the map storing profiles by typeid will be removed, the three maps holding the new profiles will be made private, and the existing profile dictionary API will be revised to access the new profile maps.

Replaces #156

@Levi-Armstrong
Copy link
Contributor

LGTM, feel free to merge when you are ready.

@marip8
Copy link
Contributor Author

marip8 commented Jan 13, 2023

What did you have to do on the master branch to prevent the Bionic build from failing? It complains about not knowing about a scoped_lock, but the builds aren't failing on the master branch. I also noticed that the last PR we merged into this branch also did not have a successful Bionic build for the same reason

@Levi-Armstrong
Copy link
Contributor

Maybe we just need to rebase this on master

@Levi-Armstrong
Copy link
Contributor

I rebased feat/ProfileRefactor on master. Rebase yours on feat/ProfileRefactor again and lets see if the CI issue goes away. If not after that then I would say just let it go for now and we track down the issue later since Focal is passing.

@marip8 marip8 force-pushed the feature/new-planner-profiles-v2 branch from 4cd7f79 to a97babc Compare January 16, 2023 15:59
@marip8
Copy link
Contributor Author

marip8 commented Jan 16, 2023

I think the rebase of #255 and feat/ProfileRefactor onto the current master messed up some of the unit tests since you also changed them in the recently merged PRs. These should be fixed now

We should also seriously consider merging #255 and the last two commits here (a40f272 and bcc8f8e) into the master branch. These examples were failing because profiles for the specified profile names of "FREESPACE" and "UPRIGHT" were never actually added to the profile dictionary, and it was silently using the default profiles.

@codecov
Copy link

codecov bot commented Jan 16, 2023

Codecov Report

❗ No coverage uploaded for pull request base (feat/ProfileRefactor@fafbd46). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                   @@
##             feat/ProfileRefactor     #267   +/-   ##
=======================================================
  Coverage                        ?   58.71%           
=======================================================
  Files                           ?      215           
  Lines                           ?     9723           
  Branches                        ?        0           
=======================================================
  Hits                            ?     5709           
  Misses                          ?     4014           
  Partials                        ?        0           

@marip8 marip8 merged commit 2f56986 into tesseract-robotics:feat/ProfileRefactor Jan 16, 2023
@marip8 marip8 deleted the feature/new-planner-profiles-v2 branch January 16, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants