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

docs: update creating vehicle and sensor model pages #460

Conversation

ismetatabay
Copy link
Member

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.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@ismetatabay ismetatabay force-pushed the ismet/dev/create-sensor-and-vehicle-model branch 5 times, most recently from d07b88a to c646a14 Compare September 22, 2023 15:21
@ismetatabay ismetatabay added the tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) label Sep 25, 2023
@ismetatabay ismetatabay force-pushed the ismet/dev/create-sensor-and-vehicle-model branch 6 times, most recently from 0930e3f to 51eca2e Compare September 29, 2023 13:03
@ismetatabay ismetatabay marked this pull request as ready for review September 29, 2023 13:06
@ismetatabay
Copy link
Member Author

@mitsudome-r Hi Mitsudome-san, this PR is ready for review. (FYI)

@mitsudome-r mitsudome-r self-requested a review October 6, 2023 10:33
@ismetatabay ismetatabay force-pushed the ismet/dev/create-sensor-and-vehicle-model branch 2 times, most recently from 45d0a65 to 15e353f Compare October 13, 2023 10:23
@danielsanchezaran
Copy link

danielsanchezaran commented Oct 20, 2023

@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?

@ismetatabay ismetatabay force-pushed the ismet/dev/create-sensor-and-vehicle-model branch from 3c747ba to 43bb1aa Compare October 20, 2023 12:42
@ismetatabay
Copy link
Member Author

@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

Copy link

@danielsanchezaran danielsanchezaran left a 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.

@danielsanchezaran
Copy link

@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

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.

@ismetatabay ismetatabay force-pushed the ismet/dev/create-sensor-and-vehicle-model branch from bc0fa56 to 6d6c2fb Compare October 23, 2023 10:33
@ismetatabay
Copy link
Member Author

@danielsanchezaran I updated the documentation with your suggestions. These suggestions really enhance the readability of the documentation. Thank you for your effort. 💯

@ismetatabay ismetatabay force-pushed the ismet/dev/create-sensor-and-vehicle-model branch from 6d6c2fb to a83ba50 Compare October 23, 2023 10:42
@ismetatabay
Copy link
Member Author

@danielsanchezaran, @mitsudome-r Hi, is it okay to merge?

@mitsudome-r
Copy link
Member

@danielsanchezaran Thanks for the review.
@ismetatabay It seems like there are conflicts. We can merge this once they are fixed.

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]>
@ismetatabay ismetatabay force-pushed the ismet/dev/create-sensor-and-vehicle-model branch from 72a3938 to 1c3d603 Compare November 17, 2023 07:46
@ismetatabay ismetatabay merged commit 9643c40 into autowarefoundation:main Nov 17, 2023
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants