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.

Sign up for GitHub

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

Jump to bottom

924 introduce default depth param #943

Merged
merged 7 commits into from
Feb 21, 2024
Merged

924 introduce default depth param #943

merged 7 commits into from
Feb 21, 2024

Conversation

PhilippPolterauer
Copy link
Contributor

@PhilippPolterauer PhilippPolterauer commented Feb 17, 2024 •
edited
Loading

this pull request attempts to fix #924

changes:

  • changed the argument name range_max which was missleading to invalid_depth which captures the true meaning of the paramter
  • Introduction of a parameter called invalid_depth, which is the value that will be assigned to points that are not valid according to DepthTraits<T>::valid
  • small fixes regarding use of non const shared ptr references
  • changed for loop from int to uint32_t avoiding many type casts

Open:

  • UB when reinterpret casting the pointcloud data?
  • introduction of a true "max_depth" or "max_range" paramter

Comment:
i think the term range and depth is not always used consistently, my understanding is
range meaning point to point distance, and depth point to camera plane distance

@PhilippPolterauer PhilippPolterauer marked this pull request as ready for review February 17, 2024 11:21
@PhilippPolterauer PhilippPolterauer mentioned this pull request Feb 17, 2024
Closed
@PhilippPolterauer
Copy link
Contributor Author

in the meantime i further research the UB topic, and after watching Video i am now fairly certain, that most use of reinterpret_cast is actually UB. Should i raise a new issue dedicated to this?

@mikeferguson
Copy link
Member

Yeah, go ahead and open a new issue where we can further investigate the reinterpret_cast

mikeferguson
mikeferguson requested changes Feb 18, 2024
View reviewed changes
Copy link
Member

@mikeferguson mikeferguson left a comment

Choose a reason for hiding this comment

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

A couple of minor items - once we address those, this can be merged (I'm going to just double check things by firing up my robot on this branch tomorrow to make sure the default behavior hasn't changed even though it all looks correct)

depth_image_proc/include/depth_image_proc/conversions.hpp Outdated Show resolved Hide resolved
depth_image_proc/src/point_cloud_xyz.cpp Show resolved Hide resolved
@PhilippPolterauer
Copy link
Contributor Author

Thanks for the feedback. As soon as i have time, i will implement the changes.

@PhilippPolterauer
Copy link
Contributor Author

  • added invalid_depth to parameter description in the docs
  • removed comment
  • added the invalid_depth to the other non radial nodes as well.

If there is something more to do, please let me know!

@mikeferguson
Copy link
Member

This looks good - thanks for the contribution - just waiting on the final CI to pass

@PhilippPolterauer
Copy link
Contributor Author

i just clicked update branch which i think was a mistake, since now the ci is waiting again. sorry for the confusion :)

@mikeferguson mikeferguson merged commit 432f134 into ros-perception:rolling Feb 21, 2024
3 checks passed
@PhilippPolterauer PhilippPolterauer deleted the 924-introduce-default-depth-param branch February 24, 2024 18:57
CursedRock17 added a commit to CursedRock17/image_pipeline that referenced this pull request Feb 26, 2024
@CursedRock17
Adding a parameter for camera calibration
7f5d1d1 
Signed-off-by: CursedRock17 <[email protected]>

Revert "Initial Merging: Setting up ImageSaverNode"

This reverts commit 4534c95.

DisparityNode: replace full_dp parameter with sgbm_mode (ros-perception#945)

Previously, only the SGBM and HH modes were allowed

add invalid_depth param (ros-perception#943)

Add option to set all invalid depth pixels to a specified value, typically the maximum range.

 * Updates convertDepth parameter name and optimizes use of the parameter.
 * Updates PointCloudXYZ, PointCloudXYZI, and PointCloudXYZRGB with new invalid_depth parameter

Adding scale parameter to camera calibrator

Signed-off-by: CursedRock17 <[email protected]>
Kotochleb pushed a commit to Kotochleb/image_pipeline that referenced this pull request May 27, 2024
@PhilippPolterauer
add invalid_depth param (ros-perception#943)
aa251da 
Add option to set all invalid depth pixels to a specified value, typically the maximum range.

 * Updates convertDepth parameter name and optimizes use of the parameter.
 * Updates PointCloudXYZ, PointCloudXYZI, and PointCloudXYZRGB with new invalid_depth parameter
mikeferguson added a commit that referenced this pull request Aug 16, 2024
@mikeferguson
fix signature issue from #943 (#1018)
a7c0b09 
Without this, we get

```
symbol lookup error: /home/ubr/jazzy/install/depth_image_proc/lib/libdepth_image_proc.so: undefined symbol: _ZN16depth_image_proc10convertRgbERKSt10shared_ptrIKN11sensor_msgs3msg6Image_ISaIvEEEES0_INS2_12PointCloud2_IS4_EEEiiii

c++filt _ZN16depth_image_proc10convertRgbERKSt10shared_ptrIKN11sensor_msgs3msg6Image_ISaIvEEEES0_INS2_12PointCloud2_IS4_EEEiiii
depth_image_proc::convertRgb(std::shared_ptr<sensor_msgs::msg::Image_<std::allocator<void> > const> const&, std::shared_ptr<sensor_msgs::msg::PointCloud2_<std::allocator<void> > >, int, int, int, int)
```
mergify bot pushed a commit that referenced this pull request Aug 16, 2024
@mikeferguson @mergify
fix signature issue from #943 (#1018)
999d961 
Without this, we get

```
symbol lookup error: /home/ubr/jazzy/install/depth_image_proc/lib/libdepth_image_proc.so: undefined symbol: _ZN16depth_image_proc10convertRgbERKSt10shared_ptrIKN11sensor_msgs3msg6Image_ISaIvEEEES0_INS2_12PointCloud2_IS4_EEEiiii

c++filt _ZN16depth_image_proc10convertRgbERKSt10shared_ptrIKN11sensor_msgs3msg6Image_ISaIvEEEES0_INS2_12PointCloud2_IS4_EEEiiii
depth_image_proc::convertRgb(std::shared_ptr<sensor_msgs::msg::Image_<std::allocator<void> > const> const&, std::shared_ptr<sensor_msgs::msg::PointCloud2_<std::allocator<void> > >, int, int, int, int)
```

(cherry picked from commit a7c0b09)
mikeferguson added a commit that referenced this pull request Aug 16, 2024
@mergify @mikeferguson
fix signature issue from #943 (backport #1018) (#1023)
ac6707b 
Without this, we get

```
symbol lookup error: /home/ubr/jazzy/install/depth_image_proc/lib/libdepth_image_proc.so: undefined symbol: _ZN16depth_image_proc10convertRgbERKSt10shared_ptrIKN11sensor_msgs3msg6Image_ISaIvEEEES0_INS2_12PointCloud2_IS4_EEEiiii

c++filt _ZN16depth_image_proc10convertRgbERKSt10shared_ptrIKN11sensor_msgs3msg6Image_ISaIvEEEES0_INS2_12PointCloud2_IS4_EEEiiii
depth_image_proc::convertRgb(std::shared_ptr<sensor_msgs::msg::Image_<std::allocator<void> > const> const&, std::shared_ptr<sensor_msgs::msg::PointCloud2_<std::allocator<void> > >, int, int, int, int)
```

This is an automatic backport of pull request #1018 done by
[Mergify](https://mergify.com).

Co-authored-by: Michael Ferguson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeferguson mikeferguson mikeferguson approved these changes

Assignees
No one assigned
Labels
ros2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

range_max sensor value introduced but not used
2 participants
@PhilippPolterauer @mikeferguson