Add python binding for LineIterator by LaurentBerger · Pull Request #24261 · opencv/opencv

LaurentBerger · 2023-09-12T14:44:03Z

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 LineIterator poorly supported in bindings #21307
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

modules/imgproc/src/drawing.cpp

asmorkalov

The proposal looks good! Please remove not related renaming in the class interface. Such changes does not bring value, but makes merges 4.x->5.x and conflict resolution harder.

modules/imgproc/include/opencv2/imgproc.hpp

modules/imgproc/src/drawing.cpp

review 2

opencv-alalek

It make sense to add simple Python test to avoid regressions.
Does Python types work as expected?

LaurentBerger · 2023-09-14T13:00:02Z

It make sense to add simple Python test to avoid regressions.

add a test in /modules/python/test?

Does Python types work as expected?

What do you mean?
getPos() and Next() return a tuple
h = cv.LineIterator_create(np.array([30, 400]), np.array([200, 90]))
h = cv.LineIterator_create([30, 400], [200, 90])
h = cv.LineIterator_create((30, 400), (200, 90))
works

May be Next() method should become next()?

asmorkalov · 2023-09-14T14:32:59Z

add a test in /modules/python/test? - yes.
Most probably alalek means Python type stubs. They should be generated automatically with bindings generator script.

VadimLevin · 2023-09-14T15:23:48Z

modules/imgproc/include/opencv2/imgproc.hpp

