-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Feat/smac planner include orientation flexibility #4127
base: main
Are you sure you want to change the base?
Feat/smac planner include orientation flexibility #4127
Conversation
@stevedanomodolor what's the status here - do you want me to review in detail, have gaps that are TODO, or have some questions to discuss? I don't want to go through and nitpick some small issues if you're really looking for feedback elsewhere right now. |
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 didn't review the analytic expansions yet, pending your answer to my above question. But overall from what I read so far, very few notes. This is very good and I couldn't have done it better myself!
If you consider the general approach to be ok, then you can review it in detail. If the approach is good, what is just left is to modify the test to take into consideration these changes hench the todo in the CMakelists. |
I think the analytic expansions might need to be rethought a bit. I think we should be taking all
I think your logic is that if we sort by heuristic, then the first that comes back as a valid expansion will be the shortest. I think that would generally be true if the heuristic was a very purist implementation of a distance heuristic. But instead, we have the maximum of a few heuristics including cost information so the "closest" and the one with the "lowest travel cost" aren't necessarily the same thing. So I think largely these changes should be taken back to square one unfortunately and loop to find each of the So after
You can use that best_score, store it for that particular angle to decide which to use. |
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 ran out of time this evening to review, but this is a few things -- also you have a number of linting issues I can see. Check CI for the full list of formatting problems
Its also ready enough to update docs for the new variable for the mode to describe the mode, and the migration guide update to show this feature. An image/gif of this in action with the different modes would be great! I looked through it and all looks good except the analytic expansions I didn't get to right now |
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.
Looks good!
I think you are missing some work for the distance heuristic.
The distance heuristic is pre computed based on free space. We current only calculate it for the the first goal. This means that we could artificially inflate the cost to go making the heuristic inadmissible.
My suggestion would be to remove the distance heuristic when we are in ALL_DIRECTION
mode. For the BIDIRECTIONAL
mode I would pre compute the distance for both angles and take the min of those two.
From what I have seen the distance heuristic is rarely greater than the obstacle heuristic so you probably haven't seen any issues.
Any questions or anything I can unblock on? 😄 |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
2 similar comments
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
No blocking points, just making changes based on @jwallace42 feedback and testing them but after merging to the main and pulling the latest dockers, pr is not building. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
1 similar comment
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
Yeah there's a transient issue due to Rolling moving to 24.04 so a bunch of tooling and package indices are being messed with. Don't worry about it, its not your fault as long as it works locally. Just make sure to keep up on unit testing. Want me to take a look again? |
I will take advantage of the time to add more unit testing after making the modification you suggested. After the added unit tests, you can look into it. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
9 similar comments
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. |
Signed-off-by: stevedan <[email protected]>
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.
Overall looks really good to me, I have a couple of small changes and after that the next review I'll go into detail line by line to make sure we didn't miss anything subtle, but its largely code I've already reviewed pre-coarse search and approved, so its more just for my piece of mind (and don't expect to find any issues).
Please pull in the updates from the main branch (some smac optimizations were made) and benchmark against your changes with main pulled in and the new main itself. If they are similar/same/better, then this is good to go!
_goal_heading_mode = fromStringToGH(goal_heading_type); | ||
|
||
nav2_util::declare_parameter_if_not_declared( | ||
node, name + ".coarse_search_resolution", rclcpp::ParameterValue(4)); |
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.
for both planners: check that this is an exact increment of the lattice or hybrid primitive set (i.e. equally divisible so that we don't end up with like prim_set.size() / coarse_resolution = 2.3535. It should be 2 or 3). Otherwise, in the code, we need to explicitly set a policy for handling this. overall though, I think its a misconfiguration if someone selects something non-exact, so throwing and failing to configure / do a param update seems OK
Also for the lattice planner: I think this should default to the lattice's size and not do coarse resolution (so default = 1 whereas hybrid = 4). The doc PR should be updated accordingly
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.
What of the situations where some goals are invalid because they are in a collision and we prune them earlier? This would not be valid right?. Currently, I divide with the valid number of goals(to make the search as equally distributed as possible) and not the whole angular binprepareGoalsForExpansion).
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 the behavior should be that the goals_to_expand
is based on the even-distribution of primitives. If a primitive at a position was pruned due to collision, then it should be skipped, not replaced with a nearby one.
That way, which of the primitives used is deterministic, regardless of collision state of the goal, which may change based on request. We always use the 'every-Nth' the user sets, but if its in collision, then its just not attempted.
In setGoals
the _goals_vector
should be the full set. Its pruned in areInputsValid
before we currently do the prepareGoalsForExpansion
.
To resolve that, I think (a) the preparation of goals to expand should be done before the areInputsValid
method is called (probably not good), (b) do the preparation inside of areInputsValid
's loop since we're already in a for loop for _goals_set
which could be changed to _goals_vector
, or (c) we have an _raw_goals_vector
and _goals_vector
to store the goals separately unpruned to use in the coarse preparation.
Separately, I'm thinking maybe we should do this a little more clearly while we're making this change.
Having prepareGoalsForExpansion
return coarse_search_goal_size
to denote where the coarse then fine search nodes are separated could be improved. I think either:
- We should return 2 vectors: the coarse list and the fine list. Coarse used when coarse searching. Coarse+fine used for fine searching (though I think coarse is already exhausted from your current implementation, so functionally just the 'fine' list).
- We should have the vector stored object have a field for indicating coarse or fine that can be referenced in the iteration for coarse serach and skip if its a fine-search node. Then we only pass into
tryAnalyticExpansion
the vector and change thewhile
condition into a for-each andcontinue;
if the goal is fine. This would be a new struct containing the node + the coarse/fine state. I think the 2x vectors in my first point is subjectively better, but I'm pretty ambivalent as to which.
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.
Besides the first point, I only use the goals vector for the coarse expansion actually because I needed a vector that stored the goals in order. I will go with C but with two vectors, one that stores the raw goals vector and another vector of boolean that stores if a goal is valid or not. I can use this info for the preparation of the coarse and fine list in the next stage, what do you think?
Besides the second point, I think the first option works fine for me, clearer to understand, to return two vector.
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.
If you're going to go that route, it shouldn't be 2 separate vectors, but one vector with a struct containing the goal & the collision state. 2 vectors that 'zip' together is not very clear that they're associated. Structs make sense because the collision state is an attribute of the goal, so they should be bundled together.
struct GoalState
{
NodePtr goal; // <-- or whatever we need to store here, I think this is right though
bool is_valid = true; // default to valid
}
typedef GoalStates std::vector<Goal>;
Then in the areInputsValid
, we set the is_valid state to false instead of deleting the goals vector entry.
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 love this approach.
Signed-off-by: stevedan <[email protected]>
…e file Signed-off-by: stevedan <[email protected]>
_goal_heading_mode = fromStringToGH(goal_heading_type); | ||
|
||
nav2_util::declare_parameter_if_not_declared( | ||
node, name + ".coarse_search_resolution", rclcpp::ParameterValue(4)); |
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 the behavior should be that the goals_to_expand
is based on the even-distribution of primitives. If a primitive at a position was pruned due to collision, then it should be skipped, not replaced with a nearby one.
That way, which of the primitives used is deterministic, regardless of collision state of the goal, which may change based on request. We always use the 'every-Nth' the user sets, but if its in collision, then its just not attempted.
In setGoals
the _goals_vector
should be the full set. Its pruned in areInputsValid
before we currently do the prepareGoalsForExpansion
.
To resolve that, I think (a) the preparation of goals to expand should be done before the areInputsValid
method is called (probably not good), (b) do the preparation inside of areInputsValid
's loop since we're already in a for loop for _goals_set
which could be changed to _goals_vector
, or (c) we have an _raw_goals_vector
and _goals_vector
to store the goals separately unpruned to use in the coarse preparation.
Separately, I'm thinking maybe we should do this a little more clearly while we're making this change.
Having prepareGoalsForExpansion
return coarse_search_goal_size
to denote where the coarse then fine search nodes are separated could be improved. I think either:
- We should return 2 vectors: the coarse list and the fine list. Coarse used when coarse searching. Coarse+fine used for fine searching (though I think coarse is already exhausted from your current implementation, so functionally just the 'fine' list).
- We should have the vector stored object have a field for indicating coarse or fine that can be referenced in the iteration for coarse serach and skip if its a fine-search node. Then we only pass into
tryAnalyticExpansion
the vector and change thewhile
condition into a for-each andcontinue;
if the goal is fine. This would be a new struct containing the node + the coarse/fine state. I think the 2x vectors in my first point is subjectively better, but I'm pretty ambivalent as to which.
Signed-off-by: stevedan <[email protected]>
node->get_parameter(name + ".goal_heading_mode", goal_heading_type); | ||
_goal_heading_mode = fromStringToGH(goal_heading_type); | ||
|
||
nav2_util::declare_parameter_if_not_declared( |
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.
Default = 1 for lattice so that we do all resolution
_goal_heading_mode = fromStringToGH(goal_heading_type); | ||
|
||
nav2_util::declare_parameter_if_not_declared( | ||
node, name + ".coarse_search_resolution", rclcpp::ParameterValue(4)); |
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.
If you're going to go that route, it shouldn't be 2 separate vectors, but one vector with a struct containing the goal & the collision state. 2 vectors that 'zip' together is not very clear that they're associated. Structs make sense because the collision state is an attribute of the goal, so they should be bundled together.
struct GoalState
{
NodePtr goal; // <-- or whatever we need to store here, I think this is right though
bool is_valid = true; // default to valid
}
typedef GoalStates std::vector<Goal>;
Then in the areInputsValid
, we set the is_valid state to false instead of deleting the goals vector entry.
nav2_smac_planner/src/a_star.cpp
Outdated
if (_coarse_search_resolution <= 0) { | ||
throw nav2_core::PlannerException("Invalid coarse search resolution, Cannot be <= 0"); | ||
} | ||
else if(goal_heading_mode != GoalHeadingMode::ALL_DIRECTION) { |
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.
the }
and else if
should be on the same line
current_best_goal = current_goal_node; | ||
current_best_node = node; | ||
found_valid_expansion = true; | ||
break; |
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.
Note: the break here then wouldn't attempt all coarse resolutions, it ends early. We'd need to iterate through the full set so we actually get the "best" one -- since in the 'fine search' we don't reattempt any of the coarse resolutions. They're simply skipped and could actually be better than the first one found.
Also, I think this should be updated to store the current best if its better than the last score. refineAnalyticPath
should be responsible for attempting to refine the path and then returning the best score and its nodes only. Instead, it should have a return of best_score
that we compare to our current best score and only update current_best_*
if its an improvement. We do that in the fine search but not sure why we don't do that here as well.
const NodeGetter & getter, | ||
NodePtr & node, | ||
AnalyticExpansionNodes & analytic_nodes, | ||
float & best_score |
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.
To that end, best_score
should be a return type (it doesn't need to have input argument at all)
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Signed-off-by: stevedan <[email protected]>
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
How to run
Future work that may be required in bullet points
For Maintainers: