cv::line thickness 0 not supported

[adam-ce]

cv::line can only draw lines with an thickness of at least 1 as far as I can see from drawing.cpp (method ThickLine).

I tried to disable line drawing by setting the thickness to 0, but in vain. This should be either supported or at least documented, imo.

Thanks for the great lib anyways :)

[Riyuzakii]

@alalek I would like to try and fix this issue. I replicated the issue, and I found that the thickness '0' line is same as thickness '1' line.
This image was from Thickness=1:

This image was from Thickness=0:

[alalek]

I believe we should raise an error in case of 0 value instead of disabling of line drawing. "Empty" drawing hides wrong unexpected behavior like thickness = 2/3 (which is not supported at all (type is "int", not "double", so even "2.0/3.0" would not work)). So it is programming error, that should be detected and reported.

[Riyuzakii]

@alalek are we fixing this issue? I'm only a little more than a beginner in open source, so could you be a bit more elborate?

[terfendail]

@Riyuzakii Yes, the issue should be fixed. @alalek described the expected behavior of the function in case 0 is passed for thickness parameter. Also it would be great to note the new behavior in documentation as @damdamce requested.

[adam-ce]

I just wanted to note, that in my opinion allowing thickness 0 (but not drawing) is useful. In my case i was drawing lines from a list with associated line types and per type parameters. I disable a line type by setting its thickness to 0. my current workaround is a proxy method with a test.

imo the parameter type (int) already documents the lack of support for floating point thicknesses and i'd say that most programmers are aware of loosing the decimal part in int division or conversion.

at least inkscape (as an analogy) also allows to set a line thickness of 0 and doesn't draw it in that case.

[Riyuzakii]

@terfendail where should I start looking to fix this error, I mean the files.

[terfendail]

line drawing function is implemented in modules/imgproc/src/drawing.cpp and documented as doxygen comment block in modules/imgproc/include/opencv2/imgproc.hpp

[terfendail]

@damdamce I think processing of 0 thickness as if we really try to draw invisible line is strange from performance point of view. So anyway the fix should be to check whether the thickness is 0 at the very beginning of the function and do something in that case. The action could be do nothing or throw an exception. The latter case require a tiny check at callers side but could help to avoid unintended 0 thickness drawing.
If line thickness is evaluated(i.e. due to image scaling) most of the time discarding of the decimal part doesn't matter since the is no essential difference between lines with thickness 50 and 51. But in case the decimal part is the only it really matters whether the line should be drawn thicker or completely discarded(or even affect the image in a special way by specific subpixel precision algorithm). IMO it would be better if caller explicitly choose the preferred way in this case.

[Riyuzakii]

@terfendail @alalek
CV_Assert( 0 <= thickness && thickness <= MAX_THICKNESS );
this is the that should be changed. If we write this instead:
CV_Assert( 0 < thickness && thickness <= MAX_THICKNESS );
then CV_Assert will raise an error at runtime. but do we want to raise an error or do something else when the particular event occurs?

• Also we might want to do this thing for all other drawing options like polyline and ellipse etc.

• most importantly, once i make a change like the one above how do I check if its working? Please answer this one, it'll be really helpful in the long run.

[terfendail]

@Riyuzakii
I think an error should be raised in this case, so CV_Assert update is a good solution. Also it make sense to update thickness parameter description in documentation with this restriction.
Most of other drawing functions will anyway call line so the user still get an error about thickness parameter(probably a bit confusing). Also some of other functions use different meaning for thickness parameter therefore already perform thickness processing and checking on their own.
To check the fixed functionality work as expected you could create new test that would execute it with given input and check the output. In this case you could use ASSERT_THROW(line(...),cv::Exception) to assure the code will throw an exception.
Tests related to drawing functions located at modules/imgproc/test/test_drawing.cpp

[Riyuzakii]

@terfendail Sorry for the late reply, I got a bit side tracked because of exams. I've never written test before so I'm having a bit of hard time writing them, can you point me to some resource where I can learn about testing a bit?

[terfendail]

@Riyuzakii OpenCV test system is based on GoogleTest. You could read this to get some basic understanding. Also there are existing tests at modules/imgproc/test/test_drawing.cpp that are quite simple to became a reference.

[Riyuzakii]

@terfendail I went through the link you gave. here are the changes that i have done. they don't match with what you suggested earlier but I have done them by looking at other examples and google test documentation.
test_drawing.cpp

TEST(Drawing, line)
{
    Mat mat = Mat::zeros(Size(100,100), CV_8UC1);

    line(mat, Point(1,1),Point(99,99),Scalar(255),0,8,0);
    int cnt = countNonZero(mat);
    
    ASSERT_EQ(cnt, 0);
}

What i'm trying to do is, make a line with zero thickness and if the matrix has cnt not equal to zero the raise a fatal-error. because cnt should be zero if the line doesn't exist!

[terfendail]

As discussed earlier the error should be raised by the line function if it is called with zero thickness.
So for the case of this test exception will be thrown on drawing step prior to ASSERT_EQ and it will fail the test since it's unexpected.

[Riyuzakii]

If we have already raised an error before ASSERT_EQ with this ASSERT_THROW(line(...),cv::Exception)
Then do we really need ASSERT_EQ?

[terfendail]

ASSERT_EQ could check that error is raised prior to output image modification, but IMHO ASSERT_EQ isn't necessary here

[Riyuzakii]

@terfendail Please have a look and suggest changes, I know they might be needed!