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

Add batch mode of screen location #554

Merged
merged 3 commits into from Mar 30, 2021

Conversation

OttyLab
Copy link
Contributor

@OttyLab OttyLab commented Feb 21, 2021

Summary

toScreenLocationBatch is created for the better performance.

Background

I want to put a rich marker that is implemented as a Flutter Widget instead of using Symbol Layer. The idea is as same as that of Android MarkerView implementation.

This approach requires toScreenLocation query to update marker location on the screen every time the camera position is changed. toScreenLocation is implemented as a method channel and it is not performant.

The camera moving callback is called more than 10 times per second. Therefore, if the number of marker is 100, the API call should be less than 1ms. It's depends on the device but some fail to meet this requirement and the animation gets heavy.

Implementation

toScreenLocationBatch is newly defined. List<List<LatLng>> is a parameter and returned value is List<List<Point>>. The parameter and return value orders are garanteeed.

Sample code

  1. camera tracking is enabled trackCameraPosition: true,
  2. camera moving call back is handled in
    controller.addListener(() {
      if (controller.isCameraMoving) {
        _updateMarkerPosition();
      }
    });
  void _onCameraIdleCallback() {
    _updateMarkerPosition();
  }
  1. new position is calculated
  void _updateMarkerPosition() {
    var param = <LatLng>[];
    for (var marker in markers) {
      param.add(marker.Coordinate);
    }

    mapController.toScreenLocationBatch(param).then((points){
      for (var i = 0; i < markers.length; i++){
        var point = Point(points[i].x, points[i].y);
        markers[i].Position = point;
      }
    });
  }

Result

x-axis shows the number of toScreenLocation or toScreenLocationBatch call and y-axis shows the time. Blue lines show the result of toScreenLocation while red lines show toScreenLocationBatch. According to the result, toScreenLocationBatch significantly improves the performance.

Galaxy Note9

Pixel 4a

iPhone 12

Screenshot

output.mp4

Review required items

  • if the SDK minimum version update (to v2.1.0) is fine. (To my knowledge, flutter-mapbox-gl also requires v2.1.0)
  • if the implementation toScreenLocationBatch is acceptable
  • if method channel for each platform are correctly implemented
  • if the sample code is efficient

@OttyLab OttyLab force-pushed the add-batch-toScreenLocation branch 2 times, most recently from 5f06f75 to d1373f4 Compare February 22, 2021 02:33
Copy link
Collaborator

@felix-ht felix-ht left a comment

Choose a reason for hiding this comment

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

LGTM besides the todo - very nice to have this feature! @shroff would be great if you could review

example/lib/custom_marker.dart Outdated Show resolved Hide resolved
Copy link
Collaborator

@shroff shroff left a comment

Choose a reason for hiding this comment

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

The code to perform the batch coords to position translation LGTM, but naming in the example code is quite confusing and should be fixed before landing.

example/lib/custom_marker.dart Outdated Show resolved Hide resolved
example/lib/custom_marker.dart Outdated Show resolved Hide resolved
example/lib/custom_marker.dart Outdated Show resolved Hide resolved
example/lib/custom_marker.dart Outdated Show resolved Hide resolved
example/lib/custom_marker.dart Outdated Show resolved Hide resolved
example/lib/custom_marker.dart Outdated Show resolved Hide resolved
example/lib/custom_marker.dart Outdated Show resolved Hide resolved
example/lib/custom_marker.dart Outdated Show resolved Hide resolved
example/lib/custom_marker.dart Outdated Show resolved Hide resolved
lib/src/controller.dart Outdated Show resolved Hide resolved
mapbox_gl_web/lib/src/mapbox_map_controller.dart Outdated Show resolved Hide resolved
@OttyLab
Copy link
Contributor Author

OttyLab commented Mar 7, 2021

@shroff
Thank you for the review. I'll update them.

