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 costs to path #187

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Conversation

jwallace42
Copy link

Hello,

I have been doing some work on the nav2 stack @SteveMacenski and I wanted to add in a cost attribute to the Path.msg.

This change will help ros-navigation/navigation2#2880.

Cheers,

Josh

@jwallace42 jwallace42 closed this Apr 6, 2022
@jwallace42 jwallace42 reopened this Apr 6, 2022
@@ -5,3 +5,4 @@ std_msgs/Header header

# Array of poses to follow.
geometry_msgs/PoseStamped[] poses
int8[] costs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float32, not ints!

@tfoote
Copy link
Contributor

tfoote commented Apr 7, 2022

Following an offline discussion with @SteveMacenski I just wanted to capture some planned followups for this.

To make the case for updating this message we'll need the following:

  • The updated data needs to stand on it's own semantically, with a clear type and be well documented. Potentially even having a data structure to capture different types of costs/scoring. Such that the message data will stand on it's own without knowing the context in which it was generated. Aka introspecting a rosbag recorded on a robot by someone else at another time.
  • An analysis of the impact on current users. Paying attention to overhead, as well as any required usage changes and/or migration guide. As well as fallback behaviors for unset values.
  • And a functional demonstration of using the new variation to make sure that we are confident that it will work for the designed use case.

@SteveMacenski
Copy link
Contributor

SteveMacenski commented Apr 7, 2022

We'll circle back in the coming weeks on this.

The only mention worth of note is that since this message is so central to Nav2, it is impossible for us to merge in a solution that would depreciate this message in order to show case a demonstration of its use in the main branches. To temporarily replace nav_msgs/Path in rolling with a nav2_msgs/Path would instantly break hundreds of users application-level software that relies on Nav2, and only for a short while during the process of these changes rae being analyzed.

The hot-swap of a new nav_msgs/Path would not have this effect since we would not make any API breaking changes to existing users' use of this message: simply adding a new field.

So I suggest the following: We will create a practical and necessary application of the changes we will later propose to add costs to the Path message, but it will have to be reviewed as a pull-request that would be merged in tandem with the merging of the message modification. I'll wait to poke back on this until we have not only the PR but an approved PR that is ready to be staged to production, so there is no misunderstanding that they are immediate and necessarily tied PRs (e.g. so there's no delay in changes here resulting in an instance of use of it). This will make sure that (1) you can see a use case of it for review, (2) immediately once this is merged, that will also be merged to use it and (3) we break as few people's systems as possible.

Does that seem fair / doable?

@tfoote
Copy link
Contributor

tfoote commented Apr 8, 2022

Yes, the demonstrated use of the enhanced message would not need to have been merged to allow review of the message. It can still be staged on the branch or branches ready for a merge once this is approved and merged. And hopefully with this being an extension only it can be managed to land in a way that does not break systems in the intermediate state either. But we should be giving high visibility warnings of the changes when it is landing if there's any potential breakage on rolling.

@tfoote
Copy link
Contributor

tfoote commented May 5, 2022

Since this is still under iteration I'm going to move this to draft status so that it doesn't appear on our triage lists.

@tfoote tfoote marked this pull request as draft May 5, 2022 17:07
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:17
@jwallace42
Copy link
Author

We have completed a use case for the message change here. ros-navigation/navigation2#3100.

@jwallace42 jwallace42 marked this pull request as ready for review September 26, 2022 16:27
Signed-off-by: Joshua Wallace <[email protected]>
Signed-off-by: Joshua Wallace <[email protected]>
Signed-off-by: Joshua Wallace <[email protected]>
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.

3 participants