Missing a test for BBoxHierarchy.search (or something in test_picture if you touches src/skia/Pictures.cpp? Also, this is the sort of information that should be in the commit message itself. (Try to keep the commit self-contained without requiring reading this pull message.) . On the command line, it is git commit --amend to edit the commit message of the commit you just made, or git rebase -i ..., then choose to "re-write", for older commits, before pushing.

BBoxHierarchy.search is a virtual method, I have no idea how to test it.

We have code that binds BBoxHierarchy.search so either that's a mistake (not invokable, and should be removed) or should be tested... I think it should be invokable if not directly.

The editing was a suggestion - it would be nice to include that info (in a future commit in this pull in particular, and generally in future contributions elsewhere), and there is a chance of adding that in a later / additional commit. Force-push on already public branches is a bit frowned upon, should be done only if the previous was a clear mistake - this wasn't a clear mistake, it was correct (as it seems) just somewhat incomplete. Anyway.

I found there is a RTreeFactory class that can generate RTree class, which is derived from BBoxHierarchy. It can be used for test.

I found there is a RTreeFactory class that can generate RTree class, which is derived from BBoxHierarchy. It can be used for test.

That's probably correct - RTreeFactory.seach or something derived might be invokable.

We have code that binds BBoxHierarchy.search so either that's a mistake (not invokable, and should be removed) or should be tested... I think it should be invokable if not directly.

The editing was a suggestion - it would be nice to include that info (in a future commit in this pull in particular, and generally in future contributions elsewhere), and there is a chance of adding that in a later / additional commit. Force-push on already public branches is a bit frowned upon, should be done only if the previous was a clear mistake - this wasn't a clear mistake, it was correct (as it seems) just somewhat incomplete. Anyway.

Thanks for your suggestion, I just dont't want to have too many extra commits...

Anyway, I've completed the commit message and test as you mentioned.

For future maintenence, we don't want to accept large changes wholesale, with very brief / one-line message either :-). While I think you have a point of trying not too do too many small commits, I would generally say that, you will want to look at the commit in 6 months or a years' time (if there is a future problem), know which part to revert, and which part to fix, without ripping the whole thing with a single revert.

https://github.com/kyamagu/skia-python/blob/832cb4e733548dedc6fa93f59066e149329ba4d3/src/skia/Picture.cpp#L30-L35
By the way, I'm confused about the signature of BBoxHierarchy.insert and BBoxHierarchy.search.
When I testing the two methods, I found that:

• I can only pass one skia.Rect object to insert, not a list, see https://github.com/MeetWq/skia-python/blob/719dda52e57e979cc819e4531cd1537588a724dd/tests/test_picture.py#L83-L84
• The results argument of search didn't seems to be changed after calling search method.

https://github.com/kyamagu/skia-python/blob/832cb4e733548dedc6fa93f59066e149329ba4d3/src/skia/Picture.cpp#L30-L35

By the way, I'm confused about the signature of BBoxHierarchy.insert and BBoxHierarchy.search.
When I testing the two methods, I found that:

• I can only pass one skia.Rect object to insert, not a list, see https://github.com/MeetWq/skia-python/blob/719dda52e57e979cc819e4531cd1537588a724dd/tests/test_picture.py#L83-L84
• The results argument of search didn't seems to be changed after calling search method.

The signature looks slightly wrong - there are some subtlety in passing a pointer to a list between c++ and python - the length info is lost on the way. I think the code needs to be modified to receive a python list, extract the length and the pointer from the input object, before calling the skia c++.

If you lose the length info on the way, on the skia side it knows about only one element, which is what you are observing.

I think the skia-python code is at least for the search method looks wrong - pybind11 in general cannot fill an input pointer with fetched stuff, so the search method need to be adapted to something like this:

std::vector<int> search(const& query) {
       std::vector<int> result;
       this->search(query, &result);
       return result;
}

I think the skia-python code is at least for the search method looks wrong - pybind11 in general cannot fill an input pointer with fetched stuff, so the search method need to be adapted to something like this:

std::vector<int> search(const& query) {
       std::vector<int> result;
       this->search(query, &result);
       return result;
}

This looks better. Should I modify this in this PR? Or you will modify it?

I found two usages of this API:
https://github.com/google/skia/blob/95ef9caae482173eb7b20d5e13f8d9773a93f435/src/core/SkRecordDraw.cpp#L63
https://github.com/google/skia/blob/95ef9caae482173eb7b20d5e13f8d9773a93f435/tests/PictureTest.cpp#L889

And both of them are as I outlined. That said, I wonder if Google folks will change this as at some point (ie. How committed are they to keep it in the current form, or as public api, at all). So it looks like a bit more involved - not only adding a test, updating the API, and perhaps porting the latter above as an example.

I've modified the API and tests,
and ported some tests from google skia:
https://gist.github.com/MeetWq/0a0390f0a3aadb5beaaad310401398fd

The pybind11::gil_scoped_acquire gil; looks a bit dubious?

Btw, the override is for the base class in c++. I think if the signature is plainly different, it is just another overloaded method, and can be added as such; the search result appearing as a pointer input to be written isn't likely to work, as you are passing a python object into c++ and wants the c++ code to modify the python object's content. (I mean if you change the signature, actually receiving a python object in the c++ code, it might work; but casted to a pointer to x, is expected to be "read-only" on the c++ side).

I followed the example in https://pybind11.readthedocs.io/en/stable/advanced/classes.html#different-method-signatures

The class PyBBoxHierarchy is a 'trampoline' for overriding virtual functions in Python.
I'm not so sure I got it right, but the test in https://gist.github.com/MeetWq/0a0390f0a3aadb5beaaad310401398fd#file-skia-python-bbh-test-py-L56 works.

The adjustment in PyBBoxHierarchy is for extending BBoxHierarchy in python, it's different from the signature override in https://github.com/MeetWq/skia-python/blob/ffcb64e5ba33f2aaf64d4de1ab9801398ce188bb/src/skia/Picture.cpp#L328

I am still going to ask why those pybind11::gil_scoped_acquire gil; are there?

As in https://pybind11.readthedocs.io/en/stable/advanced/misc.html#global-interpreter-lock-gil

When writing C++ code that is called from other C++ code, if that code accesses Python state, it must explicitly acquire and release the GIL.

There is a pybind11::gil_scoped_acquire gil; expression inside the macro PYBIND11_OVERLOAD_PURE also.

Maybe py::gil_scoped_release release; should also be explicitly called as shown in the example.

I've modified the API and tests, and ported some tests from google skia: https://gist.github.com/MeetWq/0a0390f0a3aadb5beaaad310401398fd

I think the 3 tests can be appended verbatim as they are, to tests/test_picture.py? There are enough asserts etc inside to serve as 3 "composite" tests, if you don't feel like adding individual ones, or finding adding individual tests a bit tedious (I do find that tedious, myself...).