image_proc for ros2 #369

ahuizxc · 2018-12-04T05:37:38Z

image_proc for ros2

mjcarroll · 2018-12-08T02:22:49Z

image_proc/CMakeLists.txt

 find_package(OpenCV REQUIRED)
-find_package(Boost REQUIRED COMPONENTS thread)
+find_package(image_geometry REQUIRED)

 if(cv_bridge_VERSION VERSION_GREATER "1.12.0")


This should be unnecessary now, since we are already setting standard to 14

mjcarroll · 2018-12-08T02:23:57Z

image_proc/include/rectify.hpp

-               const image_geometry::PinholeCameraModel& model,
-               ImageSet& output, int flags = ALL) const;
-};
+    // Processing state (note: only safe because we're using single-threaded NodeHandle!)


I don't believe the single-threaded assumption holds anymore, this may need to be protected via mutex.

these code is just same as ros branch did, so i am not change it in ros2 version.

Sorry, to be clear, this was previously using single threaded NodeHandle's in the ROS1/Nodelet paradigm. Because this is now inheriting from an rclcpp::Node, we can't make the same guarantees about this only executing in a single-threaded context (people could add the Node to a multi-threaded executor). Since that assumption no longer holds, I believe that we need to add some locking here to guarantee safety.

image_proc/src/debayer.cpp

image_proc/src/edge_aware.cpp

ahuizxc · 2018-12-10T05:19:15Z

@mjcarroll already create a new commit for your suggestions, thanks :)

klintan · 2019-02-17T03:10:12Z

@ahuizxc I had some issues getting your fork of image_proc working. First it only starts the first node. Second it can't be shutdown using ctrl+c. I'm suspecting some lock/thread/callback issue, but I'm really just a C++ beginner so.

However I rewrote some of it to use the Ros2 built-in namespacing instead of a custom namespace argument which I think makes more sense. Allthough, perhaps removing the parameter service is a bad idea, but I can't really judge that.

this seems to work now:
https://github.com/klintan/image_pipeline/tree/image_proc_for_ros2

just start it using:
ros2 run image_proc image_proc __ns:=/my_namespace

would love to understand what the benefits of the parameter/callback approach ? :) feel free to provide some feedback on my branch as well. If you want to I can create an additional PR to better and easier be able to compare.

ahuizxc · 2019-02-21T02:54:52Z

@ahuizxc I had some issues getting your fork of image_proc working. First it only starts the first node. Second it can't be shutdown using ctrl+c. I'm suspecting some lock/thread/callback issue, but I'm really just a C++ beginner so.

However I rewrote some of it to use the Ros2 built-in namespacing instead of a custom namespace argument which I think makes more sense. Allthough, perhaps removing the parameter service is a bad idea, but I can't really judge that.

this seems to work now:
https://github.com/klintan/image_pipeline/tree/image_proc_for_ros2

just start it using:
ros2 run image_proc image_proc __ns:=/my_namespace

would love to understand what the benefits of the parameter/callback approach ? :) feel free to provide some feedback on my branch as well. If you want to I can create an additional PR to better and easier be able to compare.

I also ported image_view for ros2, I think we should see what advice is given in the community and then focus on the details, thanks :)

klintan · 2019-02-26T23:03:16Z

@ahuizxc I had some issues getting your fork of image_proc working. First it only starts the first node. Second it can't be shutdown using ctrl+c. I'm suspecting some lock/thread/callback issue, but I'm really just a C++ beginner so.
However I rewrote some of it to use the Ros2 built-in namespacing instead of a custom namespace argument which I think makes more sense. Allthough, perhaps removing the parameter service is a bad idea, but I can't really judge that.
this seems to work now:
https://github.com/klintan/image_pipeline/tree/image_proc_for_ros2
just start it using:
ros2 run image_proc image_proc __ns:=/my_namespace
would love to understand what the benefits of the parameter/callback approach ? :) feel free to provide some feedback on my branch as well. If you want to I can create an additional PR to better and easier be able to compare.

I also ported image_view for ros2, I think we should see what advice is given in the community and then focus on the details, thanks :)

Yes great job! However I don't really understand how it relates to this PR (except for it being in the same image_pipeline package) ?

Since this is open source, my definition of community is whoever might be using the package and feel that they want to contribute to it, but perhaps you have a more narrow definition of it? :)

