peci1 / robot_body_filter

Filters the robot's body out of point clouds and laser scans.
BSD 3-Clause "New" or "Revised" License
84 stars 22 forks source link

Humble #27

Open spelletier1996 opened 7 months ago

spelletier1996 commented 7 months ago

Changes required to build in ros2

peci1 commented 7 months ago

Thank you for the kickoff!

I can't merge the PR in its current state as it has several problems:

  1. As the PR is pretty large, you have to make sure it is "minimally invasive". I.e. do not perform any changes on lines that need not be touched (e.g. wrapping lines, changing whitespace etc.)
  2. The automatic tests must not be disabled and they must pass (do not care about the red flags from Github CI, they run on ROS 1; but locally, the tests have to pass).
  3. You've basically disabled most of the configuration of the filter. That is not acceptable. Generally, as ROS 2 works quite differently with parameters, maybe there needs to be a broader discussion about how to configure the filter via ROS params. Do you know any other filter implementation already on ROS 2?
  4. We will need to upstream the obb library into geometric_shapes. ROS 1 already has it and I don't foresee any problems porting it to ROS 2. The local implementation in this library was only a workaround and I definitely don't want to drag this weight again in ROS 2. Do you want to lead the porting?
  5. The build files (package.xml, CMakeLists.txt) also need to be edited in a minimally invasive manner. If possible, I'm against using ament_cmake_auto as it is doing too much automagic.
  6. If you want to remove a line, remove it and do not comment it out. I understand commenting out is practical while you're working on the PR. But no commented-out lines should get into the final version of the PR.
spelletier1996 commented 7 months ago

Thank you for the kickoff!

I can't merge the PR in its current state as it has several problems:

  1. As the PR is pretty large, you have to make sure it is "minimally invasive". I.e. do not perform any changes on lines that need not be touched (e.g. wrapping lines, changing whitespace etc.)
  2. The automatic tests must not be disabled and they must pass (do not care about the red flags from Github CI, they run on ROS 1; but locally, the tests have to pass).
  3. You've basically disabled most of the configuration of the filter. That is not acceptable. Generally, as ROS 2 works quite differently with parameters, maybe there needs to be a broader discussion about how to configure the filter via ROS params. Do you know any other filter implementation already on ROS 2?
  4. We will need to upstream the obb library into geometric_shapes. ROS 1 already has it and I don't foresee any problems porting it to ROS 2. The local implementation in this library was only a workaround and I definitely don't want to drag this weight again in ROS 2. Do you want to lead the porting?
  5. The build files (package.xml, CMakeLists.txt) also need to be edited in a minimally invasive manner. If possible, I'm against using ament_cmake_auto as it is doing too much automagic.
  6. If you want to remove a line, remove it and do not comment it out. I understand commenting out is practical while you're working on the PR. But no commented-out lines should get into the final version of the PR.

Apologies, I meant to have this be a merge into my own forked version of master to review changes. But I guess this serves the same purpose and we can hopefully get your input on certain changes. Let me know if you'd like me to close this PR or if there's a more appropriate way to go about this.

Thank you for taking a look, this is very much a work in progress (hence all the commented out code) and we will hopefully be tackling the points you've mentioned in the coming days.

peci1 commented 7 months ago

I'd like to keep this PR open so that other contributors could see there already is something in the works. We still haven't swtiched over to ROS 2, so I won't be of much help with testing. We're thinking about slowly starting the transition with Jazzy.

I'm not sure how many changes there were in Iron and Jazzy in the relevant libraries. Have you also tried this on Rolling? Or do you intend to test it after the Humble port is ready? It would be great to support both Rolling and LTS.

spelletier1996 commented 7 months ago

We are going to focus on the humble port since our system is currently humble and we have short/medium term goals that we'd like to hit, but we can try to do Rolling testing when the humble port is done. We will also be upgrading to Jazzy when that is released as well.

spelletier1996 commented 7 months ago

