Dashing: Image_proc with old PR comments fixed #426

klintan · 2019-07-07T22:48:49Z

This is pretty much just a copy of this PR #369.

However all (? ) of the comments and requested changes should be done. Since the other one has not been touched since Apr 25 I thought maybe I could just do those fixes so that we can get some basic image_proc functionality into image_pipeline.

The major thing that was mentioned by @JWhitleyAStuff is that there are a couple of nodes missing. I'm thinking maybe we could put these in a separate PR ?

If you think we should just continue the other PR I'm all fine with that, would just love to get this going.
Ping:
@mjcarroll
@JWhitleyAStuff
@yechun1
@ahuizxc

Let me know if there is anything in this one that still needs changing (if my point above is ok regarding the missing nodes)

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

I'll give it a look. Thanks for doing this work! I'm OK merging without the additional nodes to get the functionality in. Let me make a real review of it and I'll get back to you.

klintan · 2019-07-07T23:42:57Z

Gaah, realized I probably messed up a bit on the rebase, this will prob not have the changes from Melodic. Will take a look to incorporate some of this, at least the new circle is in.

No worries. One small thing I noticed though; If you put the executable in /bin instead of /lib (which according to comments in the other PR is suppose to be only libraries) the executable does not show up in the executables list. Maybe something is missing the CMakeLists for the /bin executable to work properly? Otherwise we'll have to change to /lib I guess.
Thanks for the prompt reply and looking through the code :)

image_proc/CMakeLists.txt

klintan · 2019-07-08T00:01:48Z

image_proc/src/debayer.cpp

  // Make sure we don't enter connectCb() between advertising and assigning to pub_XXX
-  boost::lock_guard<boost::mutex> lock(connect_mutex_);


This should probably not be removed ? Otherwise it will not start publishing unless a reconfiguration request is made. Let me know if I'm missing something here, but will add until I hear otherwise.

See the 2 lines below this as well.

JWhitleyWork

See comments.

image_proc/include/rectify.hpp

image_proc/include/visibility.h

image_proc/package.xml

image_proc/src/debayer.cpp

image_proc/src/image_proc.cpp

image_proc/src/rectify.cpp

yechun1 · 2019-07-08T05:42:18Z

@klintan thanks for taking uncompleted image_proc porting work. Sorry that we fail to find time to complete the porting since we are busy on other tasks.

As previously check, stereo_image_proc depends on image_proc and require #include <image_proc/processor.h>, from this PR, the file processor.h has be renamed, so that stereo_image_proc will not find the include file while porting to ros2. May I suggest to keep the same INCLUDE file name (or processor.hpp) while port code. It would be helpful for other package porting on ROS2 without interface changes.

JWhitleyWork · 2019-07-08T05:50:47Z

Also, @klintan, just one more thing: Dashing has deprecated several calls and building this branch comes with many deprecation warnings. Could you take care of these, at least for image_proc since we are now targeting Dashing with this code?

klintan · 2019-07-09T02:26:27Z

@klintan thanks for taking uncompleted image_proc porting work. Sorry that we fail to find time to complete the porting since we are busy on other tasks.

As previously check, stereo_image_proc depends on image_proc and require #include <image_proc/processor.h>, from this PR, the file processor.h has be renamed, so that stereo_image_proc will not find the include file while porting to ros2. May I suggest to keep the same INCLUDE file name (or processor.hpp) while port code. It would be helpful for other package porting on ROS2 without interface changes.

of course I'll put it back.

klintan · 2019-07-09T02:26:58Z

Also, @klintan, just one more thing: Dashing has deprecated several calls and building this branch comes with many deprecation warnings. Could you take care of these, at least for image_proc since we are now targeting Dashing with this code?

Yepp I'll make sure to fix that as well.

image_proc/include/edge_aware.h

klintan · 2019-07-10T05:38:25Z

Also, @klintan, just one more thing: Dashing has deprecated several calls and building this branch comes with many deprecation warnings. Could you take care of these, at least for image_proc since we are now targeting Dashing with this code?

