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

adding dwb param descriptions #31

Merged
merged 12 commits into from
Jun 12, 2020

Conversation

Marwan99
Copy link
Contributor

per #21

All DWB param descriptions should be there now. (hardest part completed). Examples to be added.
@SteveMacenski What do think of the layout? Any suggestions on where the publish and kinematic parameters should live?

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

I can't find this in the generated website, where is this work?

Co-authored-by: Steve Macenski <stevenmacenski@gmail.com>
@SteveMacenski
Copy link
Member

I think you should split the main page into 4 pages: dwb controller, iterator, kinematic parameters, visualization - along with the other separated pages. Its just too long. I think the main landing page here should just be a list of links to the other pages and then the full example below it. The DWB main-controller parameters should be first, then the other 3 on this main page, then the others you have listed.

configuration/packages/dwb-params/controller.rst Outdated Show resolved Hide resolved
@@ -1,11 +1,13 @@
.. dwb_iterator:
.. _dwb_iterator:

Iterator
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the XYTheta iterator, not just the generic iterator.

@@ -1,11 +1,13 @@
.. dwb_kinematic_params:
.. _dwb_kinematic_params:

Kinematics
Copy link
Member

Choose a reason for hiding this comment

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

this should also be the full name like the iterator page. Kinematic Parameters

@@ -1,11 +1,13 @@
.. dwb_vis:
.. _dwb_vis:

Visualization
Copy link
Member

Choose a reason for hiding this comment

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

As well here, use the full name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Full name for this is Publisher, right?

Copy link
Member

Choose a reason for hiding this comment

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

Correct

Co-authored-by: Steve Macenski <stevenmacenski@gmail.com>
@SteveMacenski
Copy link
Member

Reminder to self: post on discourse about this when this is in. Last PR before complete

@SteveMacenski
Copy link
Member

SteveMacenski commented Jun 12, 2020

@Marwan99 please delete the Tunable Parameters page completely. We now have completely replaced it

https://navigation.ros.org/configuration/params/tunable-params.html

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Please put as a full PR. Once we delete that one page, we can merge this and be done 😄

@SteveMacenski SteveMacenski marked this pull request as ready for review June 12, 2020 22:46
@SteveMacenski
Copy link
Member

Looks like you have a couple of merge conflicts the draft PR was hiding

@SteveMacenski SteveMacenski merged commit f3fa28b into ros-navigation:master Jun 12, 2020
@SteveMacenski
Copy link
Member

Congrats @Marwan99! This was a big effort and we got it done in a couple weeks. What's your discourse handle?

@Marwan99
Copy link
Contributor Author

Thanks @SteveMacenski, appreciate your guidance and support! My discourse handle is marwan99, just created the account!

@Marwan99 Marwan99 deleted the dwb-params branch June 12, 2020 23:02
@SteveMacenski
Copy link
Member

https://discourse.ros.org/t/navigation2-complete-parameter-guide-now-available/14649

@SteveMacenski
Copy link
Member

Celebrate the small victories or no one else will :-)

@Marwan99
Copy link
Contributor Author

Should we tick the first item in this list? ros-navigation/navigation2#1589 (comment)

@SteveMacenski
Copy link
Member

good call, done

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.

2 participants