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

Almost a major refactor #21

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

kunaltyagi
Copy link

@kunaltyagi kunaltyagi commented Jun 26, 2018

This code uses C++11 and CMake 3.1. I don't know what the compatibility requirements are. If you are not able to merge this, no issues.

Modified the code to have the following:

  • reuse of ImageCapture logic elsewhere (not dependent on ROS)
  • looping to enable endless debugging of videos, images and image_sequences
  • error count to allow for resetting streams in case of loss of connection (to be used together with looping)
  • some code cleanup (and addition of dirty macro)
  • allow for easier support for ROS2 in future (only utility.hpp needs to be changed for that)
  • removed usage of experimental boost thread sync_queue

@awesomebytes
Copy link
Member

Hello. Sorry for the late reply, I was away for a few weeks. Thank you very much for your efforts.

As you say... this is mainly a complete rewrite. I don't feel confident merging it. At least we'd need to test every old possible use case to make sure it still works as before. Said that, I have the following concerns:

  • About CMake 3.1, Ubuntu 14.04 has version 2.8.12.2, and this package is supported on ROS Indigo, so I would rather not merge that. Is it really necessary?

  • About C++11, I'm not a C++ expert, so I don't know if that has any backwards compatibility problems either.

I do like the capability of looping and also pausing the play. Couldn't that be implemented in the previous code base?

@kunaltyagi
Copy link
Author

kunaltyagi commented Jul 11, 2018

Regarding the versions

  • CMake 3.1 is required for setting -std=c++11 in the best practices. We can always use set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
  • I've removed Boost as a dependency, hence the need for C++11. AFAIK, it requires a fairly recent compiler (though as per this, the compilers have been around for a long time)
  • Indigo support has long gone, but I understand your wish to keep it in place. Would a nomenclature like other ROS repositories work? (indigo-devel for indigo, and master for the rest?)

Regarding the refactor:

  • No, it's not requried. I'm using the innards for my work and I refactored the code anyways. Thought the refactor might be useful to the community as well.
  • It's pretty simple to add the changes I made without refactoring

TLDR: This PR is just a refactor with minor changes. Makes it easier for developers, but not required to be merged upstream. The minor changes can be cherry-picked and merged.

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.

2 participants