Skip to content
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

Dashing: Image_proc with old PR comments fixed #426

Merged
merged 10 commits into from
Jul 16, 2019
Merged

Dashing: Image_proc with old PR comments fixed #426

merged 10 commits into from
Jul 16, 2019

Conversation

klintan
Copy link
Contributor

@klintan klintan commented Jul 7, 2019

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
Copy link
Collaborator

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.

@JWhitleyWork JWhitleyWork mentioned this pull request Jul 7, 2019
@klintan
Copy link
Contributor Author

klintan commented Jul 7, 2019

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 Outdated Show resolved Hide resolved
// Make sure we don't enter connectCb() between advertising and assigning to pub_XXX
boost::lock_guard<boost::mutex> lock(connect_mutex_);
Copy link
Contributor Author

@klintan klintan Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

image_proc/include/rectify.hpp Outdated Show resolved Hide resolved
image_proc/include/visibility.h Outdated Show resolved Hide resolved
image_proc/include/visibility.h Outdated Show resolved Hide resolved
image_proc/package.xml Show resolved Hide resolved
image_proc/src/debayer.cpp Outdated Show resolved Hide resolved
image_proc/src/debayer.cpp Outdated Show resolved Hide resolved
image_proc/src/debayer.cpp Outdated Show resolved Hide resolved
image_proc/src/image_proc.cpp Outdated Show resolved Hide resolved
image_proc/src/rectify.cpp Outdated Show resolved Hide resolved
@yechun1
Copy link
Contributor

yechun1 commented Jul 8, 2019

@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
Copy link
Collaborator

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?

@yechun1 yechun1 mentioned this pull request Jul 8, 2019
7 tasks
@klintan
Copy link
Contributor Author

klintan commented Jul 9, 2019

@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
Copy link
Contributor Author

klintan commented Jul 9, 2019

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.

@klintan
Copy link
Contributor Author

klintan commented Jul 10, 2019

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
Copy link
Collaborator

@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
Copy link
Collaborator

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

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@mjcarroll
Copy link
Contributor

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
Copy link
Contributor Author

klintan commented Jul 12, 2019

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

@SteveMacenski
Copy link
Member

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
Copy link
Contributor Author

klintan commented Jul 13, 2019

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
Copy link
Member

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

@klintan
Copy link
Contributor Author

klintan commented Jul 13, 2019

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

Created this issue: #430

@SteveMacenski SteveMacenski dismissed their stale review July 13, 2019 05:06

Fixed issues but cant test

@SteveMacenski
Copy link
Member

Works for me. I wont block

@JWhitleyWork
Copy link
Collaborator

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

@klintan
Copy link
Contributor Author

klintan commented Jul 16, 2019

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 :/

@JWhitleyWork JWhitleyWork merged commit 343fa8f into ros-perception:ros2 Jul 16, 2019
jacobperron added a commit that referenced this pull request Dec 11, 2019
The implementation was removed (accidentally?) in #426.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@@ -1,130 +0,0 @@
/*********************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jacobperron added a commit that referenced this pull request Dec 11, 2019
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants