-
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
Selector plugin for selecting the planner and the controller #4091
Selector plugin for selecting the planner and the controller #4091
Conversation
Ah right forgot about signing the commits.. It's been bit long since I gave in a PR. Also just remembered that I also need to add the defaults in all our default BTs. Will update that too.. |
@padhupradheep, your PR has failed to build. Please check CI outputs and resolve issues. |
7c2dbce
to
c31fed7
Compare
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.
Generally looks good to me! What's this tool look like? 😄
nav2_bt_navigator/behavior_trees/navigate_to_pose_w_replanning_and_recovery.xml
Outdated
Show resolved
Hide resolved
Also, seems easy enough to add the smoother / goal checker / progress checkers here too! Nice, simple tool, I like it! |
Video demo:
Shall I add those too? |
Please! |
6209c9f
to
1e77c77
Compare
Progress checker too ❤️ I think then its just the BT updates and perhaps this is good for testing / merging? What do you think? |
There is no BT node for that as far as I checked. We can use the dynamic reconf server for it but that would be something unique and really won't fit to the scope of this PR.
Yes, almost done! I need to update the docs about the updates in the BT and probably you can give it a swing when you get some time and we are all set for the merge once after that. |
Oh huh, that must be an oversight. OK. Yes, please update the docs to match! |
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.
Pending docs!
eeb7dc3
to
1a3fc5c
Compare
Shall I add this plugin as default in nav2 bringup ? Since it's more of a development tool, I feel it would be better if the user selects it by hand when required. |
Yeah that sounds great! Though not in the same spot / scale as your gif. Above or below the Nav2 plugin with similar button spacing / scale as that |
Yeah that new 2x2 version works, though why not have the drop down on the same line as the label instead of a new line? Either way works. I leave it to your discretion / eye If you're going to make a new gif with the format, try to use not just |
Just leaving a short note on having the selector plugin has default in the rviz bringup. There is some sort of race condition that does not allow the selector plugin to populate atleast one of the component's combo boxes, atleast once in ten instances. |
Do you see your error log about the server not being up yet? Can you change the It would be good to know where this is happening so we can see what might need to be retried / delayed to get it to work deterministically. 1/10 failures is not something we could merge :( For example, you could have that timer trigger repeatedly at your 200ms (I assume that 200 is ms) until it works properly and log an error each time it fails to get the parameters and waiting for the servers to be ready. Don't cancel until successful. That wouldn't delay rviz bringup either! A-OK on the layout, it looks good! |
Issue is fixed! Thanks for the suggestion! 👍 |
CI had error I assume isn't related to this PR, but retriggering to check. In the meantime, anything else you want to do before I test and merge? |
6f9a40f
to
605e465
Compare
Well, I just added the progress_checker too. I just updated from 2x2 to 3x2 configuration, with slight changes to the way combo boxes would be placed. Now I think, it's ready to be tested. |
Huh, your PR is regularly triggering these failures. Can you reproduce locally? Maybe try pulling in |
I already did a rebase early today.. not sure what's causing the issue. Let me check.. |
Beyond that, I tested it locally and looks good to me |
My mistake, somehow messed up the commit while fixing the DCO. I just was following the recommendation it was giving out, but somehow it broke my changes 🤷♂️ |
Now it seems to be the |
4bcfb51
to
f1150e6
Compare
I was messing around there recently, but it all passed on my CI jobs and I saw that main was working too after merge. Maybe increment your cache to v20 in the 3x places in the circleci file to break it all? If that still fails, I'll take that as an action item to look into, I won't block you further with it. |
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
Signed-off-by: PRP <[email protected]>
f1150e6
to
a21be57
Compare
v20 didn't help either.. please let me know, if there is something else that I can try to sort it outt. |
Not your problem, probably me. If you wanted to take a look at it, the more the merrier 😉 Want to post on discourse with your new feature + gif? I think that's worthy of highlighting! |
😄
Oh! Sure can do that.. 👍 |
It took me one whole year to eventually get on this PR and get it up.. 😵💫 I'll clear #3347 too.. If you have anything else, feel free to tag me in and I promise it would be faster this time around. |
Thanks and will do! |
…igation#4091) * initial commit Signed-off-by: PRP <[email protected]> * now dynamic reconfigurability also be achieved Signed-off-by: PRP <[email protected]> * updating the changes Signed-off-by: PRP <[email protected]> * fixing the mistake Signed-off-by: PRP <[email protected]> * adding default values for the selector Signed-off-by: PRP <[email protected]> * updating the plugin description Signed-off-by: PRP <[email protected]> * setting default in combox and removing after selected Signed-off-by: PRP <[email protected]> * minor Signed-off-by: PRP <[email protected]> * adding goal checker and smoother selectors to the plugins Signed-off-by: PRP <[email protected]> * updating all the relevant BTs Signed-off-by: PRP <[email protected]> * updating to 2x2 version Signed-off-by: PRP <[email protected]> * updating the bringup too Signed-off-by: PRP <[email protected]> * fixing the race condition Signed-off-by: PRP <[email protected]> * adding progress checker to the selector plugin Signed-off-by: PRP <[email protected]> * fixing CI Signed-off-by: PRP <[email protected]> * updating the cache version Signed-off-by: PRP <[email protected]> --------- Signed-off-by: PRP <[email protected]> Signed-off-by: enricosutera <[email protected]>
…igation#4091) * initial commit Signed-off-by: PRP <[email protected]> * now dynamic reconfigurability also be achieved Signed-off-by: PRP <[email protected]> * updating the changes Signed-off-by: PRP <[email protected]> * fixing the mistake Signed-off-by: PRP <[email protected]> * adding default values for the selector Signed-off-by: PRP <[email protected]> * updating the plugin description Signed-off-by: PRP <[email protected]> * setting default in combox and removing after selected Signed-off-by: PRP <[email protected]> * minor Signed-off-by: PRP <[email protected]> * adding goal checker and smoother selectors to the plugins Signed-off-by: PRP <[email protected]> * updating all the relevant BTs Signed-off-by: PRP <[email protected]> * updating to 2x2 version Signed-off-by: PRP <[email protected]> * updating the bringup too Signed-off-by: PRP <[email protected]> * fixing the race condition Signed-off-by: PRP <[email protected]> * adding progress checker to the selector plugin Signed-off-by: PRP <[email protected]> * fixing CI Signed-off-by: PRP <[email protected]> * updating the cache version Signed-off-by: PRP <[email protected]> --------- Signed-off-by: PRP <[email protected]>
…igation#4091) * initial commit Signed-off-by: PRP <[email protected]> * now dynamic reconfigurability also be achieved Signed-off-by: PRP <[email protected]> * updating the changes Signed-off-by: PRP <[email protected]> * fixing the mistake Signed-off-by: PRP <[email protected]> * adding default values for the selector Signed-off-by: PRP <[email protected]> * updating the plugin description Signed-off-by: PRP <[email protected]> * setting default in combox and removing after selected Signed-off-by: PRP <[email protected]> * minor Signed-off-by: PRP <[email protected]> * adding goal checker and smoother selectors to the plugins Signed-off-by: PRP <[email protected]> * updating all the relevant BTs Signed-off-by: PRP <[email protected]> * updating to 2x2 version Signed-off-by: PRP <[email protected]> * updating the bringup too Signed-off-by: PRP <[email protected]> * fixing the race condition Signed-off-by: PRP <[email protected]> * adding progress checker to the selector plugin Signed-off-by: PRP <[email protected]> * fixing CI Signed-off-by: PRP <[email protected]> * updating the cache version Signed-off-by: PRP <[email protected]> --------- Signed-off-by: PRP <[email protected]>
Basic Info
Description of contribution in a few bullet points
Now we can select the controller and the planner plugins on the fly
Description of documentation updates required from your changes
Maybe RViz plugins after all needs a documentation somewhere?
Future work that may be required in bullet points
For Maintainers: