opencv4nodejs
opencv4nodejs copied to clipboard
Fix #750 making it compatible with Opencv 4.4
@justadudewhohacks can confirm this fixes the problem with 4.4
@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 👍
Definitely a much anticipated PR +1
Now with support to Opencv4.5.2 https://github.com/wandenberg/opencv4nodejs/commit/91bc118a43da570638db466e51cf56dbbf615b9e
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.
@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
@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 :)
Ok thanks for the follow up !