Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose more functionality via the SWIG wrapper #257

Merged
merged 10 commits into from
Sep 9, 2022

Conversation

MikePlayle
Copy link
Contributor

Add S2::UpdateMinDistance, s2pred::OrderedCCW, S2::Interpolate,
S2Polygon::InitToUnion, some more of S2::S1ChordAngle, and a
method for retrieving the S2Point's underlying 3-vector.

@google-cla
Copy link

google-cla bot commented May 26, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@MikePlayle
Copy link
Contributor Author

Extended to add S2BufferOperation, S2BooleanOperation and S2Builder, along with various things that they depend on.

I'm not sure that this is necessarily the "right way" to do it - if there are any problems here, now would be a good time to discuss them.

@MikePlayle
Copy link
Contributor Author

Is there anything else I need to do to this pull request?

Would it help if I split it up?

@jmr
Copy link
Member

jmr commented Jun 7, 2022

Splitting it up would probably get you pieces sooner, if that's useful to you. Probably not worth the effort otherwise. I'll look more at this tomorrow.

Copy link
Member

@jmr jmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only just started to look at this. I'll have more questions later, since not everything is clear to me.

src/python/s2_common.i Show resolved Hide resolved
src/python/pywraps2_test.py Outdated Show resolved Hide resolved
src/python/s2_common.i Show resolved Hide resolved
@@ -71,6 +107,15 @@

%apply S2CellId *OUTPUT_ARRAY_4 {S2CellId neighbors[4]};

%typemap(in, numinputs=0) S2Error * (S2Error err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do / where is it used? Maybe add a comment since it's not obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pair of typemaps work together to translate errors from the C++ library into Python exceptions. The first declares a local variable S2Error err; and sets the matched argument ($1, of type S2Error *) to point to it; the second inspects the value of the S2Error after the call and throws an exception if its ok() method returns false. S2BuilderTest.testException in pywraps2_test.py tests this.

https://www.swig.org/Doc1.3/Typemaps.html#Typemaps_nn23 describes the syntax being used here.

I've added a comment.

@jmr
Copy link
Member

jmr commented Aug 24, 2022

Sorry for the delay, I'll merge after I understand https://github.com/google/s2geometry/pull/257/files#r953600009 .

The S2BufferOperationOptions class is a workaround for being unable to
wrap nested classes with SWIG. The SWIG documentation suggests it can
be done, but I couldn't get it to work, and comments in s2_common.i
suggest that others haven't been able to either.

I had similar difficulty with S2Shape, which I worked around by hiding
it from the Python interface entirely.
@jmr jmr merged commit 465c804 into google:master Sep 9, 2022
@jmr
Copy link
Member

jmr commented Sep 9, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants