Skip to content

Fix #750 making it compatible with Opencv 4.4 - #762

Open
wandenberg wants to merge 3 commits into
justadudewhohacks:masterfrom
wandenberg:master
Open

Fix #750 making it compatible with Opencv 4.4#762
wandenberg wants to merge 3 commits into
justadudewhohacks:masterfrom
wandenberg:master

Conversation

@wandenberg

Copy link
Copy Markdown

No description provided.

@adin234

adin234 commented Oct 8, 2020

Copy link
Copy Markdown

@justadudewhohacks can confirm this fixes the problem with 4.4

@pavds

pavds commented Dec 24, 2020

Copy link
Copy Markdown

@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
Copy Markdown

Definitely a much anticipated PR +1

@wandenberg

Copy link
Copy Markdown
Author

Now with support to Opencv4.5.2 wandenberg@91bc118

@UrielCh

UrielCh commented Jan 6, 2022

Copy link
Copy Markdown
Contributor

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

piercus commented Apr 12, 2022

Copy link
Copy Markdown
Collaborator

@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 piercus left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

piercus commented Jun 30, 2022

Copy link
Copy Markdown
Collaborator

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.

7 participants