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

SDF demo #31

Merged
merged 35 commits into from
Aug 11, 2020
Merged

SDF demo #31

merged 35 commits into from
Aug 11, 2020

Conversation

mohamedsayed18
Copy link
Contributor

No description provided.

Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Copy link
Contributor

@maryaB-osr maryaB-osr left a comment

Choose a reason for hiding this comment

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

@mohamedsayed18 I left a partial review since there's quite a few comments. I'll let you address these first, and carry on the changes to the rest of the doc. I really like what you've done so far!

One thing I'd like to see is an outline of the sections - just the headings. something like:

# Title

## What is SDF?

## Breaking down the code

### The model

### Inertial properties

### Frames

## Conclusion

It's good to break a tutorial up into labeled sections as much as logically possible to help learners feel like they're progressing through a set of steps and not facing one huge task

Tutorials/SDF/sdf_readme.md Outdated Show resolved Hide resolved
Tutorials/SDF/sdf_readme.md Outdated Show resolved Hide resolved
Tutorials/SDF/sdf_readme.md Outdated Show resolved Hide resolved
Tutorials/SDF/sdf_readme.md Outdated Show resolved Hide resolved
Tutorials/SDF/sdf_readme.md Outdated Show resolved Hide resolved
Tutorials/SDF/sdf_readme.md Outdated Show resolved Hide resolved
Tutorials/SDF/sdf_readme.md Outdated Show resolved Hide resolved
Tutorials/SDF/sdf_readme.md Outdated Show resolved Hide resolved
Tutorials/SDF/sdf_readme.md Outdated Show resolved Hide resolved
knowledge_base_roadmap.md Outdated Show resolved Hide resolved
@mohamedsayed18
Copy link
Contributor Author

Thank you for your detailed feedback, waiting for the rest of the readme.
I'm working now on fixing these issues.

Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I left a few suggestion on how to improve the vehicle SDF. The final vehicle I tested is here.

Tutorials/SDF/car_model.sdf Outdated Show resolved Hide resolved
Tutorials/SDF/car_model.sdf Outdated Show resolved Hide resolved
Tutorials/SDF/car_model.sdf Outdated Show resolved Hide resolved
Tutorials/SDF/car_model.sdf Outdated Show resolved Hide resolved
Tutorials/SDF/car_model.sdf Outdated Show resolved Hide resolved
Tutorials/SDF/car_model.sdf Outdated Show resolved Hide resolved
Tutorials/SDF/car_model.sdf Outdated Show resolved Hide resolved
Tutorials/SDF/car_model.sdf Outdated Show resolved Hide resolved
Tutorials/SDF/car_model.sdf Outdated Show resolved Hide resolved
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Did another pass and left some more comments 👍

Tutorials/Moving_robot/moving_robot.md Outdated Show resolved Hide resolved
Tutorials/Moving_robot/moving_robot.md Outdated Show resolved Hide resolved
Tutorials/SDF/car_demo.sdf Outdated Show resolved Hide resolved
Tutorials/Moving_robot/move_robot.sdf Outdated Show resolved Hide resolved
Tutorials/Moving_robot/moving_robot.md Outdated Show resolved Hide resolved
Tutorials/Moving_robot/moving_robot.md Outdated Show resolved Hide resolved
Tutorials/Moving_robot/moving_robot.md Outdated Show resolved Hide resolved
Tutorials/Moving_robot/moving_robot.md Outdated Show resolved Hide resolved
Tutorials/Moving_robot/moving_robot.md Outdated Show resolved Hide resolved
Tutorials/Moving_robot/moving_robot.md Outdated Show resolved Hide resolved
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
@chapulina chapulina changed the title Tutorials SDF and moving robot Jul 16, 2020
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
@chapulina chapulina added the documentation Improvements or additions to documentation label Jul 22, 2020
mohamedsayed18 and others added 3 commits July 23, 2020 14:57
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: maryaB-osr <[email protected]>
@mohamedsayed18 mohamedsayed18 changed the title SDF and moving robot SDF demo Jul 23, 2020
tutorials/SDF/car_demo.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I just have some minor comments.

I just had an idea so the user can see something happening by the end of this tutorial. What do you think of making the ground plane slightly rotated so that in the end of the tutorial, we can tell the user to press "play" and they see the car rolling downhill?

knowledge_base_roadmap.md Outdated Show resolved Hide resolved
knowledge_base_roadmap.md Outdated Show resolved Hide resolved
knowledge_base_roadmap.md Outdated Show resolved Hide resolved
knowledge_base_roadmap.md Outdated Show resolved Hide resolved
knowledge_base_roadmap.md Outdated Show resolved Hide resolved
tutorials/SDF/car_demo.md Outdated Show resolved Hide resolved
tutorials/SDF/car_demo.md Outdated Show resolved Hide resolved
tutorials/SDF/car_demo.md Outdated Show resolved Hide resolved
tutorials/SDF/car_demo.md Outdated Show resolved Hide resolved
tutorials/SDF/car_demo.md Outdated Show resolved Hide resolved
Signed-off-by: mohamedsayed18 <[email protected]>
@mohamedsayed18 mohamedsayed18 marked this pull request as ready for review August 4, 2020 09:54
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I just have some final tweaks to suggest and then I think this is ready to go in.

As I mentioned on chat, I also think we should rename this tutorial from car_demo to some more descriptive name.

tutorials/SDF/car_demo.md Outdated Show resolved Hide resolved
tutorials/SDF/car_demo.md Outdated Show resolved Hide resolved
tutorials/SDF/car_demo.md Outdated Show resolved Hide resolved
tutorials/SDF/car_demo.sdf Outdated Show resolved Hide resolved
Signed-off-by: mohamedsayed18 <[email protected]>
@chapulina chapulina merged commit 3c21b16 into gazebosim:master Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants