Skip to content

core: C-API cleanup: RNG algorithms in core(4.x)#26259

Merged
asmorkalov merged 3 commits intoopencv:4.xfrom
Kumataro:fix26258
Oct 8, 2024
Merged

core: C-API cleanup: RNG algorithms in core(4.x)#26259
asmorkalov merged 3 commits intoopencv:4.xfrom
Kumataro:fix26258

Conversation

@Kumataro
Copy link
Copy Markdown
Contributor

@Kumataro Kumataro commented Oct 5, 2024

  • replace CV_RAND_UNI and NORMAL to cv::RNG::UNIFORM and cv::RNG::NORMAL.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

- remove cvRandArr() and cvRandShuffle()
- remove CV_RAND_UNI and NORMAL
@opencv-alalek opencv-alalek added category: core cleanup Code cleanup (e.g, drop legacy C-API, legacy unmaintained code) labels Oct 5, 2024
@mshabunin
Copy link
Copy Markdown
Contributor

I thought we are trying to preserve old functions and constants in 4.x. @asmorkalov , is it so?

@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Oct 6, 2024

Thank you for your comment.
I believe removing these functions are more appliciate than keeping them.

As referencing https://github.com/opencv/opencv/wiki/Opencv4 , we can exclude these functions completely from 4.x branch because they use cvRNG and CvScalar, which are the classical C data structures.

Also, the classical C data structures, such as CvMat, IplImage, CvMemStorage etc., as well as the corresponding functions, such as cvCreateMat(), cvThreshold() etc., are mostly excluded from API and will be completely excluded in further OpenCV 4.x updates.

And current implementation casts from classical C data (uint64 *) to C++ instance (cv::RNG *) forcefully. I think they are not recommended implementations.

CV_IMPL void
cvRandArr( CvRNG* _rng, CvArr* arr, int disttype, CvScalar param1, CvScalar param2 )
{
    cv::Mat mat = cv::cvarrToMat(arr);
    // !!! this will only work for current 64-bit MWC RNG !!!
    cv::RNG& rng = _rng ? (cv::RNG&)*_rng : cv::theRNG();

@asmorkalov asmorkalov self-requested a review October 7, 2024 05:31
@asmorkalov asmorkalov self-assigned this Oct 7, 2024
@asmorkalov
Copy link
Copy Markdown
Contributor

@Kumataro Thanks for the contribution. The plan for today is following:

  • Conservative policy in 4.x. We do not change C API, do not add tests and reduce usage internally.
  • Remove C API completely in 5.x for the 5.0 release.

I propose the following steps:

  • Apply all changes besides symbols removal to 4.x
  • Merge 4.x to 5.x and remove symbols completely there. It makes 4.x-5.x merges simpler.

@asmorkalov asmorkalov added this to the 4.11.0 milestone Oct 7, 2024
@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Oct 7, 2024

OK, thank you for your reply !!

Currently I have no idea to fix this line.
So the simplest fixing was "removing these functions", but they should be keeped at 4.x branch.
And I'll continue to try it (at 4.x).

    cv::RNG& rng = _rng ? (cv::RNG&)*_rng : cv::theRNG();

@mshabunin
Copy link
Copy Markdown
Contributor

mshabunin commented Oct 7, 2024

Maybe let's just ignore them:

#if defined(__GNUC__) && __GNUC__ == 14
#define CV_IGNORE_CAST_WARNING
#endif

<...>

#ifdef CV_IGNORE_CAST_WARNING
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wcast-user-defined"
#endif

<...>

#ifdef CV_IGNORE_CAST_WARNING
#pragma GCC diagnostic pop
#endif

@vpisarev vpisarev self-requested a review October 7, 2024 16:24
@vpisarev vpisarev self-requested a review October 7, 2024 16:24
Copy link
Copy Markdown
Contributor

@vpisarev vpisarev left a comment

Choose a reason for hiding this comment

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

agree with @asmorkalov, the part of the patch with API changes should be done in 5.x branch instead

@Kumataro Kumataro changed the title core: C-API cleanup: RNG algorithms in core core: C-API cleanup: RNG algorithms in core(4.x) Oct 7, 2024
@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Oct 8, 2024

@mshabunin Thank you for your advice, I updated with your suggest and it works well !!

@asmorkalov asmorkalov merged commit 40428d9 into opencv:4.x Oct 8, 2024
@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Oct 8, 2024

@asmorkalov Thank you very much for merging !!

At 5.x branch cvRandArr() and cvRandShuffle() had been removed at 4f0fe1d .
So I think there are nothing to do for 5.x, we can close #26258

@asmorkalov asmorkalov mentioned this pull request Oct 23, 2024
ting-xin pushed a commit to ting-xin/opencv that referenced this pull request Nov 17, 2024
core: C-API cleanup: RNG algorithms in core(4.x) opencv#26259

- replace CV_RAND_UNI and NORMAL to cv::RNG::UNIFORM and cv::RNG::NORMAL.

### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
thewoz pushed a commit to CobbsLab/OPENCV that referenced this pull request Feb 13, 2025
core: C-API cleanup: RNG algorithms in core(4.x) opencv#26259

- replace CV_RAND_UNI and NORMAL to cv::RNG::UNIFORM and cv::RNG::NORMAL.

### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: core cleanup Code cleanup (e.g, drop legacy C-API, legacy unmaintained code)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants