-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add PerceptionFeatures library and implement ArucoDetector class #159
Conversation
0f66887
to
24dc1ff
Compare
There seem to be two issues in the CI:
Do you have any idea? Does the memory leak occurs also in your system? |
Probably we need to install |
It seems that the valgrind error is not a memory leak, but rather a use of not initialized memory in the opencv 3.2 version available in Ubuntu. Probably we can just add that error to a valgrind suppression file, or add one if we don't have it (see robotology/idyntree#352 for an example of how to add it). |
We already have it in https://github.com/dic-iit/bipedal-locomotion-framework/blob/e43cc1827fa1e2062f117fc37f1bf986cf6b5fc3/cmake/valgrind-linux.supp , so we just need to add the appropriate suppressions there. |
5662cbd
to
8fda15e
Compare
How do we choose that in the CI? because I noticed in vcpkg opencv port that it is not part of the default features. Will this work?
UPDATE Seems like the windows CI passes? The only failing CI is Ubuntu for the valgrind memory leak for which we need to suppress the valgrind check. |
That will probably work, but it takes a lot of time, and we need to propagate that eventually to https://github.com/robotology/robotology-vcpkg-ports . As it seems to be that conda-forge's OpenCV has instead aruco, can we just add the install of opencv in the conda-forge based CI, and disable this component in the vcpkg-based Windows build? |
Note that in the C++ CI windows and conda-forge CI, the ArucoDetectorTest is not being called. Probably, it's not being compiled due a missing |
Damn, librealsense on conda-forge on the other hand does not support Windows (see conda-forge/librealsense-feedstock#3). Probably it is just easier to not test this stuff on Windows for the time being. |
I skimmed through https://wiki.wxwidgets.org/Valgrind_Suppression_File_Howto to understand better the changes. Added the suppression in f8c02f3 |
src/Perception/include/BipedalLocomotion/Perception/Features/ArucoDetector.h
Outdated
Show resolved
Hide resolved
src/Perception/include/BipedalLocomotion/Perception/Features/ArucoDetector.h
Outdated
Show resolved
Hide resolved
src/Perception/include/BipedalLocomotion/Perception/Features/ArucoDetector.h
Outdated
Show resolved
Hide resolved
a6d0a96
to
daa3310
Compare
CI status related to
|
The CI failure is:
while the suppression is:
As I don't see any |
I added the failure related suppression in 4924ce2 and it didn't seem to work
I don't understand how |
How are you generating this suppression files? |
Are you using https://wiki.wxwidgets.org/Valgrind_Suppression_File_Howto#How_to_make_a_suppression_file ? Probably we can add |
Compiled in
|
This failure is different from the CI one. I would quickly add that command to CI to get the suppression corresponding to CI. |
Solid advice (I will remember this). Thanks @traversaro. Looks like it worked.
|
As far as I remember we disable Windows compilation of this component due to the fact that by default opencv in vcpkg does not have the contrib component with the aruco stuff? |
Ah okay, yeah. So it is automatically taken care of by |
c8c700f
to
07263b4
Compare
@GiulioRomualdi could you take a look at this PR and merge it if you are ok with the changes? |
07263b4
to
d67154e
Compare
d67154e
to
c552e59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment
This PR implements a
PerceptionFeatures
(Perception -> Features (Image features) open to renaming suggestions) library and a ArcuoDetector class (requires OpenCV as dependency).The ArucoDetector class is implemented as an Advanceable gives as an output a container of
ArucoMarkerData
which more or less has the same structure as aContactBase
class (Landmark features from images seen by a camera can be seen analogous to contacts made by the robot).Currently only implements individual markers detection.
cc @HosameldinMohamed @gabrielenava