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

ROS1 documentation updates and enhancements #316

Merged
merged 79 commits into from
Dec 11, 2022
Merged

Conversation

tsampazk
Copy link
Collaborator

@tsampazk tsampazk commented Sep 22, 2022

  • Main changes in projects/opendr_ws/src/perception/README.md include:
    • Added a prerequisite section describing common prerequisites between nodes and notes section for general information
    • Separated the node documentation in sections depending on their input type, similar to the contents in projects/opendr_ws/README.md
    • Added argument information about all currently updated nodes that use argparse
  • Did some restructuring on projects/opendr_ws/README.md to match the order of the projects/opendr_ws/src/perception/README.md content
  • Added a class ID table for semantic segmentation bisenet both in the ROS documentation and the tool's documentation
  • Added some TODO comments on not yet updated nodes, see tracker issue [Tracker] ROS1 nodes updates and fixes #305.

@tsampazk tsampazk added the documentation Improvements or additions to documentation label Sep 22, 2022
@tsampazk tsampazk self-assigned this Sep 22, 2022
@tsampazk
Copy link
Collaborator Author

Hey @stefaniapedrazzi! Right now from my end the PR is ready for review, but the docs are 'incomplete' as some nodes are not yet updated. Do you think we can go ahead and review/merge this and change the docs for remaining nodes as soon as they are updated or keep this as draft and update it simultaneously?

@stefaniapedrazzi
Copy link
Collaborator

Given that the list of nodes that still needs to be updated should be already listed in #305, I think that we can review and merge this PR.

@tsampazk tsampazk marked this pull request as ready for review September 26, 2022 08:59
Copy link
Collaborator

@stefaniapedrazzi stefaniapedrazzi left a comment

Choose a reason for hiding this comment

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

Changes are fine!

There is just one link pointing to a file that doesn't exist.
And maybe the very long lines containing multiple sentences may be split.

projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/perception/README.md Outdated Show resolved Hide resolved
@tsampazk
Copy link
Collaborator Author

Thank you for the review!

And maybe the very long lines containing multiple sentences may be split.

Sorry about that, i am not exactly sure what convention we are following in the docs, regarding the long lines. I applied the fixes suggested.

@stefaniapedrazzi
Copy link
Collaborator

stefaniapedrazzi commented Sep 26, 2022

Initially I tried to insist on having one sentence per line on the documentation for MD filed because it is easier to read and review the sources.
But this doesn't seem very intuitive for all the other partners and at the end it is not a very important issue.

So it is preferable to not split sentences on different lines even if they are very long and it is ok to have multiple sentences per line if they are not too long.
But if there are long sentences I would say it is better to write them on a new line.

Copy link
Collaborator

@stefaniapedrazzi stefaniapedrazzi left a comment

Choose a reason for hiding this comment

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

Thank you!

@tsampazk
Copy link
Collaborator Author

Initially I tried to insist on having one sentence per line on the documentation for MD filed because it is easier to read and review the sources. But this doesn't seem very intuitive for all the other partners and at the end it is not a very important issue.

So it is preferable to not split sentences on different lines even if they are very long and it is ok to have multiple sentences per line if they are not too long. But if there are long sentences I would say it is better to write them on a new line.

Alright TYVM for the explanation!

@tsampazk
Copy link
Collaborator Author

tsampazk commented Dec 1, 2022

I have made any updates needed for #364 locally (also merged that branch into this, resolving the conflicts) and i am going to push them as soon as #364 is merged, as i think this will make the procedure as clean as possible and everything ends up merging in develop smoothly.

@tsampazk
Copy link
Collaborator Author

tsampazk commented Dec 7, 2022

The updates in the ROS docs are complete now, PR is ready for review.

Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

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

That's quite the improvement, very clear and complete, thank you!
Only a few suggestions

projects/opendr_ws/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/README.md Outdated Show resolved Hide resolved
projects/opendr_ws/src/opendr_perception/README.md Outdated Show resolved Hide resolved
@tsampazk
Copy link
Collaborator Author

tsampazk commented Dec 8, 2022

Thanks @ad-daniel for the review! Should we add the test-tools label now that the install.sh file is modified?

@ad-daniel ad-daniel added test sources Run style checks test tools Test the toolkit methods labels Dec 8, 2022
@ad-daniel
Copy link
Collaborator

sure doesn't hurt

Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

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

We can wait for the tests, but it looks good to me, thanks!

@ad-daniel
Copy link
Collaborator

no need to wait the tests again, once they pass we sync the branch and remove the label

@ad-daniel ad-daniel removed the test tools Test the toolkit methods label Dec 8, 2022
Copy link
Collaborator

@passalis passalis left a comment

Choose a reason for hiding this comment

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

Also looks good to me as well! Thank you!

@ad-daniel ad-daniel merged commit a57fb9f into develop Dec 11, 2022
@ad-daniel ad-daniel deleted the ros-nodes-docs branch December 11, 2022 17:53
lucamarchionni pushed a commit to lucamarchionni/opendr that referenced this pull request Jun 10, 2024
* Added prerequisites section for common prerequisites across nodes

* Overhauled the dataset nodes section and added RGB nodes section

* Rearranged the listed node links

* General rearrangement and input sections

* Additional modifications and pose estimation section

* Section renaming for consistency

* Some rearrangement in contents list to match the order

* Fall detection doc and moved dataset nodes to bottom

* Face det, reco, 2d object detection overhaul and todo notes

* Added a class id table on sem segmentation doc

* Panoptic and semantic segmentation overhaul

* Fix long lines as per suggestions

* Fix long lines as per suggestions

* Removed commented pose estimation usage suggestion

* Update video HAR docs foro ROS node

* Updated the video human activity recognition section and some other minor fixes

* Fixed italics showing as block

* Removed redundant line separators after headers

* Removed redundant horizontal line from RGBD header

* Added notes for output visualization and updated pose estimation docs

* Added missing space in pose estimation docs

* Updates on formatting for all other applicable nodes' docs and minor fixes

* More detailed ros setup instructions

* Added skipping of workspace build step

* Updated RGBD hand gesture recognition ros node doc

* Updated speech command recognition ros node doc and some minor fixes

* Updated heart anomaly detection ros node doc

* Reordered audio section and added RGB + Audio section

* Added audiovisual emotion reco missing doc and reordered audio section

* Added link to csv file with classes-ids for activity recognition

* Added link to csv file with class-ids for activity recognition

* Minor improvements

* Several minor fixes and landmark-based facial expression recognition

* Skeleton-based human action recognition and minor fixes

* Moved fair mot in rgb input section

* Completed ROS1 docs

* Updates on default values for FairMOT ros node class ctor

* Fixed duplicate shortcut on deepsort ros node argparse

* Fixed missing shortcut on rgbd hand gesture reco ros node argparse

* Added "opendr_" to data gen package and "_node" to the node file name

* Renamed package to "opendr_perception" and added "_node" to scripts

* Applied fixes to yolov5

* Added "opendr_" to planning package

* Added "opendr_" to bridge package

* Added "opendr_" to simulation package

* Fixed old version of torch in pip_requirements.txt

* Renamed ros bridge package doc

* Updated based on new names and some minor modifications

* Fixed list numbers

* Merge clean-up

* Added a new notes item with a node diagram and explanation

* Minor formatting fixes in the diagram section

* Fixed step X dot consistency

* Fixed step X dot consistency

* Added detached & in instructions for running stuff

* Removed apt ros package installation instructions

* Added some additional required ros packages and made ROS version a variable

Co-authored-by: LukasHedegaard <[email protected]>
Co-authored-by: ad-daniel <[email protected]>
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 test sources Run style checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants