-
Notifications
You must be signed in to change notification settings - Fork 120
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
Declare depth compression params in a per-topic namespace #110
Conversation
ROS1In ROS1 noetic it was still using the correct value (dynamic reconfigure provided). image_transport_plugins/compressed_depth_image_transport/src/compressed_depth_publisher.cpp Lines 69 to 77 in cca6781
ROS2Looks like the bug you mention was introduced during large API reworking for ROS2: Consequence
Uninitialized. Which may often happen to be 0 by pure luck. The intricacies of C++ initialization that lead to this are funny but irrelevant. Good catch @Kettenhoax |
- runtime reconfiguration - <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.compressedDepth.png_level` instead of `image.png_level` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter - synchronize deprecated changes to new ones - this is necessary for dynamic reconfigure - add ROS1 like ranges for parameters (min/max) - some cleanup - remove unnecessary includes, using statements, etc. Default png_level was left the same as for compressed_image_transport - this is OpenCV default (3) - the deprecated paramterer name clashes here with compressed_image_transport - I don't really want to consider if there is a race between plugin loading for default value - side notes: - in ROS2 default was 9 - 9 is not usable for real-time processing on most machines - in ROS1 default is now 1 - 3 is typically usable for real-time processing - this should probably be made even with ROS1 after removing deprecated parameters Related to ros-perception#140 Builds up on ros-perception#110
- runtime reconfiguration - <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.compressedDepth.png_level` instead of `image.png_level` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter - synchronize deprecated changes to new ones - this is necessary for dynamic reconfigure - add ROS1 like ranges for parameters (min/max) - some cleanup - remove unnecessary includes, using statements, etc. Default png_level was left the same as for compressed_image_transport - this is OpenCV default (3) - the deprecated paramterer name clashes here with compressed_image_transport - I don't really want to consider if there is a race between plugin loading for default value - side notes: - in ROS2 default was 9 - 9 is not usable for real-time processing on most machines - in ROS1 default is now 1 - 3 is typically usable for real-time processing - this should probably be made even with ROS1 after removing deprecated parameters Related to #140 Builds up on #110 (cherry picked from commit dfdf48b)
This PR was included in #145 and merged. @Kettenhoax Apologies I didn't get around to reviewing this PR earlier, but thank you for your contribution! |
This change declares the parameters of
compressedDepth
in the same namespace as done bycompressed
since 30d0401Indirectly fixes #89, because the depth_quantization parameter of the publisher was inadvertently assigned to the wrong config field before.
Presumably the quantization parameter remained 0 or uninitialized, which could result in the bad values.
Edit
I used this adapted gazebo depth example to test:
depth_camera_ros.zip