I didn't see any of these for image_proc when I'm compiling with Dashing? I'll double check, however if you have time and you still see those, perhaps you could just post a screenshot and i'll go through them.

edit: Could it have been for depth_image_proc you saw the errors?

JWhitleyWork · 2019-07-10T15:00:58Z

@klintan - You're right, the errors are only in depth_image_proc and image_rotate. I'll put in a separate PR to fix those.

JWhitleyWork · 2019-07-10T15:05:23Z

I think the only comment I had left was regarding the extern "C" code in visibility.h.

SteveMacenski

I don't have the cycles the next month or so to properly test this, I'll defer to other maintainers for that.

image_proc/src/edge_aware.cpp

mjcarroll · 2019-07-10T22:28:05Z

The failure in Cpr__image_pipeline is expected. It will resolve once this is released into dashing. I'll probably hold off on any sort of release of this until this PR lands.

klintan · 2019-07-12T20:49:38Z

I think all PR feedback should be addressed, but let me know if I missed something or if there is anything else needed.

SteveMacenski · 2019-07-12T21:17:01Z

What's going on with crop_decimate, resize, etc tools that were just deleted in this PR? Are those functions covered somewhere else now?

klintan · 2019-07-13T04:49:02Z

What's going on with crop_decimate, resize, etc tools that were just deleted in this PR? Are those functions covered somewhere else now?

No not at the moment, I asked if we could put them in a separate PR, mostly to get some basic functionality in soonish. But if that is not an alternative I'll get them into to this one.

The major thing that was mentioned by @JWhitleyAStuff is that there are a couple of nodes missing. I'm thinking maybe we could put these in a separate PR ?

SteveMacenski · 2019-07-13T04:54:41Z

Got it, that’s fine. Can we just file a ticket for it so they’re not lost to time?

klintan · 2019-07-13T05:03:14Z

Got it, that’s fine. Can we just file a ticket for it so they’re not lost to time?

Created this issue: #430

Fixed issues but cant test

SteveMacenski · 2019-07-13T05:07:09Z

Works for me. I wont block

JWhitleyWork · 2019-07-15T18:37:30Z

Check the one comment on the copyrights. That's all I've got left.

…llback

klintan · 2019-07-16T06:41:19Z

Check the one comment on the copyrights. That's all I've got left.

Ok updated the copyright. Had to do some additional changes to make it work. I tested it out now and get proper output on all topics in RVIZ2.

Maybe you need to take a look at the last commit. Sorry about that :/

The implementation was removed (accidentally?) in #426. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2019-12-11T02:18:57Z

image_proc/src/libimage_proc/processor.cpp

