[RSDK-7954] Add PCD support and organize encoding helpers #41
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since we put PCD processing in the |
hexbabe
changed the title
seanavery
reviewed
Jun 18, 2024
| return response; | ||
| } | ||
|
|
||
| std::vector<unsigned char> rsPointsToPCDBytes(const rs2::points& points, const rs2::frame& colorFrame) { |
There was a problem hiding this comment.
[q] Do you have any resources/examples or more info on how the pcd transformation is done here?
There was a problem hiding this comment.
I followed this resource https://pointclouds.org/documentation/tutorials/pcd_file_format.html
seanavery
reviewed
Jun 18, 2024
There was a problem hiding this comment.
LGTM.
Seems great to have pointclouds native to the module now!
- Makes sense that the
get_point_cloudcalls would not affect the performance of background image loop thread.- Looks like cpp sdk grpc stuff uses CompletionQueue async interface (ref), so long response times here should not affect anything on the main thread either.
- Any info you have to help me grok
rsPointsToPCDByteshelper addition would be awesome (Not up to speed yet with librealsense/pc formatting).
bhaney
requested changes
Jun 18, 2024
There was a problem hiding this comment.
Thanks for the addition - we'll want to make sure that the points created by the function are actually scaled to real-world distances. Also the 4MB grpc message limit is just a default, it is not a real limit, you don't want to destroy data with no way of recovering it. Potentially could create a new optional config for scaling down point clouds, or telling the user to use a smaller resolution of image
randhid
approved these changes
Jun 19, 2024
hexbabe
commented
Jun 20, 2024
randhid
approved these changes
Jun 20, 2024
seanavery
approved these changes
Jun 20, 2024
Co-authored-by: randhid <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RSDK-7954
Before removing
join_color_depth, we wanted to try out adding PCD support directly into the Realsense module since that was the main use case forjoin_color_depthanyways. After some trial and error, it looks like adding this logic intoget_point_cloudis the right path forward. Since we do PCD computation using the latest depth frame and color frame on invocation, it doesn't slow down the logic of frame loop or get_image/images.I implemented the above in this PR along with some re-organization of the encoding helpers into a separate file since the main file is getting quite large.
As for testing, I tested
little_endian_depthset totrueand the PCD came out fine. I tested getting color images and viewing point clouds on the App stream with 640x480 and 1280x720 with both sensors. I also verified that using only the depth sensor works for non-color PCDs.