search

microsoft / Azure_Kinect_ROS_Driver

A ROS sensor driver for the Azure Kinect Developer Kit.
MIT License
295 stars 220 forks source link

Fix Unordered Point Clouds #241

Open exo-core opened 2 years ago

exo-core commented 2 years ago

Fixes

Description of the changes:

Fixed the wrong size information in published point clouds (see #227 and #163).

Required before submitting a Pull Request:

I tested changes on:

christian-rauch commented 2 years ago

For my understanding, does this essentially "reshape" the unstructured point cloud (list of points) into a structured point cloud (grid of points in the image domain)?

exo-core commented 2 years ago

For my understanding, does this essentially "reshape" the unstructured point cloud (list of points) into a structured point cloud (grid of points in the image domain)?

Yes. Actually the correct width and height values are already being set in lines 669/670 and 727/728, respectively, but the call to pcd_modifier.resize(n) resets the point cloud back to n times 1.

christian-rauch commented 2 years ago

For my understanding, does this essentially "reshape" the unstructured point cloud (list of points) into a structured point cloud (grid of points in the image domain)?

Yes. Actually the correct width and height values are already being set in lines 669/670 and 727/728, respectively, but the call to pcd_modifier.resize(n) resets the point cloud back to n times 1.

I see. In this case, you could also move all of: https://github.com/microsoft/Azure_Kinect_ROS_Driver/blob/c0742b9e470c9e688d796029f10cb52e1a763a4a/src/k4a_ros_device.cpp#L727-L730 to after the pcd_modifier.resize(point_count); so that setting point_cloud->height and point_cloud->width does not appear twice.

christian-rauch commented 2 years ago

Also... can you squash your commits? :-) Your third commit reverts your second commit. Such commits should not appear at all in the git history. And I think you can also squash your last commit into the first commit as they touch the same things.

v4hn commented 1 year ago

Thank you for tracking this down @exo-core ! It works.

@christian-rauch It's rather annoying that this is not the default behavior upstream yet. Can't you just squash-commit and maybe add your own cleanup commit?

christian-rauch commented 1 year ago

Thank you for tracking this down @exo-core ! It works.

@christian-rauch It's rather annoying that this is not the default behavior upstream yet. Can't you just squash-commit and maybe add your own cleanup commit?

Which commit? I don't have a commit in this PR and I also cannot squash or change other people's PRs.

exo-core commented 1 year ago

I guess we are a little spoiled from Gitlab, which has a "squash commits" option in the merge menu... (no clue what kind of back magic it does in the background)

I'm quite busy, finalizing my Ph.D. thesis at present, but I'll try to squeeze in a some time next week to update my PRs. For this one it should be straightforward. #240 seems to need some more thought and discussion though...

christian-rauch commented 1 year ago

I'm quite busy, finalizing my Ph.D. thesis at present

Good luck with your thesis writeup :-)

exo-core commented 1 year ago

I guess the PR should be fine now ;-)

Good luck with your thesis writeup :-)

Thanks!

CalaW commented 1 year ago

The process died after modifying and compiling the package in accordance with this commit. However I published ordered point clouds by simply remove the following line:

pcd_modifier.resize(point_count);

I am not familiar with point cloud representation in ROS, so I'm wondering if it's a valid solution.

v4hn commented 1 year ago

I guess we are a little spoiled from Gitlab, which has a "squash commits" option in the merge menu

The same is true for github. Also, unless the pr author explicitly disabled it, repository authors are able to push (and force-push) to remote branches that belong to pull-requests and can thus clean up history themselves. Even if the PR author disabled it, it's straight-forward to open a new PR for it (but a bit more work).

I'm quite busy, finalizing my Ph.D. thesis at present

Hang in there and congrats on getting that far! :-)

exo-core commented 1 year ago

The process died after modifying and compiling the package in accordance with this commit. However I published ordered point clouds by simply remove the following line:

pcd_modifier.resize(point_count);

I am not familiar with point cloud representation in ROS, so I'm wondering if it's a valid solution.

Thanks for pointing this out. I was able to get a camera in the lab today and could reproduce the issue.

exo-core commented 1 year ago

Ok, so turned out the initial resize wasn't save to remove after all, since resizing at the end invalidated the iterators. Guess, skipping the resize entirely isn't a good idea either, so I just moved the iterator initializations behind the resize operation.

Sorry, I didn't run detailed tests before; it seemed trivial and I did not have a camera at hand...