Refactor: change parameter naming style#854
Refactor: change parameter naming style#854AJedancov wants to merge 10 commits intoros-navigation:masterfrom
Conversation
|
@mini-1235 please sanity check there aren't missing things :- ) |
| <PipelineSequence name="NavigateWithReplanning"> | ||
| <ControllerSelector selected_controller="{selected_controller}" default_controller="FollowPath" topic_name="controller_selector"/> | ||
| <PlannerSelector selected_planner="{selected_planner}" default_planner="GridBased" topic_name="planner_selector"/> | ||
| <ProgressCheckerSelector selected_progress_checker="{selected_progress_checker}" default_progress_checker="progress_checker" topic_name="progress_checker_selector"/> |
There was a problem hiding this comment.
Here and in general: This is going to be hard considering these docs touch all distributions :( changing the casing will make it so configuration files in Kilted and prior will not work with docs / BTs / yamls in the docs.
I'd like to brain storm a way for how to handle this to make it more clear. For some like the configuration guide pages, I think we can pretty easily just have a note saying its FollowPath in Kilted and older and follow_path in Lyrical and newer.
The BTs/YAMLs maybe just add a comment on each line <!-- FollowPath in kilted and older --> and # FollowPath in kilted and older anywhere the snake case is introduced?
Its alot of redundancy, but its the best I can think of right now without having different versions of docs which is something on our minds, but not something we have today
There was a problem hiding this comment.
Here and in general: This is going to be hard considering these docs touch all distributions :( changing the casing will make it so configuration files in Kilted and prior will not work with docs / BTs / yamls in the docs.
Oh yeah, that will be confusing in this case.
I'd like to brain storm a way for how to handle this to make it more clear.
Okay, I think we can leave a small note in brackets for the text.
The BTs/YAMLs maybe just add a comment on each line and # FollowPath in kilted and older anywhere the snake case is introduced?
For code blocks and images, I would consider an option using tabs instead of comments.
I see several advantages here:
- Using
group-tab::, we can switch all tabs synchronously when switching one on the same page. I've checked this works even with other pages in one folder (but not for entire project). An example of how it works can be seen on one of the pages of ROS 2 documentation. - It's easier to copy from code or config examples. The content will include only relevant information.
- It will simplify the transition process when documentation for different versions becomes available. The only thing that needs to be done is to delete the tab for the corresponding version and leave its contents.
Here are some examples of what this might look like:

I think for tables we can use additional column labeled Default in Lyrical and newer and Default in Kilted and older.

Or we can apply a similar approach with tabs:

This can be useful for syncing between tabs with subsequent code examples.
Would you consider this approach?
There was a problem hiding this comment.
Oh I like that! Tabs for the BT XML and the additional column for the yaml parameter descriptions I think look best :-)
There was a problem hiding this comment.
@AJedancov I changed my mind actually. Can you instead of doing the additional column for the yaml parameter descriptions, have the entire configuration guide parameter section be under that tab? That would future proof for other additional parameter updates. Even if right now Jazzy/Kilted/Rolling are all the same, setting up that pattern in all of the configuration guide pages would help us for enabling multi-distribution docs moving forward.
Great idea!
There was a problem hiding this comment.
Can you instead of doing the additional column for the yaml parameter descriptions, have the entire configuration guide parameter section be under that tab?
Sure! Sounds good to me too.
There was a problem hiding this comment.
I have prepared commit b3037c0 with tabs. I just want to point out that in some places I have included adjacent paragraph into tab, for example in behavior_trees/trees/nav_through_poses_recovery.rst, since some configuration files of different distributions include different BT nodes and mentions of them. And in other places I inserted notes directly in the text, as discussed above.
In addition 939836f, I think it would be useful to add tabs for examples to the migration guide I prepared earlier.
behavior_trees/trees/nav_to_pose_with_consistent_replanning_and_if_path_becomes_invalid.rst
Outdated
Show resolved
Hide resolved
437df76 to
8d6ef31
Compare
|
Let us know when we should review again! |
mini-1235
left a comment
There was a problem hiding this comment.
Largely looks good to me. I left a few comments regarding parameters/types/plugins that don't exist in older distributions
| ------------------------------------- ------- | ||
| geometry_msgs::msg::PoseStamped N/A | ||
| ===================================== ======= | ||
| .. tabs:: |
There was a problem hiding this comment.
If I remember correctly, we don't use nav_msgs::msg::Goals in Jazzy and Older
There was a problem hiding this comment.
You are right, in Jazzy is geometry_msgs::msg::PoseStamped[] instead. The situation is that Kilted already uses nav_msgs::msg::Goals.
One more tab? :)
There was a problem hiding this comment.
Or add a comment similar to this one:
collision_monitor:
ros__parameters:
...
enable_stamped_cmd_vel: True # False for Jazzy or older
There was a problem hiding this comment.
Yeah, I think this is one downside of using tabs to handle multi-version. Since we are usually referring to a range of distributions rather than a specific one, adding a tab for every distro doesn't feel right to me either
Thoughts? @SteveMacenski
Maybe we should just remove the tabs and use comment instead as @AJedancov suggested in this case?
| string N/A | ||
| ====== ======= | ||
|
|
||
| Description |
There was a problem hiding this comment.
The path handler does not exist in Kilted and older
| ============== ============================= | ||
|
|
||
| Description | ||
| How far (in meters) along the path the searching algorithm will look for the closest point. |
There was a problem hiding this comment.
Path handler and search window only exist in Lyrical and newer
|
|
||
| Description | ||
| The lifecycle node bond mechanism publishing period (on the /bond topic). Disabled if inferior or equal to 0.0. | ||
| :allow_partial_planning: |
There was a problem hiding this comment.
allow partial planning only exist in lyrical and newer
| stateful: true | ||
| use_dynamic_window: false | ||
|
|
||
| .. group-tab:: Kilted and older |
There was a problem hiding this comment.
Kilted and older should be:
plugin: "nav2_regulated_pure_pursuit_controller::RegulatedPurePursuitController"
desired_linear_vel: 0.5
lookahead_dist: 0.6
min_lookahead_dist: 0.3
max_lookahead_dist: 0.9
lookahead_time: 1.5
rotate_to_heading_angular_vel: 1.8
use_velocity_scaled_lookahead_dist: false
min_approach_linear_velocity: 0.05
approach_velocity_scaling_dist: 0.6
use_collision_detection: true
max_allowed_time_to_collision_up_to_carrot: 1.0
use_regulated_linear_velocity_scaling: true
use_fixed_curvature_lookahead: false
curvature_lookahead_dist: 0.25
use_cost_regulated_linear_velocity_scaling: false
cost_scaling_dist: 0.3
cost_scaling_gain: 1.0
regulated_linear_scaling_min_radius: 0.9
regulated_linear_scaling_min_speed: 0.25
use_rotate_to_heading: true
allow_reversing: false
rotate_to_heading_min_angle: 0.785
max_angular_accel: 3.2
stateful: true
| plugin: "nav2_regulated_pure_pursuit_controller::RegulatedPurePursuitController" | ||
| # ... | ||
|
|
||
| .. group-tab:: Kilted and older |
There was a problem hiding this comment.
Kilted and older should be:
FollowPath:
plugin: "nav2_rotation_shim_controller::RotationShimController"
primary_controller: "nav2_regulated_pure_pursuit_controller::RegulatedPurePursuitController"
angular_dist_threshold: 0.785
forward_sampling_distance: 0.5
angular_disengage_threshold: 0.3925
rotate_to_heading_angular_vel: 1.8
max_angular_accel: 3.2
simulate_ahead_time: 1.0
rotate_to_goal_heading: false
# Primary controller params can be placed here below
# ...
without the primary controller namespace
|
|
||
| .. tabs:: | ||
|
|
||
| .. group-tab:: Lyrical and newer |
There was a problem hiding this comment.
Path handler does not exist in Kilted and older
There was a problem hiding this comment.
What is better for pages like this that will appear in the new distribution only?
- Wrap the parameters/example into a single tab with the name of the new version. At least informative and will serve as a basis for future updates.
- Insert a Note/Warning section at the very top of the page with info about belonging to the new distribution.
| controller_plugins: ["follow_path"] | ||
| follow_path: | ||
| plugin: "nav2_rotation_shim_controller::RotationShimController" | ||
| primary_controller: "dwb_core::DWBLocalPlanner" |
There was a problem hiding this comment.
Please update according to https://github.com/ros-navigation/docs.nav2.org/pull/800/files, it looks like I forgot to update the tutorial in that PR 😄
|
More thematically, I know @AJedancov is working on the plugin naming concern and that's a different problem than having corrected information in each distribution. I don't want to assign the project of updating all configuration guides for kilted/jazzy unless that's something @AJedancov that you'd volunteer to do. I understand that the tabs action item incidentally created the situation that we would want to have updated parameters for each distribution. So @AJedancov is this something that you're open to doing? If not, then we should find a different path. Maybe merging with tabs with a warning printed for all non-rolling distro that this is not yet properly updated and is WIP (with a link to a ticket). |
Due to the large improvements between distros, my previous review may still have missed a few parameters/plugins/types, so the warning and a follow-up ticket in this repository are likely still needed for this PR |
|
So, maybe we could use both approaches and benefits of each?
Another problem with tabs is that they increase the size of the original
I think this task requires a good understanding of the project's history and monitoring of all backported changes. It's difficult for me to estimate the time required for this. It's not just about implementation, but also about finding differences across all the distributions. At the moment I'm not ready to deal with this, but I'll keep that in mind for the future.
For the |
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
Actually, that's exactly what I was thinking too. Especially after places that include multiple tab groups on the same page, pasting them into each tab can be a bit cumbersome.
Sure, I consider them in the same way. This is just an attempt to distribute information a little, at least for the relevant PR.
Would you consider removing all comments that relate to older versions from the Lyrical tab and leaving them only in the second tab? To stick to the idea I mentioned in the previous comment.
The warning message sounds good to me 👍 Thank you for the suggestion! A few more thoughts on the upcoming warnings: |
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
939836f to
4b06d98
Compare
Can you provide an example?
If we are adding this in every page, this should be a few lines to the .html file: Also, please avoid force push, as it makes the review difficult given the size of this PR :) |
Wow, thanks for the link! I'll check later how it works in our case.
Sure, sorry for the inconvenience. The latest commit includes the changes from your last review only. I'll check Lyrical/Kilted migration guide in case something else is missing and open for review a bit later. |
|
Yes! I think the comments can be removed in the lyrical and newer tab |
Longer term, I think we should keep them targeted to a specific distribution, not grouped together. But I agree that for now Rolling (Lyrical) and then 'Kilted and Older' is fine considering that everything in Kilted and Older will already not up to date. As Kilted gets updated we can add increasing depth of older distributions as part of that work (separate of this PR). Only actionable note is that I'd like to use the name "Rolling" since Lyrical doesn't exist. Either way when Lyrical is cut, we'll have to add a new tab (either for the new head for Rolling, or for taking the current content in rolling and calling a tab before it Lyrical - same difference).
My suggestion is that it should be the first thing in the tab for any tabs that are not up-to-date (Kilted and older, for now). That way it only appears when you click on a bad tab. Every time we migrate a tab for a page, we can remove this warning to signal that its 'good info'. It would help us know which are updated and which are not as we go through that process & which tabs of which pages can be trusted by the readers. I'd rather this not on the top of each page for that reason, does that make sense?
I'm curious to hear why's that? I figure this is actually easier than maintaining separate branches which would involve additional PRs for docs for each backport. If tabs aren't the long term solution, I'd actually rather not do them at all here since that adds complexity to manage / revert later + alot of work to get Kilted/Older populated which would otherwise be wasted engineering cycles. Merging this involves some major time commitment in the nearterm future to populate (probably will try to recruit some community contributors + myself). I honestly have a hard time thinking of any 'perfect' solution to multidistro docs, which is why we don't have it as of the time of writing :/ Other than (a) auto generating them or (b) having a tool which extracts all parameters and validates they're on the pages they should be for each distribution or else fail the build, I'm lacking a great solution which makes sure things don't constantly fall through the cracks. Of these two, I'd personally favor (B) because (A) would involve having all comments inline in the code which would add alot of bloat. (B) makes it clear that there's a problem for immediate resolution, but also can fail on people's unrelated PRs adding burdens to them to fix things out of the scope of their work. Plus proactive PRs would add parameters not yet in those branches, so there's that. I suppose some hybrid could be maintaining Rolling and have only the released distributions auto generated. We take the descriptions from the Rolling entries but populate the in-code default values specific to each distro. For parameters that are unique to a particular distro, we'd have to maintain a separate description list file it can pull from, but those are few and far between. Hardly a huge issue. But this is trading fairly manual parameter validation for up-front development cost in these tools. I'm open to any/all suggestions. Other options we've talked about in the past using tags and backport branches seemed too fragile to rely on. As far as I suggest:
It seems like we're blocked by (1) before we can merge this work since its such a breaking change all over the place. I know we stepped into quite the mess here that was not the original point. @AJedancov I don't want to keep piling on / make this death by a thousand cuts :( |
I could be wrong, but this is my overall impression after reviewing the tab-based approach. First, if a tab refers to a range of distributions, it can easily become inaccurate, since there may be differences even between closely related distros (e.g. jazzy vs humble). You mentioned targeting a specific distribution per tab rather than grouping them, which helps, so let's focus on that. Here are a few cons that come to my mind: Appearance This is what I see when building the docs locally (with only two tabs at the moment). There are already some layout and overflow issues. Even if we remove tabs as soon as a distro is deprecated and maintain only four tabs, some pages (especially those with long parameter lists or examples) could still become quite hard to read.
This feels like something multi version docs would handle more properly(separate URLs per distribution rather than all versions living on the same page). That approach also makes it easier to clearly mark a distro as deprecated, stop updating it, but still keep it around for a while to allow users a smoother transition. It also avoids the need to later "clean up" deprecated content, which reduces maintenance cost. Maintenance With tabs, the .rst files can grow very large very quickly. That makes them harder to read and review. I also feel that it's easier for contributors and github bots to update docs if they are maintained separately (different branches or different folders per distro). Also consider the assets, how should we rename them? (humble/1.png jazzy/1.png kilted/1.png)? I call this a reasonable stopgap, mainly because it's easy to copy + paste or migrate the content without too much pain. Something that recently came to mind is https://zensical.org/, though at the moment versioning isn’t fully supported yet (see zensical/backlog#45). Overall, I don't have strong objections to the tab-based approach, but I think we can do a better job later this year when I / you / we have more time to work on it |
|
First off: https://zensical.org/ That's a cool fucking website. Whenever that's ready, would be a good option like Material for MkDocs. OK - you've convinced me with the formatting issues. To alternate to tables, is it reasonable to ask for this to setup the multi-version docs for Basically: Kilted & Older is just the docs right now. Rolling would just be your PR's changes. Just make sure to pull in all current changes in Ref I think it might be good for me to hire someone part time to migrate the docs to the new MK Docs (or another) format + do the updates for multi-distribution. If you guys know anyone that would be interested, let me know. My budget isn't infinite, but I think this is worth investing in. This is not something I'd ask you @mini-1235 to do, I wouldn't ask you to do anything I myself wouldn't do 😆 |
|
I was thinking that we could add a video of real robot navigation on the landing page like what they do, I think that would be really cool!
I haven't looked into sphinx-multiversion in detail yet, so I am not sure how much effort it would require, I would defer to @AJedancov’s opinion on this.
I have a habit of taking notes, and I have recently been trying out zensical, maybe in my free time I can think about how we might start migrating this (though we'll probably need to wait until they support multi-version). And also, I have a few issues/projects that I want to prioritize now 😆 |
Definitely. That's actually why I hired a web developer to make nav2.org
I think those PRs I referenced actually do that for you. If anything it would just be a little cleanup. |
Tracking the progress of changes will be an advantage here, I agree.
We can probably try using the combination again:
In each case users will be warned about possible version differences. And for us there will be a progress indicator that cover all places. We can postpone the discussion of these issues to the next PR after this. I'll make it first only in tabs, and we'll check how it looks. Perhaps other points or implementation ideas will arise. |
I am also not familiar with their implementation yet.
I had the opportunity to work a little with Material for MkDocs and I think is good alternative. Markdown seems nicer to work with. And the overall appearance is pretty cool. I noticed multi-version support in docs, but I haven't checked how well it works. And it's great that they grew into a separate project (zensical). I'd like to take a closer look at it later. |
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
Signed-off-by: AJedancov <andrei.jedancov@gmail.com>
|
In last three commits:
For the last point, it was necessary to add tabs to some more pages to match the new parameters. Therefore, it is "a little" difficult to check what exactly was changed using git-diff. For simplicity, I'll highlight key points below. Added to Kilted tabs:
Removed from Kilted tabs:
Updated Kilted tabs:
Removed from Rolling tab (MPPI only):
Added single tab for Rolling:
Added single tab for Kilted:
Changed parameter name in Kilted/Lyrical Migration guide:
And I think I can remove |
I agree, with the declarative input/output ports, this should be easy, except when they derive from a base class. We'd need to check for that too.
In what way?
I thought though perhaps the answer is not to use tables but use the multidistro support - which would resolve this a more scalable way towards our long-term goals? Before you keep cranking this out, I think we should figure the direction. I think keeping this cleanly only you changes for now would be good and not trying to fix other distribution issues until we know what we're doing. Keep those changes and/or commits handy though for later. |
I was thinking about some custom extension for Sphinx that will be declared in
Sure, I just wanted to finish those changes in the last commit since I had started them a little earlier, so I just described and shared it. In any case, this may be used as a starting point for the upcoming division into versions. In the last commit, I was focused only on the parameters that intersect with the new plugins names (appear on the same page or same code block) and differ only between Rolling/Kilted. I didn't include other parameters from the latest migration guide that don't intersect with plugins, but I have saved them for the near future, taking into account that we are leaning towards switching to multidistro format. |
|
I think we're chatting about MK Docs, so maybe we can pause on the Sphinx integrations if we're moving away from that. Does MK Docs have something similar? |
|
As for MkDocs, it allows to write own plugins as an alternative to extensions. It seems that there is no built-in alternative to Sphinx directives, but I have already found mkdocs-macros-plugin that provide similar functionality using macros. And at first glance, it gives a pretty flexible way to replace data in .md files (local/global variables, macros, filters) thanks to templates. There may be other alternative plugins, but I think macros are a good fit for the example above. |





Basic Info
Description of contribution in a few bullet points