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

Added development container for replicability #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

YevgeniyEngineer
Copy link

@YevgeniyEngineer YevgeniyEngineer commented Jun 6, 2024

Hi, I would like to check performance of your code.

I was able to make it build by wrapping it in Dev container in VSCode and using Dockerfile to pull necessary build dependencies.

I reorganised the folder to follow ROS formatting guidelines.

I have experienced two problems along the way, and I cannot seem to fix them:

In groundgrid.rviz there are several plugins that I am not sure where to get from, so I had to comment some lines:

  # - Class: autonomos_rviz_bag_control/BagControl
  #   Name: BagControl
  # - Class: fub_simulator_rviz_control/SimulatorControl
  #   Name: SimulatorControl

and

# - Class: autonomos/KeyboardTool

Another problem is Kitti dataset data loader. I found the dataset format I downloaded from https://www.cvlibs.net/datasets/kitti/raw_data.php is not matching the expected data format you have in kitt_data_publisher.py. Could you point me out to the dataset you used?

The devcontainer I set up also pulls NVidia graphics installed on the host machine (requires Nvidia Docker setup), which will allow graphics acceleration of RViz. If this is troublesome, can comment out these lines:

Screenshot from 2024-06-06 18-25-48

An example of devcontainer:
Screenshot from 2024-06-06 18-26-39

Made some improvements to the documentation formatting in README.md

Setup the Docker such that the groundgrid folder should be placed in ~/workspace folder in the host machine, allowing reading data that is stored in the same folder from the development container within Docker.

Formatted each source file using LLVM style with 4 space indent width - see .clang-format

@Nussknacker
Copy link
Collaborator

Thank you for your contribution and for sharing your Docker-Devcontainer integration.

As for your problems:
The mentioned unavailable plugins in the Rviz configuration are obsolete and can be removed.

