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

Rework jni geojson bindings #11471

Merged
merged 1 commit into from
Mar 19, 2018
Merged

Conversation

ivovandongen
Copy link
Contributor

Fixes #11212

Depends on #11468

@ivovandongen ivovandongen self-assigned this Mar 16, 2018
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@osana osana left a comment

Choose a reason for hiding this comment

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

Would it be more efficient to use corresponding fromLatLng(double []) methods when creating different Geomentries from native. That will save on going to Java for every Point object creation. Probably you were doing the changes when those methods were not yet available.

@LukasPaczos LukasPaczos added this to the android-v6.0.0 milestone Mar 19, 2018
@ivovandongen
Copy link
Contributor Author

@osana

Would it be more efficient to use corresponding fromLatLng(double []) methods when creating different Geomentries from native. That will save on going to Java for every Point object creation. Probably you were doing the changes when those methods were not yet available.

Considered that. But I'm not sure it's actually more efficient to create an array and then set data on it then creating an object with a single call. We could benchmark that.

An easy win here though would be to allow setting arrays instead of lists on everything. That avoids converting arrays to lists for each conversion.

@ivovandongen ivovandongen merged commit 2464520 into release-boba Mar 19, 2018
@ivovandongen ivovandongen deleted the ivd-jni-geojson-bindings branch March 19, 2018 09:19
@tobrun tobrun mentioned this pull request Mar 20, 2018
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants