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

Bev version #116

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

BV-OpenSource
Copy link

No description provided.

@cjue
Copy link
Contributor

cjue commented Dec 13, 2021

@BV-OpenSource Thank you for your mail regarding this PR. I am interested in merging all your changes but might want to suggest some minor changes in the coming days. How soon do you want to finish the ROS release?

@BV-OpenSource
Copy link
Author

@BV-OpenSource Thank you for your mail regarding this PR. I am interested in merging all your changes but might want to suggest some minor changes in the coming days. How soon do you want to finish the ROS release?

As soon as possible. Feel free to ask any question regarding our changes.

@cjue
Copy link
Contributor

cjue commented Dec 31, 2021

  • Which OS, CUDA and maybe ROS versions are you currently working with?
  • Also which GPU versions?
  • Are there setups that you know are still non-working with your latest commits?

I would like to keep the known issues and known working sections as up to date as possible.
I'll spend more time on all the changes in the coming week.

@BV-OpenSource
Copy link
Author

We are using this package with Ubuntu 18.04 and 20.04, so ROS melodic and noetic and CUDA 10.2.
Our developers computer's are equipped with NVIDIA GPU but our robots are using Jetson Nano, TX2 and Xavier NX.
So far, all the code used for our purpose (Collision Detection and/or Avoidance) is working as pretended.

@BV-OpenSource
Copy link
Author

@cjue do you have any update on this request? Is there anything we can help?
Feel free to contact us.

@cjue
Copy link
Contributor

cjue commented Jan 11, 2022

Thanks for reaching out again. I now plan to first have a release with essentially just this PR and then add in our internal changes afterwards.

I saw that you also have the melodic-dev in your repo.

What about the two commits on there?

  • Is the "ubuntu 20.04 compatibility" commit needed?
  • Why did you disable the icl_core tests: just for speed or because of issues?

@BV-OpenSource
Copy link
Author

Even though is not needed, the code is compatible with both Ubuntu 18.04 and Ubuntu 20.04, I think.
Just for speed, no issues was found.

@@ -199,7 +199,7 @@ ICMAKER_CONFIGURE_PACKAGE()

###############################################################################
# Build examples
ADD_SUBDIRECTORY(src/examples)
# ADD_SUBDIRECTORY(src/examples)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you disable this to save compilation time or because of errors?
I would prefer introducing a CMake build option for this.

Copy link
Author

Choose a reason for hiding this comment

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

As mentioned before, no issues was found. I agree with that suggestion.

Copy link

@wxmerkt wxmerkt left a comment

Choose a reason for hiding this comment

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

Hi @BV-OpenSource and @cjue,
Thank you very for your work and getting this integrated and ready for binary release to the ROS buildfarm. We are heavy users of GPU-Voxels and are planning to make a PR with additional features in the near-term as well.

Apologies for "jumping in" - a few comments/questions below before this hits the official ROS buildfarm and thus gains wider distribution :-). I used the suggestions feature for easier possible integration. Please ignore comments/questions if irrelevant

Many thanks for all your hard work - this is highly valuable to the wider community,
Wolfgang

git clone https://gitlab.com/libeigen/eigen.git
sudo cp -r signature_of_eigen3_matrix_library /usr/include/eigen3
sudo cp -r unsupported/ /usr/include/eigen3
sudo cp -r Eigen/ /usr/include/eigen3
Copy link

Choose a reason for hiding this comment

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

We shouldn't overwrite system-installed packages. What is this required for?

Copy link
Author

Choose a reason for hiding this comment

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

As mentioned in README, to fix "math_functions.hpp not found" on Ubuntu 18.04 with CUDA 10.0.
If you have a better way to fix this, I'm okay with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember that some way of introducing a newer version of Eigen was needed when Ubuntu 18.04 came out.

We have a script that clones Eigen into a subfolder so you can add it to the build itself, for 18.04, I prefer that over the system overwrite also.

Copy link

@wxmerkt wxmerkt Jan 11, 2022

Choose a reason for hiding this comment

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

You could check the Eigen version and conditionally use FetchContent or ExternalProject_Add to fetch if required?

@@ -199,7 +199,7 @@ ICMAKER_CONFIGURE_PACKAGE()

###############################################################################
# Build examples
ADD_SUBDIRECTORY(src/examples)
# ADD_SUBDIRECTORY(src/examples)
Copy link

Choose a reason for hiding this comment

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

I'd suggest adding an OPTION(BUILD_EXAMPLES ON) so it doesn't need to be commented

Suggested change
# ADD_SUBDIRECTORY(src/examples)
IF(BUILD_EXAMPLES)
ADD_SUBDIRECTORY(src/examples)
ENDIF(BUILD_EXAMPLES)

@@ -61,6 +62,10 @@ class ProbVoxelMap: public TemplateVoxelMap<ProbabilisticVoxel>,

size_t collideWith(const voxelmap::BitVectorVoxelMap* map, float coll_threshold = 1.0, const Vector3i &offset = Vector3i());
size_t collideWith(const voxelmap::ProbVoxelMap* map, float coll_threshold = 1.0, const Vector3i &offset = Vector3i());

virtual void moveInto(ProbVoxelMap& dest, const Vector3f offset) const;
Copy link

Choose a reason for hiding this comment

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

Should Vector3f be a const-ref?

Copy link
Author

Choose a reason for hiding this comment

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

Not that I'm disagreeing with you but, why not?

Copy link

Choose a reason for hiding this comment

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

Assuming it's an Eigen type, it's recommended to pass by const-ref

Copy link
Author

Choose a reason for hiding this comment

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

Now I see what you were saying, I thought that '&' was there. You're right.

@@ -61,6 +62,10 @@ class ProbVoxelMap: public TemplateVoxelMap<ProbabilisticVoxel>,

size_t collideWith(const voxelmap::BitVectorVoxelMap* map, float coll_threshold = 1.0, const Vector3i &offset = Vector3i());
size_t collideWith(const voxelmap::ProbVoxelMap* map, float coll_threshold = 1.0, const Vector3i &offset = Vector3i());

virtual void moveInto(ProbVoxelMap& dest, const Vector3f offset) const;
virtual void move(Voxel* dest_data, const Voxel* src_data, const Vector3f offset) const;
Copy link

Choose a reason for hiding this comment

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

Similarly here, should Vector3f a const-ref?

@@ -60,6 +60,12 @@ This is important if you are still using ROS Indigo and need to compile without
- If the ROS dependency was found, but the GPU-Voxels URDF features are still unabailable, run `source /opt/ros/YOUR_ROS_DISTRO/setup.bash` before running cmake.
Copy link

Choose a reason for hiding this comment

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

Suggested change
- If the ROS dependency was found, but the GPU-Voxels URDF features are still unabailable, run `source /opt/ros/YOUR_ROS_DISTRO/setup.bash` before running cmake.
- If the ROS dependency was found, but the GPU-Voxels URDF features are still unavailable, run `source /opt/ros/$ROS_DISTRO/setup.bash` before running cmake.

@cjue
Copy link
Contributor

cjue commented Jan 11, 2022

Apologies for "jumping in" - a few comments/questions below before this hits the official ROS buildfarm and thus gains wider distribution :-). I used the suggestions feature for easier possible integration. Please ignore comments/questions if irrelevant

Thank you for your input, feel free to add comments and suggestions!

@BV-OpenSource
Copy link
Author

@cjue Sorry if we are being annoying but we want to publish our code and we depend on yours.

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.

3 participants