We use the SemanticKITTI-dataset (http://www.semantic-kitti.org) which consists of the KITTI Odometry Benchmark Velodyne point clouds along with calibration data and the SemanticKitti-labels. You can find the download links here: http://www.semantic-kitti.org/dataset.html#download
The kitti_data_publisher-scripts expects an argument to the folder containing the "sequences"-folder (your folder structure should look like the one for the semantic segmentation task: http://www.semantic-kitti.org/dataset.html#format)

Thank you for expanding the Readme. Just a small thing: The explicit starting of the roscore is not necessary, it is automatically started by the roslaunch command.

Additionally, you seem to have applied a global reformatting on all files. Please refrain from a global reformatting like this since it leads to conflicts with other forks/branches.
Also, you seem to have moved the files in a way that the project should be put directly into the root of the ROS-workspace folder. We want to publish it as a self-contained ROS package. This means it is supposed to go into the src folder of your existing workspace.

@YevgeniyEngineer
Copy link
Author

YevgeniyEngineer commented Jun 10, 2024

Thanks for the clarification.

I will attempt to follow the suggested steps to replay datasets.

I have not used ROS 1 much recently, mostly relying on ROS 2 development, so was not aware that roscore is executed during roslaunch. Thanks for the clarification.

I will undo the folder structure to it's original layout as per your recommendation, leaving just DevContainer and README changes, and removing optional packages from RViz config for repeatability, to simplify PR review.

@YevgeniyEngineer
Copy link
Author

YevgeniyEngineer commented Jun 22, 2024

Hi @Nussknacker

I addressed your comments by only introducing changes that do not affect functional components of the code, but improve the quality of life for further development of code refinement.

Could you please attempt to setup Devcontainer yourself and see if it works for you?

The container will pull graphics drivers from your base system if you have nvidia-smi working and you installed Nvidia container toolkit.

The container offers several advantages over conventional development:

  1. People contributing to the code base will be able to follow C++ style guide setup in .clang-format that is automatically recognised by devcontainer when it is setup
  2. Devcontainer pulls all the ROS build dependencies
  3. It sets up useful extensions in VSCode and binds to the physical volume allowing to browse Kitti dataset located within ~/workspace folder offering extra flexibility
  4. It does not require the base Linux system to be compatible with the installed ROS version, Docker handles all dependencies and sets up virtual operating system

I did not have much time to run the code due to being extremely busy at work recently, but will do so when I find some free time most likely during weekends.

@YevgeniyEngineer
Copy link
Author

YevgeniyEngineer commented Jun 22, 2024

Reading through the code, I noticed there is a bug (should be std::numeric_limits::epsilon()):

Screenshot from 2024-06-22 21-16-55

Also, I am not sure if this is intended, but it is set to std::numeric_limits::min() instead of std::numeric_limits::lowest()
Screenshot from 2024-06-22 21-19-46

@Nussknacker
Copy link
Collaborator

Hi @YevgeniyEngineer,

thanks again for the effort you put into this.
I don't use VSCode personally, so I can't test the Devcontainer myself.
However, I used the dockerfile to set up a container and can confirm that it works within the container.
Only pandas was missing and had to be installed manually. This could also be added to the dockerfile.
Another minor thing is that the workspace folder is owned by root and isn't writable by the devuser account. I don't know if that's intended.
Also, the shell's display variable is set to display :1 via the bashrc. Not sure if that's needed for VSCode, but I had to set it to :0 to get RViz to work.

I had another look at the improved Readme file. It generally looks better now, but I found two small issues:
First, the slash separator between SemanticKITTI and dataset disappeared for the playback command. It is intentional because the directory argument needs the path to the SemanticKIITI dataset, including the dataset directory.
Second, you formatted the example output of the evaluation script into a proper table. This looks nicer, of course, but might give a false impression of what output to expect from the evaluation script. I think I'd prefer the original code block style so it properly reflects the script's actual output.

@Nussknacker
Copy link
Collaborator

Reading through the code, I noticed there is a bug (should be std::numeric_limits::epsilon()):

Screenshot from 2024-06-22 21-16-55

Also, I am not sure if this is intended, but it is set to std::numeric_limits::min() instead of std::numeric_limits::lowest() Screenshot from 2024-06-22 21-19-46

The first usage of min() is intentional, But you are absolutely right in the second case. The maximum ground height is only collected for statistical reasons and not used for the ground segmentation. I might just remove the calculation of this layer since it's incorrect anyways.

@YevgeniyEngineer
Copy link
Author

@Nussknacker I addressed your considerations in the latest commits. Apologies for taking so long

@Nussknacker
Copy link
Collaborator

Hi @YevgeniyEngineer,

thanks again for your work. I tested the updated version of the Dockerfile, but unfortunately, I can not get it to run anymore. The container immediately terminates after launching. This seems to be related to the newly defined entrypoint, which overwrites the /ros_entrypoint.sh script. If I remove the entrypoint, then it works again. Is this new entrypoint necessary?

@YevgeniyEngineer
Copy link
Author

@Nussknacker You are right, the ENTRYPOINT line is not needed, because $DISPLAY can be loaded when executing docker run - fixed

@Nussknacker
Copy link
Collaborator

Hi @YevgeniyEngineer,

now it runs well again :)
However, I saw that you added a symlink in the groundgrid/src folder pointing to its own parent resulting in a file system loop. You also added this to the Readme, but I don't understand what it is supposed to do.
To build groundgrid in the docker container you just need to create a src-directory in the workspace folder ("mkdir src"), then run "catkin init" to initialize the workspace. Now you can just clone the groundgrid git in the src-folder of your workspace: "cd src; git clone https://github.com/dcmlr/groundgrid.git; cd .." and then build it with "catkin build -DCMAKE_BUILD_TYPE=Release groundgrid".
After sourcing your workspace ("source devel/setup.bash") to make the newly build groundgrid package available, you can launch it with roslaunch as described in the Readme.
For further information, you can check the documentation of ROS workspaces here: https://wiki.ros.org/catkin/workspaces

For people familiar with ROS, most of these steps won't be necessary because they already have set up a ROS workspace. For this reason, I did not put this description in the Readme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants