-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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]>
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]>
) Add header data to goal for short paths. Commit d8ae3c1 added the possibility to the rotation shim controller to rotate towards the goal when the goal was closer that the `forward_sampling_distance`. This feature was not fully working as the goal was missing proper header data, causing the rotation shim to give back control to the main controller. Co-authored-by: agennart <[email protected]>
* Added load and save panel Signed-off-by: Alberto Tudela <[email protected]> * Improved dock_panel state machine Signed-off-by: Alberto Tudela <[email protected]> * Added loading dock plugins log Signed-off-by: Alberto Tudela <[email protected]> * Redo UI Signed-off-by: Alberto Tudela <[email protected]> * Update tooltips Signed-off-by: Alberto Tudela <[email protected]> * Fix null-dereference Signed-off-by: Alberto Tudela <[email protected]> --------- Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Daniil Khaninaev <[email protected]>
…4724) Add frame_id to goal when rotating towards goal heading, otherwise the transform would fail. This bug was introduced in 30e2cde by not setting the frame_id. Signed-off-by: agennart <[email protected]> Co-authored-by: agennart <[email protected]>
…on_r1803809978 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]>
…ithub.com:stevedanomodolor/navigation2 into feat/smac_planner_include_orientation_flexibility
Signed-off-by: stevedan <[email protected]>
But if you use a single goal, its not necessarily representative of randomness. If you have the same random seed at the start, then it should be deterministic.
I guess this is where I'd handwave that the heuristic is an estimate, not an exact science - but this is a good point and a reason that if we don't see performance degradation we could do the for loop over each of the goal orientations for the distance heuristic, we can/should do that (which also simplifies code changes, which makes me happy 😄 ). Why is that one
It would be good to understand this to know that's the reason with some confidence. I don't disagree that could be a future improvement, but I'd like for us to at least understand if this is a correct assumption (and if not, maybe the issue is somewhere we can fix easily) |
I dont think it is though because i did two tests.
I think the problem is we are trying to expand to multiple goals, we are jumping from 1 goals to the worst case 72 different goals(if we have 72 bins).
I think i can do just that, i will time the functions in the planning loop to see where rhe bottleneck is. |
Yeah that sounds good to me. I generally agree that this is probably the issue, I just want to know for sure before we assume too much :-) I'm trying to think of some strategies to speed this up (if this is the case): Check every- That way, normal searches that have no expanions possible completes |
The Hybrid-A* can have 64, 72, or other total bins it needs to check. The state lattice planner typically has ~16 since the primitives have to create a regularized lattice structure. You could do more, but typically 16 is sufficient. If you reduced Hybrid-A* to 16, do you see a similar result as the state lattice planner? |
To summarize our call: We should do a course-to-fine search rather than going for all N (72) bins. We could introduce a new parameter for the analytic expansion that is the coarse search resolution (default: 4). If we find any valid path in that coarse search, then we do the full resolution. If we make it default to 4, it should have similar characteristics to the state lattice performance. This should be relatively easy to implement using a queue and a while loop to replace the for loop, where it is initialized with every :-) |
yes, get the same result |
Oh great - that means what we discussed will almost definitely improve performance to that level! |
I tried the improvement, and it did reduce the overall time. 🎉 Now, I am testing another idea to further reduce the time for the distance heuristic. What do you think: The main issue with the current method of storing a loopup table that contains the minimum values for all directions is that the chosen minimum goal might still end up in collision. In addition during the isInputValid check, we discard goals that are in collision. As a result, the "minimum" value stored may not be valid. I propose the following, the first lookup table stores the distance as is now. In addition to this, i create another lookup table that for each position (x, y, θ) to (x,y,theta) i store the index of the position of the loop up table but sorted from minimum to maximum distance. During the isInputValid step, I create a boolean list that marks which goals are invalid (i.e., in collision) and gives this information to the distance heuristic. Using the Second Table, when evaluating the getDistance heuristic, we iterate through the indices in the second lookup table for the given (x, y, θ). We pick the first index corresponding to a goal that is not marked as invalid in the boolean list. I tried before to create another lookup table during the is input valid, but its takes a long time to do that loop. |
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: