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

Add PoseStampedArray #262

Merged
merged 6 commits into from
Jan 7, 2025
Merged

Conversation

tonynajjar
Copy link
Contributor

The need originated from here. @SteveMacenski

Copy link
Member

@mjcarroll mjcarroll left a 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.

@SteveMacenski
Copy link
Contributor

I think the global message stamp could be removed?

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Dec 3, 2024

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
Same concept no?

@SteveMacenski
Copy link
Contributor

SteveMacenski commented Dec 3, 2024

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 PoseStampedArray have a similar need? I'm ambivalent, just couldn't think of a reason why we needed it for PoseStampedArray. I think to Michael's point, there's no objections, but if included we should explain how it should be used so putting that reason inline would resolve his concern and provide clarity. If we can't think of a reason, then maybe just remove it

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Dec 3, 2024

Okay. I also don't see a reason to keep it but I wanted to stay consistent. Your explanation as to why nav_msgs/Path would have a header makes sense. I removed the header

@tonynajjar
Copy link
Contributor Author

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

@tonynajjar tonynajjar force-pushed the add-PoseStampedArray branch from 6d6b803 to 5b43387 Compare December 5, 2024 14:19
@SteveMacenski
Copy link
Contributor

@mjcarroll I think this is in the final form with the header description. Do we need another maintainer to review or can we ship?

tonynajjar and others added 6 commits December 5, 2024 17:02
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]>
@tonynajjar
Copy link
Contributor Author

tonynajjar commented Dec 9, 2024

Thanks for the review @clalancette , changes were made, anything else?

@SteveMacenski
Copy link
Contributor

We're also planning on using this for the NavigateToPose interface - which makes alot more sense than our vector<PoseStamped> for a list of goal representation so we can have a stamp to gauge the age of the original request. We're finding more places to use this by the day :-)

@tonynajjar
Copy link
Contributor Author

We're also planning on using this for the NavigateToPose interface

NavigateThroughPoses*

@tonynajjar
Copy link
Contributor Author

Happy new year! (ping in disguise @clalancette 😄 )

@mjcarroll
Copy link
Member

Pulls: #262
Gist: https://gist.githubusercontent.com/mjcarroll/e51db32e9e99205be4fa32319946c28b/raw/5e9a971bdd5ed5eb905513bd6477c1a698030d59/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15039

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@SteveMacenski
Copy link
Contributor

@mjcarroll they've all passed :-)

@ahcorde ahcorde merged commit 0cf96ab into ros2:rolling Jan 7, 2025
3 checks passed
@gavanderhoorn
Copy link

gavanderhoorn commented Jan 7, 2025

It's surprising to see this merged, especially after the many discussions about extending geometry_msgs with more messages with low semantic value (of which the linked ros/common_msgs#144 is an example).

What changed? Different maintainers, changed opinions, evolution of best-practices? All of the above?

@Swepz
Copy link

Swepz commented Jan 13, 2025

Friendly reminder to bump tag :)
image

@tfoote
Copy link
Contributor

tfoote commented Jan 30, 2025

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.

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.

8 participants