rst-tu-dortmund / costmap_converter

A ros package that includes plugins and nodes to convert occupied costmap2d cells to primitive types
140 stars 107 forks source link

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

Open twdragon opened 1 year ago

twdragon commented 1 year ago

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 commented 1 year ago

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.

lopsided98 commented 1 year ago

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.

https://github.com/opencv/opencv/pull/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 commented 1 year ago

@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 commented 1 year ago

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