+
+    /** @brief next pixel. Use for python binding.
+    */
+    CV_WRAP Point Next() {


use next() - OpenCV uses camelCase naming convention for functions

VadimLevin · 2023-09-14T15:25:30Z

modules/imgproc/src/drawing.cpp

+    Ptr<LineIterator> lineiterator = makePtr<LineIterator>(pt1, pt2, connectivity, leftToRight);
+    return lineiterator;


Suggested change

Ptr<LineIterator> lineiterator = makePtr<LineIterator>(pt1, pt2, connectivity, leftToRight);

return lineiterator;

return makePtr<LineIterator>(pt1, pt2, connectivity, leftToRight);

VadimLevin · 2023-09-14T15:26:14Z

samples/python/lineiterator.py

+img = cv.imread(cv.samples.findFile('butterfly.jpg'))
+pt1 = (30, 40)
+pt2 = (200, 90)
+h = cv.LineIterator_create(pt1, pt2)


prefer to use .create notation instead of _create

Suggested change

h = cv.LineIterator_create(pt1, pt2)

h = cv.LineIterator.create(pt1, pt2)

VadimLevin · 2023-09-14T15:26:27Z

samples/python/lineiterator.py

+cv.destroyAllWindows()
+pt1 = (40, 80)
+pt2 = (30, 200)
+h = cv.LineIterator_create(pt1, pt2)


Suggested change

h = cv.LineIterator_create(pt1, pt2)

h = cv.LineIterator.create(pt1, pt2)

VadimLevin · 2023-09-14T15:27:05Z

modules/imgproc/include/opencv2/imgproc.hpp

    @param leftToRight If true, the line is traversed from the leftmost endpoint to the rightmost
    endpoint. Otherwise, the line is traversed from \p pt1 to \p pt2.
    */
-    LineIterator( const Mat& img, Point pt1, Point pt2,


what is the point of renaming img argument to src?
It is unnecessary change

before PR there was an attribute Mat img in LineIterator. Many warning were generated parameter img hide attribute . i change parameter name. attribute img was unused in class. I delete it after first or second review.
After two changes it is now unnecessary

VadimLevin · 2023-09-14T15:28:40Z

modules/imgproc/include/opencv2/imgproc.hpp

 @endcode
 */
-class CV_EXPORTS LineIterator
+class CV_EXPORTS_W LineIterator


Why do you need create function. Have you tried to use CV_EXPORTS_W_SIMPLE and add CV_WRAP to the appropriate constructor?

~~I tried it does not work without static method create~~

h = cv.LineIterator.create(pt1, pt2) pt = h.pos()

error is

[ WARN:0@0.057] global samples.cpp:61 cv::samples::findFile cv::samples::findFile('butterfly.jpg') => 'C:\lib\opencv\samples/data\butterfly.jpg' Traceback (most recent call last): File "C:\lib\opencv\samples\python\lineiterator.py", line 8, in <module> pt = h.pos() cv2.error: Unknown C++ exception from OpenCV code

I floolowed guide line (https://docs.opencv.org/4.x/da/d49/tutorial_py_bindings_basics.html)
For large classes also, CV_EXPORTS_W is used. To extend class methods, CV_WRAP is used. Similarly, CV_PROP is used for class fields.

Small classes/structs are extended using CV_EXPORTS_W_SIMPLE. These structs are passed by value to C++ functions. Examples are KeyPoint, Match etc. Their methods are extended by CV_WRAP and fields are extended by CV_PROP_RW.

~~Now code is with CV_EXPORTS_W_SIMPLE and with static method create~~

opencv-alalek · 2023-09-16T22:15:31Z

modules/python/test/test_lineiterator.py

@@ -0,0 +1,32 @@
+#!/usr/bin/python


#!/usr/bin/env python is more usable

opencv-alalek · 2023-09-16T22:16:02Z

modules/python/test/test_lineiterator.py

+#!/usr/bin/python
+
+'''
+This example illustrates how to use LineIterator to get point coordinates on a line


This example

This line is not needed (at least not accurate)

opencv-alalek · 2023-09-16T22:17:52Z

modules/python/test/test_lineiterator.py

+import cv2 as cv
+import numpy as np
+import sys
+import math


math

Where is it used?

opencv-alalek · 2023-09-16T22:21:01Z

samples/python/lineiterator.py

+
+pt = h.pos()
+while min(pt1[0], pt2[0]) <= pt[0] <= max(pt1[0], pt2[0]) and \
+      min(pt1[1], pt2[1]) <= pt[1] <= max(pt1[1], pt2[1]):


Could we improve stop condition?

Could we provide a "native" Python iterator?

for pt in cv.LineIterator(pt1, pt2): ... process pt ...

See how to write Python (3+ only) code here: modules/core/misc/python/package (but for imgproc).

Python allows to replace modules's entity.
Redefine class (create new) or redefine __iter__/__next__ methods:

def LineIterator__iter__(self): ... reset ... return self def LineIterator__next__(self): if self.next(): return self.pos else: raise StopIteration cv2.LineIterator.__iter__ = LineIterator__iter__ cv2.LineIterator.__next__ = LineIterator__next__

There is a problem when import cv2 :
cv2.LineIterator.iter = LineIterator__iter__
TypeError: cannot set 'iter' attribute of immutable type 'cv2.LineIterator'

file init.py content in imgproc/python/package/lineiteraor/

import cv2 def LineIterator__iter__(self): return self def LineIterator__next__(self): if self.next(): return self.pos else: raise StopIteration cv2.LineIterator.__iter__ = LineIterator__iter__ cv2.LineIterator.__next__ = LineIterator__next__

opencv-alalek · 2023-09-16T22:48:07Z

modules/imgproc/include/opencv2/imgproc.hpp

+
+    /** @brief next pixel. Use for python binding.
+    */
+    CV_WRAP Point next() {


bool next() interface is more appropriate for iterators.

opencv-alalek · 2023-09-17T20:52:38Z

modules/imgproc/misc/python/package/lineiterator/__init__.py

@@ -0,0 +1,18 @@
+import cv2 as cv
+
+class LineIteratorB(cv.LineIterator):


👍 for derived class.

Please use LineIteratorWrapper or LineIteratorForPython. `B looks as unclear name.

opencv-alalek · 2023-09-17T20:52:44Z

modules/imgproc/misc/python/package/lineiterator/__init__.py

+        super().__init__(pt1, pt2)
+
+    def __iter__(self):
+        return self


Please check:

line = cv.LineIterator(p1, p2) print('first pass...') for pt in line: print(pt) print('second pass...') for pt in line: print(pt)

Passes should print the same elements.

weird https://stackoverflow.com/questions/25336726/why-cant-i-iterate-twice-over-the-same-iterator-how-can-i-reset-the-iterator

See comments above - reset part should be implemented.

def LineIterator__iter__(self): ... reset ... return self

BTW, another case from Python iter world:

line = cv.LineIterator(p1, p2) it1 = iter(line) it2 = iter(line) ... it1 and it2 should not interfere on each other (must be independent) ...

Perhaps we could not derive. May be we should use some kind of proxy pattern.

opencv-alalek · 2023-09-17T20:53:43Z

modules/imgproc/misc/python/package/lineiterator/__init__.py

+
+LineIteratorB.__module__ = cv.__name__
+cv.LineIterator = LineIteratorB
+cv._registerMatType(LineIteratorB)


cv._registerMatType(LineIteratorB)

Remove that.

opencv-alalek · 2023-09-17T20:58:26Z

modules/python/test/test_lineiterator.py

+            count = count + 1
+            if count == 10:
+                self.assertEqual(p.pos(), (10.0, 10.0))
+                break


Lets check for stop condition instead of break:

for ... ... self.assertLess(count, 102) self.assertEqual(count, 101)

BTW, 101, 102 could be adjusted.

opencv-alalek · 2023-09-17T20:59:28Z

modules/imgproc/include/opencv2/imgproc.hpp

+    */
+    CV_WRAP bool next() {
+        this->operator++();
+        return true;


It never stops.

I must change c++ class

opencv-alalek · 2023-09-17T21:00:28Z

samples/python/lineiterator.py

+
+pt = h.pos()
+pt_tl, pt_br = boundaries_rect(pt1, pt2)
+while pt_in_rect(pt, pt_tl, pt_br):


make it simple.

change in c++ class

LaurentBerger · 2023-09-21T11:24:47Z

@opencv-alalek something is weird
result with test_lineiterator.py in PR
#https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/104652
result without test_lineiterator.py in PR everything is green
#https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/104654
https://pullrequest.opencv.org/buildbot/builders/precommit_windows64/builds/102841
https://pullrequest.opencv.org/buildbot/builders/precommit_docs/builds/104438
https://pullrequest.opencv.org/buildbot/builders/precommit_linux64_no_opt/builds/103032
append test_lineiterator.py result with test_lineiterator.py in PR
now everything is red
Problem in python 2-3 test

In python 2 test_lineiterator (test_lineiterator.lineiterator_test) ... ERROR how to disable python2 test

in python3
test_lineiterator (test_lineiterator.lineiterator_test) ... ok Linux x64 Debug
for https://pullrequest.opencv.org/buildbot/builders/precommit_linux64_no_opt/builds/103034/steps/test_python3/logs/stdio
and for linux x64
In linux x64 a test failed est_arithm_op_with_saturation (test_misc.Arguments) ... /build/precommit_linux64/.container/job_2165934: line 19: 40186 Segmentation fault (core dumped) /app/bin/buildenv python3 ../4.x/opencv/modules/python/test/test.py --repo ../4.x/opencv --data /opt/build/python_test_data -v
for win64 python2 failed and test_arithm_op_with_saturation (test_misc.Arguments) ... program finished with exit code -1073741819

How my test can kill other testt?

still waiting an answer @opencv-alalek
@LaurentBerger

asmorkalov · 2023-10-05T06:41:44Z

@opencv-alalek please take a look.

LaurentBerger · 2023-10-11T18:25:51Z

@opencv-alalek please take a look.

I proposed to close this PR

asmorkalov · 2023-10-12T07:06:13Z

@LaurentBerger Thanks a lot for the contribution. Feel free to close PR, if you treat it as not successful, or does not have a chance to finish the work.

LaurentBerger · 2023-10-12T12:29:24Z

Thanks a lot for the contribution. Feel free to close PR, if you treat it as not successful, or does not have a chance to finish the work.

the work is finished but buildbot is wrong. All my unit tests are good but other tests failed with timeout

asmorkalov requested a review from VadimLevin September 12, 2023 14:51

asmorkalov added the category: python bindings label Sep 12, 2023

asmorkalov added this to the 4.9.0 milestone Sep 12, 2023

LaurentBerger force-pushed the lineiterator branch from 1f8d338 to 23335e3 Compare September 12, 2023 15:32

Add python binding for LineIterator

23335e3

opencv-alalek reviewed Sep 12, 2023

View reviewed changes

modules/imgproc/src/drawing.cpp Outdated Show resolved Hide resolved

review 1

cd2f7ca

asmorkalov reviewed Sep 13, 2023

View reviewed changes

modules/imgproc/include/opencv2/imgproc.hpp Outdated Show resolved Hide resolved

modules/imgproc/src/drawing.cpp Outdated Show resolved Hide resolved

LaurentBerger added 2 commits September 13, 2023 14:47

Update imgproc.hpp

dee82a8

review 2

Update drawing.cpp

e5f81a2

review 2

opencv-alalek reviewed Sep 13, 2023

View reviewed changes

VadimLevin reviewed Sep 14, 2023

View reviewed changes

LaurentBerger added 2 commits September 14, 2023 18:53

remocve create change CV_EXPORTS_W in CV_EXPORTS_W_SIMPLE

dd51aa1

python test

33f74aa

opencv-alalek reviewed Sep 16, 2023

View reviewed changes

LaurentBerger added 2 commits September 17, 2023 18:15

python binding

23bc466

update test

20c0e39

opencv-alalek reviewed Sep 17, 2023

View reviewed changes

LaurentBerger added 10 commits September 18, 2023 18:22

many changes

9814dd7

another try

5264441

rebuild

757b1ed

rebuild disable test for python2

86c1a69

include iter() in python test

0374290

add private member in LineIteratorWrapper

d6012a2

disable py2

1bca712

only test

5786d16

remove test to check buildbot

1cb3f8c

add test_lineiterator.py

fea78f2

LaurentBerger added 9 commits September 22, 2023 17:24

empty test

8537aa4

only instanciation

fce488a

one more line

305948d

4 more lines

1296f36

disable python2 sys.version_info

5108773

iter test

f247939

python 2

5f812cb

remove iter

0316751

reduce time and new test

f22db6b

LaurentBerger closed this Oct 12, 2023

LaurentBerger mentioned this pull request Dec 17, 2023

Add blobrecttoimage #24539

Merged

6 tasks

		Ptr<LineIterator> lineiterator = makePtr<LineIterator>(pt1, pt2, connectivity, leftToRight);
		return lineiterator;

	Ptr<LineIterator> lineiterator = makePtr<LineIterator>(pt1, pt2, connectivity, leftToRight);
	return lineiterator;
	return makePtr<LineIterator>(pt1, pt2, connectivity, leftToRight);

	h = cv.LineIterator_create(pt1, pt2)
	h = cv.LineIterator.create(pt1, pt2)

		@@ -0,0 +1,18 @@
		import cv2 as cv

		class LineIteratorB(cv.LineIterator):

Uh oh!

Conversation

LaurentBerger commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

opencv-alalek left a comment

Choose a reason for hiding this comment

Uh oh!

LaurentBerger commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asmorkalov commented Sep 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurentBerger Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurentBerger Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurentBerger commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asmorkalov commented Oct 5, 2023

Uh oh!

LaurentBerger commented Oct 11, 2023

Uh oh!

asmorkalov commented Oct 12, 2023

LaurentBerger commented Sep 12, 2023 •

edited

Loading

LaurentBerger commented Sep 14, 2023 •

edited

Loading

LaurentBerger Sep 14, 2023 •

edited

Loading

LaurentBerger Sep 17, 2023 •

edited

Loading

LaurentBerger commented Sep 21, 2023 •

edited

Loading