Polygon holes#7397
Conversation
1ec5
added
Android
|
@1ec5, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @zugaldia and @ansis to be potential reviewers. |
1ec5
force-pushed
the
1ec5-android-polygon-hole-5170
branch
6 times, most recently
from
774a2aa to
87be52b
Compare
ivovandongen
left a comment
ivovandongen
left a comment
There was a problem hiding this comment.
@1ec5 I don't see any holes in the example.
| */ | ||
| public final class Polygon extends MultiPoint { | ||
|
|
||
| private List<List<LatLng>> holes; |
There was a problem hiding this comment.
Could you make this a final list and initialize in declaration?
| * @param points A {@link List} {@link List}s of {@link LatLng} points making up the holes. | ||
| */ | ||
| public void setHoles(List<? extends List<LatLng>> holes) { | ||
| this.holes = new ArrayList<>(holes); |
There was a problem hiding this comment.
Can just update the member list with the collection, so the reference is not exposed.
There was a problem hiding this comment.
You mean this.holes = holes? I think the idea was to make a copy of the list, similar to MultiPoint.setPoints().
|
|
||
| std::size_t size = jni::GetArrayLength(*env, *jarrayHoles); | ||
| for (std::size_t j = 0; j < size; j++) { | ||
| jni::jobject* hole = reinterpret_cast<jni::jobject*>(jni::GetObjectArrayElement(*env, *jarrayHoles, j)); |
There was a problem hiding this comment.
This cast is redundant, jni::GetObjectArrayElement returns jni::jobject* already
There was a problem hiding this comment.
Fixed in 2ec060543899367c3b1f9026117488d8c133ec10.
|
Thanks for taking a look!
I think the example originally worked, but now the star's outer ring has been moved to a static member of the class instead of a local array. For the purposes of this example, I think we'll be able to define the hole similarly to the outer ring instead of trying to define the hole dynamically. |
1ec5
force-pushed
the
1ec5-android-polygon-hole-5170
branch
2 times, most recently
from
2ec0605 to
ac4d3b8
Compare
|
I modernized the example activity, though I haven’t tried it yet. |
|
@1ec5 Polygon looks different, but I still see no holes:
|
|
Thanks @zugaldia. I need to actually get around to setting up a development environment. GitHub comments don't make for a good debugger. 😅 |
|
@1ec5 totally :) wanna work on that today to benefit from the small delta in our coordinates? |
- Added holes to the Java SDK, JNI, and renderer
1ec5
force-pushed
the
1ec5-android-polygon-hole-5170
branch
from
cf42502 to
6da5e07
Compare
|
I see that there's a pull request open: Does this mean that in a future version, it is guaranteed that we will have interior holes on Android? I've been following this issue for a while and have seen iOS get a lot of attention on the subject, but I am more interested in Android. |
|
#1729 is the bug report; this is the PR that would fix it for Android. The issue was fixed for iOS and macOS in #5110, which also involved adding support in the core C++ code for polygon holes. This PR plumbs that support through JNI to the Android SDK and adds an activity to demonstrate the feature. Unfortunately, the activity currently demonstrates that the JNI implementation is broken. @Guardiola31337 is currently assigned to take a look. We’re still scoping out what the next release will contain, but this PR is on our radar. |
|
#8557 landed instead. |
7 participants
A slapdash attempt at reviving #5170 to add support for holes in
PolygonandPolygonOptions. I resolved the conflicts without the aid of an IDE, so this PR needs close scrutiny – there’s probably quite a bit of copy 🍝 in here.Fixes #1729.