@OttyLab OttyLab force-pushed the add-batch-toScreenLocation branch 2 times, most recently from 302cdbf to afcfa81 Compare March 20, 2021 13:20
@OttyLab OttyLab requested a review from shroff March 20, 2021 13:23
Copy link
Collaborator

@shroff shroff left a comment

Choose a reason for hiding this comment

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

Not tested after the changes, but code LGTM.

Comment on lines 198 to 199
reply.append(Double(returnVal.x))
reply.append(Double(returnVal.y))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as in java, better to construct a list of the known size and insert at index rather than appending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shroff

Thanks, fixed.

@OttyLab
Copy link
Contributor Author

OttyLab commented Mar 30, 2021

@shroff @felix-ht
Can I merge this PR?

Copy link
Collaborator

@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.

Appreciate the PR @OttyLab! 🚀
The performance isn't that great but we can iterate on it.

@tobrun tobrun merged commit 489bbd8 into flutter-mapbox-gl:master Mar 30, 2021
@AAverin
Copy link
Contributor

AAverin commented Apr 10, 2021

I am getting this on v11 release now

/flutter-mapbox-gl/lib/src/controller.dart:824:46: Error: The method 'toScreenLocationBatch' isn't defined for the class 'MapboxGlPlatform'.
 - 'MapboxGlPlatform' is from 'package:mapbox_gl_platform_interface/mapbox_gl_platform_interface.dart' ('/Users/aaverin/.pub-cache/git/flutter-mapbox-gl-5df934232034988c516da11c6c72f4eee119822c/mapbox_gl_platform_interface/lib/mapbox_gl_platform_interface.dart').
Try correcting the name to the name of an existing method, or defining a method named 'toScreenLocationBatch'.
    return MapboxGlPlatform.getInstance(_id).toScreenLocationBatch(latLngs);
                                             ^^^^^^^^^^^^^^^^^^^^^

@OttyLab
Copy link
Contributor Author

OttyLab commented Apr 11, 2021

@AAverin

Thank you for reporting the issue. How can I reproduce the error?

Running sample code in SDK nor creating a simple app from the scratch did not reproduce it.

@AAverin
Copy link
Contributor

AAverin commented Apr 11, 2021

@OttyLab I get when I just update from 0.10.0 to 0.11.0. What could be important is that library is used as path, that points to my fork. In my fork I checkout 0.11.0 tag and then do pub get in the main project.

@OttyLab
Copy link
Contributor Author

OttyLab commented Apr 17, 2021

@AAverin

When I create a simple app with 0.10.0 (getting via Pub.dev) and upgrading to 0.11.0 does not reproduce the issue.

What could be important is that library is used as path, that points to my fork. In my fork I checkout 0.11.0 tag and then do pub get in the main project.

Does this mean you forked flutter-mapbox-gl and your application directly refers to the local flutter-mapbox-gl as a path? In that case, does your local fork implements toScreenLocationBatch?

@tobrun tobrun mentioned this pull request May 3, 2021
m0nac0 referenced this pull request in maplibre/flutter-maplibre-gl Oct 5, 2021
* Change iOS dependencies to Maplibre

* Update MapLibre dependency

* Set iOS deployment target to 9.0

* fix Podfile syntax

* Podfile: comment out platform key

* set MinimumOSVersion: 9.0; uncomment Podfile:platform key

* CI: verbose iOS build

* Update IPHONEOS_DEPLOYMENT_TARGET to 9.0

