Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[android] Add hole handling to PolygonOptions #5170

Closed
wants to merge 1 commit into from

Conversation

nealsanche
Copy link

  • Added holes to the Java SDK, JNI, and renderer
  • Multiple holes are supported
  • Seems stable even if no holes are specified
  • Added code to add holes in the Polygon demo app which adds 3 holes to the star over portland.

@1ec5
Copy link
Contributor

1ec5 commented May 27, 2016

Could you rebase and force-push your changes (fetch upstream master, then rebase the feature branch atop master, then push --force)? Since you last updated your branch, we made a major change down in C++ land (replacing include guards with #pragma once, so it's difficult to find your changes in there.

@1ec5
Copy link
Contributor

1ec5 commented May 27, 2016

/cc @tobrun

@bleege
Copy link
Contributor

bleege commented May 27, 2016

/cc @jfirebaugh

@nealsanche
Copy link
Author

@1ec5: I did a rebase just now and force pushed.

jni::jobject* holes = jni::GetField<jni::jobject*>(*env, polygon, *polygonHolesId);
mbgl::AnnotationSegments holeSegments = annotation_segments_from_jlist_of_latlng_jlist(env, holes);
jni::DeleteLocalRef(*env, holes);
mbgl::Log::Debug(mbgl::Event::JNI, "H-H");
Copy link
Author

Choose a reason for hiding this comment

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

Left in a silly log statement here. Whoops. I'll nuke that guy.

@1ec5
Copy link
Contributor

1ec5 commented May 27, 2016

Thanks for rebasing, @nealsanche. I’m wondering if you’ve seen #5110, d0ea7c3 in particular. mbgl::AnnotationSegments and geojsonvt::ProjectedRings already support holes in polygons: these types consist of an outer ring (the first element), followed by all the inner rings, so no separate “holes” field would be needed at the JNI level on down. I’m hoping to land #5110 today or thereabouts; after that, I think this PR will simplify somewhat.

@nealsanche
Copy link
Author

@1ec5: You're right, the code would simplify with #5110 in it. I'd be happy to fix this PR when #5110 lands.

@1ec5
Copy link
Contributor

1ec5 commented May 28, 2016

OK, #5110 is merged.

@nealsanche
Copy link
Author

Alright. I will have a look into porting these changes, likely Monday.

On Sat, May 28, 2016, 12:21 AM Minh Nguyễn [email protected] wrote:

OK, #5110 #5110 is
merged.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5170 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AFHsyafYw6WPosxfXTBe4E8o438621vOks5qF9DEgaJpZM4Iol7e
.

Robots and Pencils Inc | 1507 14th Ave SW Calgary AB T3C 0W4
You can unsubscribe from the Robots and Pencils mailing lists by clicking
here. https://robotsandpencils.com/contact/unsubscribe.html

- Added holes to the Java SDK, JNI, and renderer
@nealsanche
Copy link
Author

@1ec5: Alright, I've updated the Android JNI code to use the simplified method of just appending the holes to the shell segment. Seems to be working.

@1ec5 1ec5 added Android Mapbox Maps SDK for Android annotations Annotations on iOS and macOS or markers on Android labels May 30, 2016
@nealsanche
Copy link
Author

@1ec5: So, I took a look at the conflicts here today. So much has changed in the JNI layer that I'm not sure where to go with all of this. I'm assuming that this branch is going to dead end, and that the holes API will be implemented in another branch?

@1ec5
Copy link
Contributor

1ec5 commented Jun 7, 2016

With all the things moving in both mbgl and the Android SDK, unfortunately, that’s probably the case. @bleege can comment on whether polygon hole support is something we can get into an upcoming Android SDK release, or whether it’ll have to wait until the next release cycle. Thank you for taking the time to make this PR, in any case.

@nealsanche
Copy link
Author

I suspect, even with the current changes, that the code delta to add polygons with holes to Android would quite small. I would just have to spend a lot more time that one of your team members to make the adaptations.

@bleege
Copy link
Contributor

bleege commented Jun 7, 2016

@nealsanche Thanks so much for inquiring about this (and writing code!). We're currently putting the finishing touches on the Android 4.1.0 release so we're reluctant to add new features this late in that stage. We'll be starting to think abotu Android 4.2.0 soon though and will be happy to consider it at that time. Thanks again!

@nealsanche nealsanche closed this Oct 5, 2016
@1ec5 1ec5 mentioned this pull request Dec 13, 2016
@1ec5
Copy link
Contributor

1ec5 commented Dec 13, 2016

I made an attempt at reviving this PR in #7397.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android annotations Annotations on iOS and macOS or markers on Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants