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

[core] Add batch conversion of latLngs to/from screenCoords #15891

Merged
merged 2 commits into from
Nov 7, 2019

Conversation

zmiao
Copy link
Contributor

@zmiao zmiao commented Nov 6, 2019

This patch introduces batch conversion between LatLng and ScreenCoordinate in Gl-Native core, so for multiple conversions with single point/latLng previously now it can be done with invoking one function call by passing vector of points/latLngs.

@tobrun
Copy link
Member

tobrun commented Nov 6, 2019

Thank you for picking this up @zmiao! I ticketed out mapbox/mapbox-gl-native-android#36 downstream as tailwork.

cc @julianrex

@julianrex
Copy link
Contributor

Very happy to see this PR! Thank you!

src/mbgl/map/map.cpp Show resolved Hide resolved
src/mbgl/map/map.cpp Outdated Show resolved Hide resolved
src/mbgl/map/map.cpp Show resolved Hide resolved
src/mbgl/map/transform.cpp Outdated Show resolved Hide resolved
@zmiao zmiao force-pushed the zmiao-batch-convention-latLng/srcreenCoord branch 3 times, most recently from cfb07c7 to 23c204a Compare November 7, 2019 15:09
@zmiao zmiao force-pushed the zmiao-batch-convention-latLng/srcreenCoord branch from 23c204a to 2a728d1 Compare November 7, 2019 15:15
@zmiao zmiao merged commit 2e8a350 into master Nov 7, 2019
@zmiao zmiao deleted the zmiao-batch-convention-latLng/srcreenCoord branch November 7, 2019 16:16
@julianrex
Copy link
Contributor

I think we will (at some point) want to return to the more batch-like process: From previous profiling on iOS, you can see a significant amount of time being spent in TransformState::getProjMatrix, which AFAIK doesn't need to be called more than once for a batch of coordinates/latlngs?

@chloekraw
Copy link
Contributor

Interesting @julianrex. Does anything currently block this suggested change? cc @zmiao

@zmiao
Copy link
Contributor Author

zmiao commented Nov 12, 2019

@chloekraw @julianrex Actually we had a discussion here #15891 (comment). So for the performance issue, it will be done as a separate issue. After refactoring, the perfomance will be improved, but from SDK point of view, the most performance critical part is actually by invoking the API, which is already done in this pr.

@edisonw
Copy link

edisonw commented Nov 20, 2019

@zmiao, since this API is used mostly for performance reasons, do you know if it would be possible to support reusing objects as well? e.g. Support passing in an input byte array an output byte array with sizes and simply do the conversions in place?

@edisonw
Copy link

edisonw commented Nov 20, 2019

Attaching something that was based on an old head:

Batch-LatLng-to-Pixel-conversion.txt

@zmiao
Copy link
Contributor Author

zmiao commented Nov 21, 2019

@edisonw, thank you very much for providing this patch, I have checked it. As it is totally a different approach, before taking any actions, may I ask if it is possible that you could provide some measurements between our current API and your in-place solution with use-cases that you probably need to handle, with that result we could think for the next step.

Updates: after this pr #15956 landed, the conversion performance will be improved further, saving almost 2/3 time comparing with current master.

cc: @chloekraw

@edisonw
Copy link

edisonw commented Nov 21, 2019

@zmiao the in place solution was based on an very old branch (from about 3yrs ago!) so I would not compare it with the current branch. Very glad to see that conversion performance is improved! However, on the object conversion side we would still be constructing new Java arrays; would it be possible to allow passing in a reusable array? Under high memory pressure (which is often the case when Mapbox is running on top of another app), the goal should be eliminating all non primitive JVM object creations to avoid GC as much as possible.

@tobrun
Copy link
Member

tobrun commented Nov 21, 2019

@edisonw, @zmiao is running point on the c++ changes, I will make sure your suggestion will be implemented on the Android side. Thank you for the suggestion and the sample code!

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.

7 participants