Add support for switching between SMAC planners#5840
Add support for switching between SMAC planners#5840SteveMacenski merged 14 commits intoros-navigation:mainfrom
Conversation
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
Opening as draft for visibility, @SteveMacenski can you do a high level review here? I will fix the doxygen, formatting issue later on For reference: Before: After: Switching between SMAC hybrid (dubins) and SMAC hybrid (reeds shepp): Jan.6.2026.Video.mp4 |
|
Hmmm, I reran the planner benchmarks today and don't see any performance difference. I suspect the earlier result was simply because I forgot to rebuild after removing cout 😮💨 Before: SMAC 2D 100by100_15.yaml 100by100_20.yaml SMAC Hybrid 100by100_15.yaml 100by100_20.yaml SMAC Lattice 100by100_15.yaml 100by100_20.yaml After this PR: SMAC 2D 100by100_15.yaml 100by100_20.yaml SMAC Hybrid 100by100_15.yaml 100by100_20.yaml SMAC Lattice 100by100_15.yaml 100by100_20.yaml I also loop this line over 1000 times and the execution time is 0 ms, so I think removing static is fine here |
There was a problem hiding this comment.
This PR is one where I'd love to give you more specific feedback, but unfortunately it is quite a bit involved that I know any specifics I would give you, you'd find a reason why it wasn't possible. This is one of those things you just roll your sleeves up and work it out.
What I would like to see is that the encapsulation I fought so much more in the Smac Planner remains in place. My original thinking (which may or may not be possible) was that we created a grouping of the NodeT static members "Context" within the Node templates. Then we initialize it in the Planner plugins so that the planner plugins host the context. When we replan, we check if the context exists in NodeT and only create it if it does not exist or change it to the hosted-context if the current context is different (maybe have a randomly generated ID in it?). Or I suppose if the static member in NodeT is a pointer, then we can just always assign it since its not involving a copy.
That's not perfect, I know. But, I think it would reduce the complexity of what's going on here and keep the internal details of each node outside of the A* algorithm.
Another direction is to try using -fvisibility=hidden. That may solve the issue without any changes by making the symbols not exported to the dynamic symbols table. If the pluginlib architects were forward thinking, they may have made it so that the main class having public visbility regardless. If not, we may be able to set the public visibility of a class (?) https://stackoverflow.com/questions/3570355/c-fvisibility-hidden-fvisibility-inlines-hidden though I'm not familiar with it.
I spent some time trying this today, but I ended up encountering a lot of linking errors. Unless I am missing something, this will likely more changes
I was moving the two functions to astar for two main reasons:
but I understand your point. We could instead allow write access to the other classes and introduce a common base class, then have the
When doing this, I am trying to get rid of every static member and function, to avoid running into any pluginlib issues that are hard to debug. I think your solution will work as well, if you don't really like my solution above, I can give it a try. Do you have a strong preference between the two? |
Unsurprising, I think that would be something that would take some trial and error to figure out (or finally figure out its simply not possible).
Mhm, interesting. Sure, that would work. What methods would that implement though? Just holding the context or also some of the common A*-based calls? This sounds like a similar solution to having a static context object in each NodeT. Understanding more of your design intent would be good before jumping into that in earnest if you wanted to go this way over the others.
Of the current method (not the 2 above) and my solution, I would prefer my solution. It has less moving parts / easier to review / doesn't bleed concepts from the NodeT's into the A*. |
I think we can have a new class like NodeSE2 {
LookupTable obstacle_heuristic_lookup_table;
ObstacleHeuristicQueue obstacle_heuristic_queue;
std::shared_ptr<nav2_costmap_2d::Costmap2DROS> costmap_ros;
// Dubin / Reeds-Shepp lookup and size for dereferencing
LookupTable dist_heuristic_lookup_table;
float size_lookup;
float getObstacleHeuristic(
const Coordinates & node_coords,
const Coordinates & goal_coords,
const double & cost_penalty,
const bool use_quadratic_cost_penalty,
const bool downsample_obstacle_heuristic);
void resetObstacleHeuristic(
std::shared_ptr<nav2_costmap_2d::Costmap2DROS> costmap_ros,
const unsigned int & start_x, const unsigned int & start_y,
const unsigned int & goal_x, const unsigned int & goal_y,
const bool downsample_obstacle_heuristic)
inline uint64_t getIndex(
const unsigned int & x, const unsigned int & y, const unsigned int & angle);
}then in struct NodeContext: public NodeSE2Perhaps we can also move |
|
My head's not in this problem in the same way as yours working on it -- how is I don't think this handles the static Minor bit: I'd rename this if not dealing with SE2-nature things. This is just the obstacle / distance heuristic bits.
I'd keep the context completely generic, so I'd avoid that. There are some other NodeT's I have on my mind which I would want to use this with that would have different Coordinates / GetIndex implementations. |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
nav2_smac_planner/include/nav2_smac_planner/distance_heuristic.hpp
Outdated
Show resolved
Hide resolved
nav2_smac_planner/include/nav2_smac_planner/obstacle_heuristic.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
Otherwise, I have no more notes - doing perf testing is the last thing |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
For correctness and performance: Before: SMAC 2D 100by100_15.yaml 100by100_20.yaml SMAC Hybrid 100by100_15.yaml 100by100_20.yaml SMAC Lattice 100by100_15.yaml 100by100_20.yaml After this PR: SMAC 2D 100by100_15.yaml 100by100_20.yaml SMAC Hybrid 100by100_15.yaml 100by100_20.yaml SMAC Lattice 100by100_15.yaml 100by100_20.yaml I also added more tests to get > 90% in code coverage today One last thing from my side: would you like to rename "context" in this PR? Do you think something like |
|
Also moving to final review, I think this is close to merge |
SteveMacenski
left a comment
There was a problem hiding this comment.
LGTM
On the metrics, if you run them again for the "after" do they get more in line with the "before"? I see its a little bit slower, but I'm really thinking its probably just variance of running and wanted to just sanity check that you don't see a meaningful change if run a few times (and maybe averaging them out). The ~2-5% difference here isn't nothing, but could also just be from a single run.
| MotionPrimitive * _motion_primitive; | ||
| bool _backwards; | ||
| bool _is_node_valid{false}; | ||
| NodeContext * _ctx; |
There was a problem hiding this comment.
For all: initialize to nullptr. Could save us some hassle for later changes that create bugs with garbage data.
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
Yes, I reran both "before" and "after" a few more times, and the results look quite similar overall. Sometimes the "before" run is faster, sometimes the "after" run is faster |
SteveMacenski
left a comment
There was a problem hiding this comment.
Anything else you think before merging?
|
Nothing from my side. But let me know if you would like to rename "NodeContext" |
|
We can leave it. Can you open a migration guide PR mentioning this fix? Its not a ‘migration’ task but its a good FYI this is fixed |
Ok will do |
* Prototype for fixing multiple planners, all unit tests passed Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Lint Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Fix analytic expansion Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Remove log Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Revert Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Add distance heuristic and obstacle heuristic class Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Drop const and lint Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Apply suggestions from code review Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com> * Move init motion moel back to nodes Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Doxygen Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Add code coverage Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Initialize as nullptr Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> --------- Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com> Co-authored-by: Steve Macenski <stevenmacenski@gmail.com>
* Prototype for fixing multiple planners, all unit tests passed Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Lint Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Fix analytic expansion Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Remove log Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Revert Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Add distance heuristic and obstacle heuristic class Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Drop const and lint Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Apply suggestions from code review Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com> * Move init motion moel back to nodes Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Doxygen Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Add code coverage Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Initialize as nullptr Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> --------- Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com> Co-authored-by: Steve Macenski <stevenmacenski@gmail.com>
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Description of how this change was tested
Future work that may be required in bullet points
For Maintainers:
backport-*.