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

Add img_hash module #15

Merged
merged 2 commits into from
Apr 1, 2024
Merged

Add img_hash module #15

merged 2 commits into from
Apr 1, 2024

Conversation

mdeleau
Copy link
Contributor

@mdeleau mdeleau commented Mar 31, 2024

Hello,

First of all, thank you for this project which allows me to kickstart an old project idea of mine. And I think it will help a lot of other developers.

I needed the img_hash module so I took the liberty to implement it.
I am a beginner in flutter and this is my first contribution to open source so do not hesitate to point at anything wrong in what I did.

I tried to stay as close as possible to the gocv implementation but I think some things can be improved, especially the tests.

@rainyl
Copy link
Owner

rainyl commented Mar 31, 2024

Great! Thanks for you contributions, I will take a look at it recently.

BTW, actually, the current implementation is not robust so I am working on rewriting the implementations of gocv, including adding the ability to catch exceptions and avoid using bare pointers, this will make the exceptions more friently (#13 ), and I am trying to make the APIs more concise and similar to opencv-python, these are breaking changes and may be released in v1.0.0.

I will push recent commits to catch-exceptions branch, you can take a look at it and change the current implementation of img_hash to the new styles if you have more time.

@rainyl rainyl self-requested a review March 31, 2024 15:19
Copy link
Owner

Choose a reason for hiding this comment

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

Minor please consider:

The current implementation is "static", the hash class instances will be freed after computation, maybe we can do someting like

CVD_TYPEDEF(cv::RNG, RNG)
and provide operation interfaces like

opencv_dart/src/core/core.h

Lines 518 to 531 in 76c542d

CvStatus Rng_New(RNG *rval);
CvStatus Rng_NewWithState(uint64_t state, RNG *rval);
void Rng_Close(RNG *rng);
CvStatus TheRNG(RNG *rval);
CvStatus SetRNGSeed(int seed);
CvStatus RNG_Fill(RNG rng, Mat mat, int distType, double a, double b, bool saturateRange);
CvStatus RNG_Gaussian(RNG rng, double sigma, double *rval);
CvStatus RNG_Uniform(RNG rng, int a, int b, int *rval);
CvStatus RNG_UniformDouble(RNG rng, double a, double b, double *rval);
CvStatus RNG_Next(RNG rng, uint32_t *rval);
CvStatus RandN(Mat mat, Scalar mean, Scalar stddev);
CvStatus RandShuffle(Mat mat);
CvStatus RandShuffleWithParams(Mat mat, double iterFactor, RNG rng);
CvStatus RandU(Mat mat, Scalar low, Scalar high);

This can also solve: https://github.com/mdeleau/opencv_dart/blob/ccba658be755ddd7d5551daea9691070cc961347/lib/src/contrib/img_hash.dart#L93

But for current status it is fine and feasible, like I said, in the future we will refactor this.

Copy link
Owner

Choose a reason for hiding this comment

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

I reviewed it briefly and it's quiet great, no much to change and I'm going to merge it in to main branch.

I will be grateful if you are willing to refactor it for #13 , to do this please open PRs into catch-exceptions branch, but if you have no time, don't worry, I will finish it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, thank you for the review. I do not think I will have the time to refactor it during the next weeks. But I will try to refactor it as soon as I can.

@rainyl rainyl merged commit 0d11814 into rainyl:main Apr 1, 2024
1 of 2 checks passed
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.

2 participants