-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add planner name to trajectory info #5
Add planner name to trajectory info #5
Conversation
Codecov Report
@@ Coverage Diff @@
## ros2 #5 +/- ##
==========================================
+ Coverage 41.53% 41.64% +0.12%
==========================================
Files 81 83 +2
Lines 7933 7962 +29
==========================================
+ Hits 3294 3315 +21
- Misses 4639 4647 +8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Why not optionally set (or add to) the comment instead of creating a new field? I did not review the rest of this patch. |
Thanks for your review!
I think it is nicer to have a specifically named field rather than an arbitrary "comment" field for this data
The refactored version of the PipelinePlanner we're using here does not. See PR against main mtc repository for details |
@@ -73,7 +73,13 @@ class Connect : public Connecting | |||
WAYPOINTS = 1 | |||
}; | |||
|
|||
using GroupPlannerVector = std::vector<std::pair<std::string, solvers::PlannerInterfacePtr> >; | |||
struct PlannerIdTrajectoryPair |
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 for doing this!
My comment was less about the struct vs. pair data structure, and the fact that if you ever had to add more data to this it would be challenging. Same still holds here because the struct has Pair
in its name. Is there some more generic name we can use like TrajectoryInfo
?
... and same with the variable names that use this data type, which are still called vector
and pair
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 think we should keep it like this for now because the name describes what it is and if we add more to this data structure in the future the name can be updated accordingly. Despite the fact that this could also be of datatype pair, I think making it a struct made the code more readable 👍
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.
TrajectoryInfo
is very generic and MTC has already a datatype called like this.
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.
Sounds good -- I can't think of anything better anyhow.
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.
LGTM!
@sjahr: I'm going through all commits PickNik committed to the ros2 branch. For this one, could you please argue why this additional field is actually needed? How do you handle generators, which also generate trajectories but do not have a planner necessarily? |
I think it is useful to include information about the planner that created a solution into the solution info. We're using it e.g. for debugging and introspection. I did not think about the generator use-case, but I think in that case the field could be left empty or populated with something like "generated" (depending on how it was actually created). Before the parallel planning API, I guess it was possible to infer the planner that was used from the pipeline id but now, the pipeline planner uses multiple algorithms and without this field it would be harder to say which one actually created the best solution. Just out of curiosity: Are there any trajectory generator stage examples in the mtc main repository? |
Add planner id to SubTrajectory SolutionInfo.