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

[Community assistance needed] Smart pointer fix in blob_detector.cpp: OpenCV version #38

Open
twdragon opened this issue Jan 12, 2023 · 4 comments · May be fixed by #41
Open

[Community assistance needed] Smart pointer fix in blob_detector.cpp: OpenCV version #38

twdragon opened this issue Jan 12, 2023 · 4 comments · May be fixed by #41

Comments

@twdragon
Copy link

Building the package from source with Clang 15 and OpenCV 4.7.0 built from source on Ubuntu 22.04 LTS and Mint 22 the following line 9 in src/costmap_to_dynamic_obstacles/blob_detector.cpp:

return cv::Ptr<BlobDetector> (new BlobDetector(params)); // compatibility with older versions

produces the error: BlobDetector: attempt to instantiate an abstract class, because BlobDetector is inherited from cv::SimpleBlobDetector now containing abstract member functions. To harmonize the smart OpenCV pointers instantiation here, the following workaround is used:

cv::Ptr<BlobDetector> BlobDetector::create(const cv::SimpleBlobDetector::Params& params)
{
  // return cv::Ptr<BlobDetector> (new BlobDetector(params)); // compatibility with older versions
  return cv::Ptr<BlobDetector>(dynamic_cast<BlobDetector*>(cv::SimpleBlobDetector::create(params).get()));
}

Could anyone please help me figure out the OpenCV version from which this error appears? Then I will create a pull request with proper definitions to handle the issue.

@ambrosekwok
Copy link

Hello,

I have the same issue with OpenCV 4.7.0 built from source on Ubuntu 20.04 LTS. Your workaround works.
And I tried OpenCV 4.6.0 that do not have this issue. Thank a lot.

@twdragon twdragon changed the title Smart pointer fix in blob_detector.cpp (community assistance needed): OpenCV version [Community assistance needed] Smart pointer fix in blob_detector.cpp: OpenCV version Feb 18, 2023
@lopsided98
Copy link

I don't think the solution proposed here works correctly, since I don't think you can just dynamic_cast a SimpleBlobDetector (really a SimpleBlobDetectorImpl) into a BlobDetector.

opencv/opencv#20367 is what broke this code. It added the pure virtual setParams() and getParams() methods, which aren't implemented by BlobDetector. BlobDetector could add implementations of these methods, but would still be vulnerable to other breaking changes in future OpenCV versions (for example SimpleBlobDetector::getBlobContours() will become pure virtual in OpenCV 5)

On the other hand, I don't see why BlobDetector needs to inherit from SimpleBlobDetector at all. SimpleBlobDetector is an interface, and provides no functionality itself (the private SimpleBlobDetectorImpl class contains the actual implementation).

I think the best solution is to either make BlobDetector inherit from Feature2D, or perhaps remove its parent class altogether, since I can't see any usage of functionality from Feature2D either.

Lastly, OpenCV 4.7 added the SimpleBlobDetector::getBlobContours() method which appears to implement the same functionality as BlobDetector, so it may be possible to get rid of the class altogether eventually.

@twdragon
Copy link
Author

@lopsided98 as they have inheritance now, it should be possible. Anyhow, it now saves the build and makes the software working without faults (for now at least). Could you please propose your solution as a PR making the code cross-version? It looks really interesting, but considerably complex in implementation.

@lopsided98
Copy link

lopsided98 commented Aug 10, 2023

cv::SimpleBlobDetector::create(params).get() returns a pointer to an instance of SimpleBlobDetectorImpl. SimpleBlobDetectorImpl is not a subclass of BlobDetector, so I believe dynamic_cast should return nullptr in this case. I haven't tested this, so if you ran this code and got a different result, then I'm misunderstanding something.

My solution is simple: switch the base class from cv::SimpleBlobDetector to cv::Feature2D. I have implemented this in #41

@lopsided98 lopsided98 linked a pull request Aug 10, 2023 that will close this issue
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 a pull request may close this issue.

3 participants