Quick question on your point 1), I use the clang style formatting, did you use a specific formatting style for the project that I could refer to to undo some of the whitespace/wrap changes? If there's a settings file you use that could also be very helpful like a .clang-format used for your project

peci1 commented 7 months ago

I haven't used any automated formatter.

I usually stick close to Google style guide combined with Gazebo style guide, but not strictly. And line length 120 in C++ files.

In my other projects, I started using roslint configured with

set(ROSLINT_CPP_OPTS "--extensions=h,hpp,hh,c,cpp,cc;--linelength=120;--filter=\
    -build/header_guard,-readability/namespace,-whitespace/braces,-runtime/references,\
    -build/c++11,-readability/nolint,-readability/todo,-legal/copyright")

However, robot_body_filter is one of the oldest pieces and has its origin in two other projects it developed from, so I assume there is some inconsistency in the style.

The preferred option is if you tell your formatter to only touch lines that were edited in the PR. Most formatters can be configured this way.

spelletier1996 commented 7 months ago

Hoping you can help with something, Ive run into an issue where this line Won't compile in the ported version. The overloaded method for OOB seems to be missing and I'm not why that would be the case.

The equivelent method for AABB objects seems to work fine if you take a look here The uncommented code is building while the commented is not.

Any ideas as to what would cause something like this? Missing Dependency? where could I find the implementation for the overloaded method for OOB computeBoundingBox?

peci1 commented 7 months ago

That's a part of https://github.com/ros-planning/geometric_shapes . As I said, https://github.com/ros-planning/geometric_shapes/pull/210 (and a few followup bugfixes) will need to be ported to ROS 2.

spelletier1996 commented 6 months ago

Was hoping to get some insight into an issue we are running into. We are getting a segfault error when attempting to call: RobotBodyFilter.cpp: Line 1339 addShape const auto shapeHandle = this->shapeMask->addShape( collisionShape, containsTestInflation.scale, containsTestInflation.padding, shadowTestInflation.scale, shadowTestInflation.padding, bsphereInflation.scale, bsphereInflation.padding, bboxInflation.scale, bboxInflation.padding, false, shapeName); RayCastingShapeMask: Line 358 addShape result.contains = ShapeMask::addShape(shape, containsScale, containsPadding);

When I call shape.print() I get Mesh[vertices=224164, triangles=450252] which would I assume indicate that our meshes are being loaded? Could the mesh be too big?

It does employ the geometric_shapes library using cloneAt although I don't believe we modified any of the related code.

It is crashing at the first link (base_link in our case). I can provide the URDF if needed, here is a link to the branch I am currently working from.

Just hoping that you might be able to point us towards what may be the cause of the issue, if you need more info please let me know.

peci1 commented 6 months ago

It'd help to post the whole backtrace, ideally with ros-humble-geometric-shapes-dbgsym package installed, or - even better - with geometric_shapes built in debug mode.

Generally, I see two possibilities that quickly came to my mind:

  1. Concurrency issues (but I'm not sure there is actually a possibility for concurrent access to addShape)
  2. Something injects unexpected compiler flags and the program segfaults because of wrong memory layout pointing at false segfault places. This can happen if you e.g. pass -mavx2 or -msse3 flags to gcc: the binary-installed libraries are built without these flags, but the source-built things are built with them. And these flags can e.g. change the memory layout/alignment of Eigen objects.
spelletier1996 commented 6 months ago
  1. Maybe running the filter in a multi-threaded callback group is causing issues here? I can try a single threaded callback and mutexing the add shapes methods?
  2. Looked through compile commands for any mention of avx or sse but couldn't seem to find anything

Here is a full stack trace, the only missing debug symbols are for the moveit library which I have not been able to aquire even after installing all moveit packages. I will attempt to build them locally. Here is also a core dump if desired. image image image

peci1 commented 6 months ago

It mostly looks like a bug in geometric_shapes. Would you be able to separate the construction and cloning of the ConvexMesh object into a separate C++ executable? Or could you try with a simpler mesh model?