Skip to content

Conversation

sssarana
Copy link

  • add dependency on grid_map_core;
  • fix build with NONFREE algorithms for humble-devel;
  • Replace Mat objects with UMat to use with OpenCL;

This will automatically use OpenCL GPU if it is available.

float d3 = util2d::getDepth(depth, corners[i][2].x*rgbToDepthFactorX, corners[i][2].y*rgbToDepthFactorY, true, 0.02f, true);
float d4 = util2d::getDepth(depth, corners[i][3].x*rgbToDepthFactorX, corners[i][3].y*rgbToDepthFactorY, true, 0.02f, true);
cv::Mat depthMat;
depth.copyTo(depthMat);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
depth.copyTo(depthMat);
depthMat = depth.getMat(cv::ACCESS_READ);

copy only if needed

Copy link
Author

Choose a reason for hiding this comment

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

The depthMat = depth.getMat(cv::ACCESS_READ); caused some lifecycle issues for me earlier, so I wouldn't trust it. The reason for that is the original object that we create a shallow copy from gets deleted, so the depthMat gets deleted also.

package.xml Outdated
@@ -22,7 +21,7 @@
<!-- <depend>libproj-dev</depend> needed due to error in vtk6 (kinetic)-->
<depend>libsqlite3-dev</depend>
<depend>liboctomap-dev</depend>
<!-- <depend>grid_map_core</depend> --> <!-- till this PR is released https://github.com/ANYbotics/grid_map/pull/499 -->
<depend>grid_map_core</depend> <!-- till this PR is released https://github.com/ANYbotics/grid_map/pull/499 -->
Copy link
Member

@matlabbe matlabbe May 18, 2025

Choose a reason for hiding this comment

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

I think we still need to wait for ANYbotics/grid_map#519 to be released, we may change the comment to

Suggested change
<depend>grid_map_core</depend> <!-- till this PR is released https://github.com/ANYbotics/grid_map/pull/499 -->
<!-- <depend>grid_map_core</depend> --> <!-- till this PR is released https://github.com/ANYbotics/grid_map/pull/519 -->

Note that dependency in package.xml is useful only if you use rosdep to install dependencies, the workaround if you want grid_map_core support locally is to install explicitly grid_map_core and cmake should be able to find it.

Copy link
Author

Choose a reason for hiding this comment

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

I created a PR to fix grid_map_core here: ANYbotics/grid_map#521, but it didn't get reviewed yet. Though I think you can apply these fixes yourself if you want to test everything properly.

@matlabbe
Copy link
Member

matlabbe commented May 18, 2025

I kinda forgot about UMat when OpenCV3 came out, thinking it was already integrated inside cv::Mat, though it should be explicitly used. I put some comments above (and check also CI errors). It looks like for most functions, we would need to convert to cv::InputArray / cv::OutputArray to avoid unnecessary copies between GPU and CPU. That would be a bigger PR though. I'll try to take time to test at least your changes to see what is the impact on those functions (using OpenCL or not) compared to old code, then merge it for now (and do UMat support for all other detectors later).

@sssarana
Copy link
Author

Thanks for your feedback. I fixed the errors and removed unnecessary logs now, let me know if anything else is needed.
Using the cv::InputArray/cv::OutputArray is a good idea, it will also make implementation for UMat support on other detectors a bit easier.
Thanks.

@sssarana
Copy link
Author

Hello! Any update on this?
Let me know if something is needed too.
Thanks.

@matlabbe
Copy link
Member

matlabbe commented Jun 2, 2025

I didn't have time to test the impact of these changes. In the mean time, if you can rebase this MR on master branch isntead of humble-devel it would be ideal.

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