* [web] Fix Mapbox GL JS CSS embedding (#551)

Fixes #514

* iOS: update Podfile to fix CI (#565)

* Update deprecated patterns to fix CI static analysis  (#568)

* [offline][android] Add setOffline method (#537)

* Add batch mode of screen location (#554)

* Change SDK version of example

* Add toScreenLocationBatch

* Add custom marker example

* Define which annotations consume tap events (Multiple annotations on click area management) (#575)

* AnnotationClickOrder working (not order but activation/desactivation)

* Switch to annotationConsumeTapEvents method

* Remove failed offline pack downloads (#583)

When a download error occurs, the app is informed through the channel so
that it can display an appropriate error. However, on subsequent calls
to list the available downloaded packs it is surprising to see that the
failed region download is present as if it had been downloaded.

There is no way for the app to repair this incomplete pack. Therefore,
it seems much better to clean up the failed download as soon as it's
detected. If any tiles were downloaded they will be at least temporarily
in the cache and available to a future re-download attempt. This follows
the same logic as failures caused by hitting the tile limit.

* [docs] update changelog for v0.11.0 release (#584)

* [release] update pubspec for v0.11.0 (#585)

* add annotationOrder to web (#588)

* emits onTap only for the feature above the others (#589)

* [web] update image dependency to ^3.0.2 (#598)

* [android] bumpn release to v9.6.0 and use SDK registry distribution mechanism (#489)

* [iOS][Android] Batch creation/removal for circles, fills and lines (#576)

* added batch mode

* added circle support

* added remove features

* removed test code

* inital android implementation

* added documentation

* removed unused code

* add batch example

* fix feature mamager on release build (#593)

* Improve description to enable location features (#596)

* Update to Mapbox-iOS-SDK 6.3.0 (#513)

* Update to Mapbox-iOS-SDK 6.3.0

The 6.0.0 upgrade had no breaking changes:

> This major release does not include any breaking changes to public
> APIs. We are treating this release as a SEMVER major change because
> our installation instructions have changed.
https://github.com/mapbox/mapbox-gl-native-ios/releases/tag/ios-v6.0.0

I did however need to cast the `accessToken` as a `String` to satisfy
the compiler after this upgrade.

Prior to the upgrade I was seeing crashes from this bug, which was fixed
in 6.2: mapbox/mapbox-gl-native-ios#485

So far I don't see any issues at all after the upgrade.

Developers will need to configure their access token in order to fetch
the mapbox sdk for this an future versions, described here:
https://docs.mapbox.com/ios/maps/guides/install/

* Setup .netrc for iOS mapbox sdk download

Writes a `~/.netrc` file containing the credentials needed to download
the iOS mapbox sdk. I'm not experienced with github workflow so I'm just
hoping that I can write to the home directory of whatever container it
runs in. This should allow curl to find and use the given credentials.

Co-authored-by: tobrun <tobrun.van.nuland@gmail.com>

* [docs] update changeog for v0.12.0 (#602)

* [release] v0.12.0 (#603)

* fix: update MapLibre dependencies and imports.

* fix: update String cast

* Fix maplibre dependency (org.maplibre.gl has migrated to maven central)

* Update iOS MapLibre to 5.12.0

* Bump MapLibreAnnotationExtension to 0.0.1-beta.3

* Update maplibre_gl.podspec

* [iOS] - Fix compilation issue with maplibre 5.12.0

* Update CHANGELOG.md

Co-authored-by: m0nac0 <58807793+m0nac0@users.noreply.github.com>
Co-authored-by: shroff <ashroff6@gmail.com>
Co-authored-by: Yoshikage Ochi <yoshikage.ochi@mapbox.com>
Co-authored-by: Stephane Trepier <stephane.trepier@gmail.com>
Co-authored-by: Nathan <nathan@transit.app>
Co-authored-by: Tobrun <tobrun.van.nuland@gmail.com>
Co-authored-by: Andrea Valenzano <andr3a689@gmail.com>
Co-authored-by: Ahmed <ahm322@hotmail.com>
Co-authored-by: Felix Horvat <felix.horvat@ocell.aero>
Co-authored-by: Ettore Atalan <atalanttore@googlemail.com>
Co-authored-by: Konrad Koeppe <konrad.koeppe@bareways.com>
Co-authored-by: Vincent Berthet <vincent@web-74.com>
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

5 participants