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 #750 making it compatible with Opencv 4.4 #762

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

Conversation

wandenberg
Copy link

No description provided.

@adin234
Copy link

adin234 commented Oct 8, 2020

@justadudewhohacks can confirm this fixes the problem with 4.4

@pavds
Copy link

pavds commented Dec 24, 2020

@mykola-mokhnach sorry for the delay. Yes, just tested and it worked with the OpenCV 4.5

Thanks, I also checked with the OpenCV 4.5.0 and everything works fine 👍

@Maximvdw
Copy link

Definitely a much anticipated PR +1

@wandenberg
Copy link
Author

Now with support to Opencv4.5.2 wandenberg@91bc118

@UrielCh
Copy link
Contributor

UrielCh commented Jan 6, 2022

Nice patch, I merged it to my fork
If you want to make further changes try them from my fork, It's way faster to use, the build script log has been improved, and each OpenCV version build has its build directory.

to get the best result, define an OPENCV_BUILD_ROOT environment variable outside your node_modules directory; Personally, I use:

export OPENCV_BUILD_ROOT=~/opencv

The OpenCV build script can be called from the command line without an extra environment variable.

Tested on Linux / MacOS X M1 / Windows.

Currently, only 2 samples failed.

@piercus
Copy link
Collaborator

piercus commented Apr 12, 2022

@wandenberg Sorry for the delay, but since i'm having contributor rights, i will try to review this :-)

The PR is great, but there is a lack of testing coverage for OpenCV 4.5 on travis

I feel it would be good to add 4.5.x version in travis in the matrix in https://github.com/justadudewhohacks/opencv4nodejs/blob/master/.travis.yml

Can you please also add some tests specifically related to your changes on 4.5 using cvVersionGreaterEqual(4, 5, 0) you can find example in https://github.com/justadudewhohacks/opencv4nodejs/blob/master/test/tests/tracking/TrackerTests.js

Thanks for your help

Copy link
Collaborator

@piercus piercus left a comment

Choose a reason for hiding this comment

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

See my comment in #762 (comment)

@wandenberg wandenberg force-pushed the master branch 6 times, most recently from e375c97 to 3bcea9f Compare June 30, 2022 08:28
@wandenberg
Copy link
Author

@piercus Sorry for the delay. I tried to change the version for OpenCV4.5.5 on travis.yml and on appveyour.yml, but this made the test fail. Probably I am missing something on the configuration.
Regarding adding tests, as far as I can see, it is not needed since the changes were only on how to call the C++ code and this is tested on the current suite, we're only missing running the tests with the correct OpenCV version like you requested.
I noticed that a new project was created and the code of this merge request is already incorporated there and tested with the proper versions, so I will let this one as is :)

@piercus
Copy link
Collaborator

piercus commented Jun 30, 2022

Ok thanks for the follow up !

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

7 participants