Also you already got a couple of comments from mjcarrol who is one of the owners which should qualify as "advice is given in the community". I think not doing it the ROS2 way merits an explanation. But as I said before, I'm just a C++ novice, so I could very well be missing a very obvious reason for it, should be easy enough to just point it out to me.

Many thanks for putting in the hard work of contributing this, I made good use of it after my minor changes.

yechun1 · 2019-03-15T07:28:59Z

@ahuizxc See the file processor.h has be renamed, but don't know why, and this makes stereo_image_proc package which depend on image_proc fail to find the include file: #include <image_proc/processor.h>

May I suggest to keep the same folder structure and file name while port code. It would be helpful for other package porting on ROS2 without interface changes. And also be easy for code review.

JWhitleyWork · 2019-04-18T20:41:10Z

@yechun1 and @ahuizxc - Is this effort still ongoing? Could we provide a crystal-devel branch for you to both have a common development pipeline? It seems like this PR and #374 were done in isolation.

yechun1 · 2019-04-19T08:58:42Z

Hi @JWhitleyAStuff
There is already ros2 branch for development. @mjcarroll had generated crystal bloom release previously, with merged code on ros2 branch. I think we could use ros2 branch currently.

@ahuizxc has ported image_proc and image_view from ROS1 to ROS2. This PR is for image_proc and #374 is for image_view. These patches are waiting for review, hope someone could give some comments, and then approve and merge.

We took effort on image_pipeline ROS2 porting, here is the status with Todo list. most of the modules ready and waiting for maintainer's review, not sure if you would please help to review the PRs, we'll update the patch if need any changes.

mjcarroll · 2019-04-19T13:51:58Z

We should be able to continue to release for ros2, and I plan on doing it for dashing, earlier testing is always better than later.

@yechun1 and his team have done a great job contributing here, I just haven't had the bandwidth to keep up with reviews. @JWhitleyAStuff if you and your team have some spare cycles, getting these things reviewed and in would be awesome!

JWhitleyWork

See my comments. Also, why are a bunch of files/nodes missing from this PR like rectify, advertisement_checker, processor, crop_decimate, and crop_non_zero?

JWhitleyWork · 2019-04-23T14:25:39Z

image_proc/CMakeLists.txt

-endif()
+install(TARGETS
+        image_proc
+        DESTINATION lib/${PROJECT_NAME})


In ROS1, this would have been installed under lib/ but the convention for ROS2 seems to be bin/ for executables and lib/ only for libraries.

JWhitleyWork · 2019-04-23T14:27:31Z

image_proc/include/rectify.hpp

@@ -31,47 +31,43 @@
 *  ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 *  POSSIBILITY OF SUCH DAMAGE.
 *********************************************************************/
-#ifndef IMAGE_PROC_PROCESSOR_H
-#define IMAGE_PROC_PROCESSOR_H
+#include <thread>


Good work on adding includes for the std:: objects that you use. However, you seem to have lost the header-guard in the process. Please re-add.

JWhitleyWork · 2019-04-23T14:30:48Z

image_proc/src/edge_aware.cpp

@@ -39,8 +39,8 @@
 #define WAVG4(a,b,c,d,x,y)  ( ( ((int)(a) + (int)(b)) * (int)(x) + ((int)(c) + (int)(d)) * (int)(y) ) / ( 2 * ((int)(x) + (int(y))) ) )
 using namespace std;


For the love of Pete, please fix this.

i don't know how to change this, do you mean this code is too long?

JWhitleyWork · 2019-04-23T14:38:11Z

image_proc/src/image_proc.cpp

+// limitations under the License.
+
+#ifdef __clang__
+// TODO(dirk-thomas) custom implementation until we can use libc++ 3.9


The version of clang included with Ubuntu Bionic (target for Crystal) is v7. I think we can likely remove this section and test with clang/libc++.

This functionality has actually been refactored into rcpputils, so I would suggest using that. https://github.com/ros2/rcpputils/blob/master/include/rcpputils/filesystem_helper.hpp

Supporting macOS still presents issues when using std::filesystem, so this is currently the suggested approach.

JWhitleyWork · 2019-04-23T14:39:45Z

image_proc/src/rectify.cpp

+#include <image_geometry/pinhole_camera_model.h>
+#include <cv_bridge/cv_bridge.h>
+#include "visibility.h"
+#include "../include/rectify.hpp"


