search

ros-perception / opencv_apps

http://wiki.ros.org/opencv_apps
64 stars 70 forks source link

indigo branch is not only tested on indigo #42

Closed furushchev closed 7 years ago

furushchev commented 7 years ago

Just noticed that indigo branch is main branch of this repository, and pull request to the branch is also tested on hydro, jade and kinetic. In kinetic it seems to be use opencv3 (not opencv2) by default and it has not compatibility among them. How should I put application for kinetic?

My proposal is:

Also I should use on PR2 which means creating backport branch to hydro (I understand hydro is already EOL.)

k-okada commented 7 years ago

1, I do not think I can maintain several branches, and you can see many example which someone start proposing developing distro-based branch and fail to maintain them.

curl -L
https://raw.githubusercontent.com/ros/rosdistro/master/jade/distribution.yaml
| grep version
curl -L
https://raw.githubusercontent.com/ros/rosdistro/master/kinetic/distribution.yaml
| grep version
  1. opencv_apps code should not depends on ros version, because basically newer ros distro should not break old ros API so if you write ROS code that runs on hydro, that code should run with kinetic

3 you can see following example to switch opencv2/opencv3, but I recommend not to write code heavily depends on new features of opencv3, unless you can keep maintain that code.

https://github.com/ros-perception/opencv_apps/pull/35/files https://github.com/ros-perception/opencv_apps/blob/b341701666725c0d56c970219ab6bf8c255f4b1c/src/nodelet/simple_compressed_example_nodelet.cpp#L98

◉ Kei Okada

2016-09-27 17:00 GMT+09:00 Furushchev notifications@github.com:

Just noticed that indigo branch is main branch of this repository, and pull request to the branch is also tested on hydro, jade and kinetic. In kinetic it seems to be use opencv3 (not opencv2) by default and it has not compatibility among them. How should I put application for kinetic?

My proposal is:

-

split branches according with ros distro:

  • indigo(-devel)

    • jade(-devel)

    - kinetic(-devel)

    split with opencv version:

  • opencv2
    • opencv3 (this may break on incompatibility between opencv minor versions 2.x, 3.x)

Also I should use on PR2 which means creating backport branch to hydro (I understand hydro is already EOL.)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ros-perception/opencv_apps/issues/42, or mute the thread https://github.com/notifications/unsubscribe-auth/AAeG3OOuiu1ap0tdWSNZBkK46S2eTh8tks5quM0PgaJpZM4KHXM6 .

furushchev commented 7 years ago

@k-okada Thank you for clarifying. At this time, I managed to add some codes to keep compatibility. Please note that opencv3 has significant change on header file structure. e.g. in opencv2, #include <opencv2/opencv.hpp> includes all features, but to use face detection / recognition in opencv3, we must also append #include <opencv2/face.hpp>. https://github.com/ros-perception/opencv_apps/pull/41/files#diff-9caf2824838f089a7f76dc27ca88ed9dR74

k-okada commented 7 years ago

such node could not compile on opencv2, so we will add ifdef within cmake, not source code.

◉ Kei Okada

2016-09-28 15:01 GMT+09:00 Furushchev notifications@github.com:

@k-okada https://github.com/k-okada Thank you for clarifying. At this time, I managed to add some codes to keep compatibility. Please note that opencv3 has significant change on header file structure. e.g. in opencv2, #include <opencv2/opencv.hpp> includes all features, but to use face detection / recognition in opencv3, we must also append #include <opencv2/face.hpp>.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ros-perception/opencv_apps/issues/42#issuecomment-250079325, or mute the thread https://github.com/notifications/unsubscribe-auth/AAeG3MYRMkXxIyEFLSyR2twPli4M4WAYks5qugLCgaJpZM4KHXM6 .