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

Fix 409 based on melodic branch #411

Merged
merged 8 commits into from Jul 10, 2019
Merged

Conversation

Tuebel
Copy link

@Tuebel Tuebel commented Apr 29, 2019

Based #409 on branch melodic.
Also removed boost::mutex and boost::shared_ptr in favor of the std types.

The constructor of the dynamic_reconfigure::Server requires a boost::recursive_mutex so in this version I use the constructor without the mutex parameter. But as far as I can tell from the code of the server the mutex is only required when calling dynamic_reconfigure::Server::updateConfig(const ConfigType &config) which we don't.

Tim Übelhör added 3 commits February 13, 2019 13:43
Using toCvCopy instead of toCvShared (copy is needed anyway).
Base fork on upstream melodic instead of indigo
@knorth55
Copy link
Contributor

knorth55 commented May 14, 2019

I'm not familiar with image_transport, but does this PR work when there is no camera_info topic?
It seems image_transport uses message_filter so that this PR change to require both camera_info and image.
Current node can resize with only image topic or camera_info topic.

@Tuebel
Copy link
Author

Tuebel commented May 15, 2019

@knorth55 image_transport::CameraSubscriber indeed needs both topics synchronized and you do have a good point. We should not brake compatibility, so I will try to implement the best of both by using image_transport::Subscriber for images and a ros::Subscriber for the CameraInfo.

@JWhitleyWork
Copy link
Collaborator

@Tuebel - Since this is a back-port, can you also create a PR to apply any changes you make here to melodic?

@knorth55
Copy link
Contributor

@Tuebel
Thank you for checking it.
Many people use resize nodelet to resize image only, so it is good to support previous behavior.

@JWhitleyAStuff
This PR is a back-port, but current PR changes previous behavior (message synchronizing part).
I think it is better to wait @Tuebel update and merge after it.

@Tuebel
Copy link
Author

Tuebel commented May 15, 2019

@knorth55 I pushed an implementation, which allows to resize the image and the camera_info independently. The images are published and subscribed using the image_transport classes so we still get the functionality of the plugins. I also kept an eye on making only the necessary changes compared to the old resize.cpp.

@JWhitleyAStuff I'm sorry but I don't understand what kind of different PR should I create?

@JWhitleyWork
Copy link
Collaborator

@Tuebel - Nevermind. I was confused. There are many tickets in-flight right now and I got them mixed up.

@JWhitleyWork JWhitleyWork merged commit 4226689 into ros-perception:melodic Jul 10, 2019
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.

None yet

5 participants