@@ -1,130 +0,0 @@
-/*********************************************************************


Can anyone comment as to why this file was removed?

In ROS 1, the implementation is used by stereo_image_proc. I've opened a PR to add back the implementation #484

* Add back processor.cpp The implementation was removed (accidentally?) in #426. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Port processor.cpp for ROS 2 * Added library back to CMakeLists.txt * Renamed image_proc executable to it's former name * Made changes for library to compile * Added new dependency to rcutils for logging * Linted to satisfy tests Signed-off-by: Jacob Perron <jacob@openrobotics.org>

image_proc updates, PR comments

21b557f

JWhitleyWork mentioned this pull request Jul 7, 2019

image_proc for ros2 #369

Closed

klintan commented Jul 8, 2019

View reviewed changes

image_proc/CMakeLists.txt Outdated Show resolved Hide resolved

klintan commented Jul 8, 2019

View reviewed changes

JWhitleyWork requested changes Jul 8, 2019

View reviewed changes

klintan added 3 commits July 7, 2019 20:42

Addressed PR comments

fe6169b

fixed a missing variable change

5a96413

added file removed by mistake

be2d6d0

yechun1 mentioned this pull request Jul 8, 2019

Port Image_pipeline on ROS2 #360

Closed

7 tasks

mjcarroll reviewed Jul 9, 2019

View reviewed changes

image_proc/include/edge_aware.h Outdated Show resolved Hide resolved

added back copyright holders, and added processor.h

26b54f2

SteveMacenski previously requested changes Jul 10, 2019

View reviewed changes

image_proc/src/edge_aware.cpp Outdated Show resolved Hide resolved

klintan and others added 2 commits July 10, 2019 20:49

updated copyright notice to be consistent

8b50d26

Removing unused visibility.h.

0b7c5f5

Tested and updated for ROS2 components

9b4bf5b

klintan added 2 commits July 15, 2019 23:35

removed getNumSubscribe parts because of missing subscriber status ca…

ec86941

…llback

update copyright

de77eec

JWhitleyWork approved these changes Jul 16, 2019

View reviewed changes

JWhitleyWork merged commit 343fa8f into ros-perception:ros2 Jul 16, 2019

jacobperron added a commit that referenced this pull request Dec 11, 2019

Add back processor.cpp

00bc038

The implementation was removed (accidentally?) in #426. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron mentioned this pull request Dec 11, 2019

Port image processor library to ROS 2 #484

Merged

jacobperron reviewed Dec 11, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashing: Image_proc with old PR comments fixed #426

Dashing: Image_proc with old PR comments fixed #426

klintan commented Jul 7, 2019

JWhitleyWork commented Jul 7, 2019

klintan commented Jul 7, 2019 •

edited

Loading

klintan Jul 8, 2019 •

edited

Loading

JWhitleyWork left a comment

yechun1 commented Jul 8, 2019

JWhitleyWork commented Jul 8, 2019

klintan commented Jul 9, 2019

klintan commented Jul 9, 2019

klintan commented Jul 10, 2019 •

edited

Loading

JWhitleyWork commented Jul 10, 2019

JWhitleyWork commented Jul 10, 2019

SteveMacenski left a comment

mjcarroll commented Jul 10, 2019

klintan commented Jul 12, 2019

SteveMacenski commented Jul 12, 2019

klintan commented Jul 13, 2019

SteveMacenski commented Jul 13, 2019

klintan commented Jul 13, 2019

SteveMacenski commented Jul 13, 2019

JWhitleyWork commented Jul 15, 2019

klintan commented Jul 16, 2019

jacobperron Dec 11, 2019

		// Make sure we don't enter connectCb() between advertising and assigning to pub_XXX
		boost::lock_guard<boost::mutex> lock(connect_mutex_);

		@@ -1,130 +0,0 @@
		/*********************************************************************

Dashing: Image_proc with old PR comments fixed #426

Dashing: Image_proc with old PR comments fixed #426

Conversation

klintan commented Jul 7, 2019

JWhitleyWork commented Jul 7, 2019

klintan commented Jul 7, 2019 • edited Loading

klintan Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

JWhitleyWork left a comment

Choose a reason for hiding this comment

yechun1 commented Jul 8, 2019

JWhitleyWork commented Jul 8, 2019

klintan commented Jul 9, 2019

klintan commented Jul 9, 2019

klintan commented Jul 10, 2019 • edited Loading

JWhitleyWork commented Jul 10, 2019

JWhitleyWork commented Jul 10, 2019

SteveMacenski left a comment

Choose a reason for hiding this comment

mjcarroll commented Jul 10, 2019

klintan commented Jul 12, 2019

SteveMacenski commented Jul 12, 2019

klintan commented Jul 13, 2019

SteveMacenski commented Jul 13, 2019

klintan commented Jul 13, 2019

SteveMacenski commented Jul 13, 2019

JWhitleyWork commented Jul 15, 2019

klintan commented Jul 16, 2019

jacobperron Dec 11, 2019

Choose a reason for hiding this comment

klintan commented Jul 7, 2019 •

edited

Loading

klintan Jul 8, 2019 •

edited

Loading

klintan commented Jul 10, 2019 •

edited

Loading