Bug
if (intersects) {
isSingle = false;
break;
}
This looks like a bug. The variable isSingle
is already false. Setting it to false here does nothing.
Code Review:
You use std::vector
but don't include its header.
std::vector <Rect> overlaps;
You should fix that.
References and Const correctness.
This is a big part of C++.
If you are not modifying an object, prefer to use reference than making a copy of an object.
Prefer const references over normal references to prevent accidental modification.
for (auto rect_1: detected) {
Here you are making a copy of the object from the array detected into the object rect_1
. Personally I would use a const reference to prevent the overhead of a copy:
for (auto const& rect_1: detected) {
The code looks mostly fine:
I think the only bug is here:
if (rect_1 == rect_2) {
continue;
}
This tells you if two rectangles are equivalent not if they are the same rectangle. If you have two distinct but otherwise identical rectangle then by my logic they would overlap and thus not be in the single's list.
So you need a way to determine if these are the same object. Personally I would do that by changing your for
loops to use references and then comparing the objects' addresses.
for (auto const& rect_1: detected) {
^^^^^^
...
for (auto const& rect_2: detected) {
^^^^^^
if ( & rect_1 == & rect_2) {
^^^ ^^^
continue;
}
test(rect_1, rect_2);
Your other alternative is to make sure you never compare an object with itself. This also brings into mind that you do twice as many comparisons as you need to. You compare every rectangle against every other rectangle. But if A & B
overlap then we already know that B & A
will overlap.
for(std::size_t loop = 0l; loop < detected.size(); ++loop) {
for(auto check = loop + 1; check < detected.size(); ++check) {
test(detected[loop], detected[check]);
in the code below "...object addresses" -- aren't rect_1
and rect_2
references already? If so, why can't we just do if (rect_1 == rect_2)
No. Because the ==
is supposed to compare the equality (is the state the same) of two objects. It is not supposed to be a test of equivalence (is it the same object).
Note: You can declare operator==
to test if the objects are the same but that would be terrible practice and would cause a lot of issues with people expecting the normal operation. So don't do that. The above statement is said assuming you are following the normal idioms of the language.
I assume you are coming from a Java like background. The term reference in C++ has a different meaning than in Java. A reference means another name for an object (they are not like Java references (which in C++ we would call pointers). So a reference is another name for an object but the test operator==
will compare if the two objects' state are the same (not if they are the same object).
Also, if I use auto const&
, I would have to change my return type to vector<Rect&>
...is that idiomatic? I think the OpenCV API expects vector<Rect>
No. You can pass a reference to the vector and it will copy the object into the vector. You don't want to hold references in a vector.
get_detected()
defined? \$\endgroup\$isSingle
to true. \$\endgroup\$break
andcontinue
with an iterator-based outer loop and replacing the inner loop withstd::find_if(i, end(), predicate)
. I had a nice spelled answer but unfortunately this question get closed before I could post. That would be clean code. \$\endgroup\$