Initial commit: Exchange boolean full_dp to integer sgbm_mode. #945
Conversation
There was a problem hiding this comment.
Thank you for the contribution - code looks good - we just need to update the documentation:
- Update the parameters here: https://github.com/ros-perception/image_pipeline/blob/rolling/stereo_image_proc/doc/components.rst (see https://github.com/ros-perception/image_pipeline/blob/rolling/image_proc/doc/components.rst for examples of how to document the 4 mode variables)
- Add a note about the (breaking) change to params in the changes page: https://github.com/ros-perception/image_pipeline/blob/rolling/image_pipeline/doc/changelog.rst
|
@mikeferguson Shouldn't the edited changes page be https://github.com/ros-perception/image_pipeline/blob/rolling/stereo_image_proc/CHANGELOG.rst ? |
stereo_image_proc/CHANGELOG.rst
Outdated
| 5.0.1 (####-##-##) | ||
| ------------------ | ||
| * Breaking change: deleted full_dp parameter and created new parameter sgbm_mode to select the algorithm for stereo matching (`https://github.com/ros-perception/image_pipeline/issues/582`) | ||
| * Contributors: Pablo David Aranda |
There was a problem hiding this comment.
ah - this part is autogenerated - we can revert the changes to this file (it will get added when I do the next release - but editing it manually like this actually breaks the release tools).
The file I was asking to update is the actual documentation that we use for big changes between releases (so that when you upgrade from say humble/iron->jazzy, you know what changed) - that gets rendered here: http://docs.ros.org/en/rolling/p/image_pipeline/changelog.html - and comes from the file previously mentioned here https://github.com/ros-perception/image_pipeline/blob/rolling/image_pipeline/doc/changelog.rst - and yes, it's in a different "package" - because image_pipeline is the "metapackage" that rolls up all of these. When the Jazzy release gets closer, we'll end up also linking this page into the main documentation - in a page like this: http://docs.ros.org/en/rolling/Releases/Release-Iron-Irwini.html#changes-since-the-humble-release
I've added notes about this to #946 - where I'm going to update our CONTRIBUTIONS.md file so these sort of things are better documented
There was a problem hiding this comment.
Okay, in the meanwhile I will add it to that file in the section that describes the changes between Iron and Jazzy, but the changes are being merged into the rolling branch, should I change the target?
There was a problem hiding this comment.
Rolling is the correct branch - Jazzy doesn't exist yet (the buildfarm will come on line in probably 3-4 weeks, and then we can make initial releases into Jazzy, although we typically wait to fork off a branch until we are really forced to since every extra branch adds to the maintainer overhead)
There was a problem hiding this comment.
Perfect, all the changes are done then.
PabloDArandaR
force-pushed
the
PabloDArandaRMODE_SGBM_3WAY
branch
from
615f741
to
e4faced
Compare
Signed-off-by: CursedRock17 <mtglucas1@gmail.com> 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 <mtglucas1@gmail.com>
This PR aims to solve the issue #582. Currently, only the SGBM and HH modes are allowed. To change this behavior, I switched the full_dp boolean parameter to the sgbm_mode parameter. This could be problematic for some users that are currently using the full_dp parameter.