-
Notifications
You must be signed in to change notification settings - Fork 113
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 PoseStampedArray #262
Add PoseStampedArray #262
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.
No arguments from me.
Should we potentially add a comment here about what the difference between the root std_msgs/Header
and the per-message std_msgs/Header
mean here, or is that entirely application specific.
I think the global message stamp could be removed? |
Would you then also remove the header from this one? -> https://github.com/ros2/common_interfaces/blob/rolling/nav_msgs/msg/Path.msg |
The path's header is for the path itself, the pose headers are for each of the poses in the path if it were a time denoted trajectory. Admittedly its rarely (if ever) used that way, but it is conceptually the right way for the path to be structured for kinodynamic paths, should uses need to represent one. Does the application of |
Okay. I also don't see a reason to keep it but I wanted to stay consistent. Your explanation as to why |
While working with this message we actually found a potential use for the global header. I added a comment but see here for more info. Feel free to propose a reformulation |
6d6b803
to
5b43387
Compare
@mjcarroll I think this is in the final form with the header description. Do we need another maintainer to review or can we ship? |
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]> Signed-off-by: Tony Najjar <[email protected]>
This reverts commit c59b96a. Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]> Signed-off-by: Tony Najjar <[email protected]> Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
7639ae6
to
f24cd35
Compare
Thanks for the review @clalancette , changes were made, anything else? |
We're also planning on using this for the NavigateToPose interface - which makes alot more sense than our |
NavigateThroughPoses* |
Happy new year! (ping in disguise @clalancette 😄 ) |
Pulls: #262 |
@mjcarroll they've all passed :-) |
It's surprising to see this merged, especially after the many discussions about extending What changed? Different maintainers, changed opinions, evolution of best-practices? All of the above? |
I'd like to review the naming and documentation of this before we make a release to be sure we've locked down the semantics of this message. I've added it to the PMC meeting agenda for next week. |
The need originated from here. @SteveMacenski