Please use local-relative paths here (e.g. "rectify.hpp"). CMake should take care of the include paths.

JWhitleyWork · 2019-04-23T14:48:46Z

@yechun1, @mjcarroll, and @ahuizxc - Are you all OK with me rebasing the ros2 branch on the melodic branch? This will incorporate some of the fixes that we've made to image_view and add in the new CI (which I will modify to work with ROS2 afterward). This would mean that you would have to rebase your PRs on the new ros2 branch. Otherwise, I can just manually apply the CI changes to the ros2 branch.

mjcarroll · 2019-04-23T14:56:36Z

image_proc/src/debayer.cpp

-
-void DebayerNodelet::onInit()
+namespace enc = sensor_msgs::image_encodings;
+DebayerNode::DebayerNode()


Nodes should take const rclcpp::NodeOptions& in order to make them composable (replacement for nodelets)

yechun1 · 2019-04-25T01:58:47Z

@JWhitleyAStuff @mjcarroll thank you for take time to review our patches.

@ahuizxc would you please help to check the comments and give some changes? thanks.

JWhitleyWork · 2019-04-25T17:56:25Z

@ahuizxc - Please rebase your branch on the ros2 branch. This will bring in fixes from melodic and the new CI.

JWhitleyWork · 2019-07-07T23:40:44Z

Closing in favor of #426.

image_proc for ros2

fefcd07

This was referenced Dec 5, 2018

Port Image_pipeline on ROS2 #360

Closed

ros2 bloom release #370

Closed

mjcarroll requested changes Dec 8, 2018

View reviewed changes

modified the code as suggested

0486e49

SteveMacenski added the ros2 label Apr 13, 2019

JWhitleyWork requested changes Apr 23, 2019

View reviewed changes

mjcarroll reviewed Apr 23, 2019

View reviewed changes

JWhitleyWork mentioned this pull request Apr 25, 2019

ros2 travis ci build fail #371

Closed

JWhitleyWork force-pushed the ros2 branch from b244db2 to 705d34e Compare April 25, 2019 17:15

klintan mentioned this pull request Jul 7, 2019

Dashing: Image_proc with old PR comments fixed #426

Merged

JWhitleyWork closed this Jul 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image_proc for ros2 #369

image_proc for ros2 #369

ahuizxc commented Dec 4, 2018

mjcarroll Dec 8, 2018

mjcarroll Dec 8, 2018

ahuizxc Dec 10, 2018

mjcarroll Feb 22, 2019

ahuizxc commented Dec 10, 2018

klintan commented Feb 17, 2019

ahuizxc commented Feb 21, 2019

klintan commented Feb 26, 2019

yechun1 commented Mar 15, 2019

JWhitleyWork commented Apr 18, 2019

yechun1 commented Apr 19, 2019

mjcarroll commented Apr 19, 2019

JWhitleyWork left a comment

JWhitleyWork Apr 23, 2019

JWhitleyWork Apr 23, 2019

JWhitleyWork Apr 23, 2019

ahuizxc May 13, 2019

JWhitleyWork Apr 23, 2019

mjcarroll Apr 23, 2019

JWhitleyWork Apr 23, 2019

JWhitleyWork commented Apr 23, 2019

mjcarroll Apr 23, 2019

yechun1 commented Apr 25, 2019

JWhitleyWork commented Apr 25, 2019

JWhitleyWork commented Jul 7, 2019

		@@ -39,8 +39,8 @@
		#define WAVG4(a,b,c,d,x,y) ( ( ((int)(a) + (int)(b)) * (int)(x) + ((int)(c) + (int)(d)) * (int)(y) ) / ( 2 * ((int)(x) + (int(y))) ) )
		using namespace std;

image_proc for ros2 #369

image_proc for ros2 #369

Conversation

ahuizxc commented Dec 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuizxc commented Dec 10, 2018

klintan commented Feb 17, 2019

ahuizxc commented Feb 21, 2019

klintan commented Feb 26, 2019

yechun1 commented Mar 15, 2019

JWhitleyWork commented Apr 18, 2019

yechun1 commented Apr 19, 2019

mjcarroll commented Apr 19, 2019

JWhitleyWork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JWhitleyWork commented Apr 23, 2019

Choose a reason for hiding this comment

yechun1 commented Apr 25, 2019

JWhitleyWork commented Apr 25, 2019

JWhitleyWork commented Jul 7, 2019