-
Notifications
You must be signed in to change notification settings - Fork 144
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
docs: update creating vehicle and sensor model pages #460
docs: update creating vehicle and sensor model pages #460
Conversation
d07b88a
to
c646a14
Compare
0930e3f
to
51eca2e
Compare
@mitsudome-r Hi Mitsudome-san, this PR is ready for review. (FYI) |
45d0a65
to
15e353f
Compare
docs/how-to-guides/integrating-autoware/creating-vehicle-and-sensor-model/index.md
Outdated
Show resolved
Hide resolved
docs/how-to-guides/integrating-autoware/creating-vehicle-and-sensor-model/index.md
Outdated
Show resolved
Hide resolved
docs/how-to-guides/integrating-autoware/creating-vehicle-and-sensor-model/index.md
Outdated
Show resolved
Hide resolved
docs/how-to-guides/integrating-autoware/creating-vehicle-and-sensor-model/index.md
Outdated
Show resolved
Hide resolved
docs/how-to-guides/integrating-autoware/creating-vehicle-and-sensor-model/index.md
Outdated
Show resolved
Hide resolved
docs/how-to-guides/integrating-autoware/creating-vehicle-and-sensor-model/index.md
Outdated
Show resolved
Hide resolved
docs/how-to-guides/integrating-autoware/creating-vehicle-and-sensor-model/index.md
Outdated
Show resolved
Hide resolved
docs/contributing/coding-guidelines/ros-nodes/coordinate-system.md
Outdated
Show resolved
Hide resolved
...s/integrating-autoware/creating-vehicle-and-sensor-model/creating-individual-params/index.md
Outdated
Show resolved
Hide resolved
...s/integrating-autoware/creating-vehicle-and-sensor-model/creating-individual-params/index.md
Outdated
Show resolved
Hide resolved
...s/integrating-autoware/creating-vehicle-and-sensor-model/creating-individual-params/index.md
Outdated
Show resolved
Hide resolved
...s/integrating-autoware/creating-vehicle-and-sensor-model/creating-individual-params/index.md
Outdated
Show resolved
Hide resolved
@ismetatabay I am yet to review the "Creating a vehicle model for Autoware" section, will do that on Monday probably. I have left several reviews and suggestions that I believe will slightly improve the readability of the guide. Overall I believe this PR offers a considerable improvement on the original guide and I thank you for your effort. One comment I would like to make is that your updated guide still uses the YOUR_VEHICLE_NAME convention along with the "tutorial_vehicle" convention. I believe that, while the guide can still be understood pretty well, it is better to only use the "tutorial_vehicle" part and rewrite the examples that use YOUR_VEHICLE_NAME. But that is my point of view. Maybe @mitsudome-r can weight in on that? |
3c747ba
to
43bb1aa
Compare
@danielsanchezaran, Thank you very much for your review comments as well. I completely agree with these changes, and I have implemented all of your suggestions. I wanted to illustrate generalization using YOUR_VEHICLE_NAME for new users, and I used tutorial_vehicle as an example in this context. If it becomes confusing for newcomers, we can discuss whether to remove YOUR_VEHICLE_NAME or tutorial_vehicle. However, I believe it's beneficial to retain both, as tutorial_vehicle features different sensor configurations. FYI: @mitsudome-r |
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.
Some suggestions for the "creating a vehicle model" section.
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
...uides/integrating-autoware/creating-vehicle-and-sensor-model/creating-vehicle-model/index.md
Outdated
Show resolved
Hide resolved
Hmm I see your point. After reading the "Creating a vehicle model for Autoware" part of your updated guide, I agree, I think it is good to have both, YOUR_VEHICLE_NAME and also tutorial_vehicle, as long a s the distinction is clear. |
bc0fa56
to
6d6c2fb
Compare
@danielsanchezaran I updated the documentation with your suggestions. These suggestions really enhance the readability of the documentation. Thank you for your effort. 💯 |
6d6c2fb
to
a83ba50
Compare
@danielsanchezaran, @mitsudome-r Hi, is it okay to merge? |
@danielsanchezaran Thanks for the review. |
Signed-off-by: ismetatabay <[email protected]>
Signed-off-by: ismetatabay <[email protected]>
Signed-off-by: ismetatabay <[email protected]>
Signed-off-by: ismetatabay <[email protected]>
Signed-off-by: ismetatabay <[email protected]>
Signed-off-by: ismetatabay <[email protected]>
Signed-off-by: ismetatabay <[email protected]>
Signed-off-by: ismetatabay <[email protected]>
72a3938
to
1c3d603
Compare
Description
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The Reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.