Declare depth compression params in a per-topic namespace #110
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
ROS1In ROS1 noetic it was still using the correct value (dynamic reconfigure provided). 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 |
bmegli
added a commit
to Extend-Robotics/image_transport_plugins
that referenced
this pull request
Apr 27, 2023
- 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
mergify bot
pushed a commit
that referenced
this pull request
May 7, 2023
- 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! |
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.
This change declares the parameters of
compressedDepthin the same namespace as done bycompressedsince 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