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

Humble #27

Draft
wants to merge 65 commits into
base: master
Choose a base branch
from
Draft

Humble #27

wants to merge 65 commits into from

Conversation

spelletier1996
Copy link

Changes required to build in ros2

@peci1
Copy link
Owner

peci1 commented Apr 16, 2024

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.

@peci1 peci1 mentioned this pull request Apr 16, 2024
@peci1 peci1 linked an issue Apr 16, 2024 that may be closed by this pull request
@peci1 peci1 added this to the ROS 2 milestone Apr 16, 2024
@peci1 peci1 self-assigned this Apr 16, 2024
@peci1 peci1 added enhancement New feature or request help wanted Extra attention is needed labels Apr 16, 2024
@spelletier1996
Copy link
Author

spelletier1996 commented Apr 16, 2024

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
Copy link
Owner

peci1 commented Apr 16, 2024

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 spelletier1996 marked this pull request as draft April 17, 2024 02:12
@spelletier1996
Copy link
Author

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
Copy link
Author

spelletier1996 commented Apr 17, 2024

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
Copy link
Owner

peci1 commented Apr 17, 2024

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
Copy link
Author

spelletier1996 commented May 1, 2024

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
Copy link
Owner

peci1 commented May 1, 2024

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
Copy link
Author

  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
Copy link
Owner

peci1 commented May 1, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ROS2 plans
3 participants