[google_maps_flutter_web] Initial support for custom overlays #3538

elitree · 2023-03-23T21:49:40Z

This is a resubmission of flutter/plugins#6982 from the now archived flutter plugins repo. I'm submitting the changes from the original author, @AsturaPhoenix. The original description is below.

Saves tile bytes to blobs and uses img elements to decode and render. Does not implement opacity, perform caching, or serve placeholder images.

Issue: Fixes flutter/flutter#98596

Known issues:

[google_maps_flutter_web] API performance bottleneck in custom tile overlays flutter#116132
Map marker visual artifacts when magnified AsturaPhoenix/trip_planner_aquamarine#22

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlay.dart

elitree · 2023-03-27T13:24:03Z

...ps_flutter/google_maps_flutter_web/example/integration_test/google_maps_controller_test.dart

Original comment by @AsturaPhoenix:

Leaving a comment here because it wasn't obvious to me the second time I looked at it:

This test was removed because it became trivial once using Mockito verification; it essentially tests to make sure that Marker.infoWindow defaults to InfoWindow.noText. On the other hand, the argument could be made to keep it anyway (e.g. to construct the verification with an explicit infoWindow: InfoWindow.noText); let me know if you'd prefer that.

elitree · 2023-03-27T13:29:43Z

FYI @ditman, I think you were the reviewer on the original PR when it was in the plugins repo, and I see that you're the reviewer here as well. I'm also interested in seeing custom tiles work for the web side of google maps flutter, so I asked @AsturaPhoenix if I could move his PR to the new repo and he's cool with that (thanks AP!). That's the background for this one.

Let me know if you have any questions and I can try to answer them or solicit AP's help if needed. Thanks!

elitree · 2023-04-03T17:32:29Z

Resolved a changelog conflict and rebased on main.

AsturaPhoenix · 2023-04-03T21:27:12Z

FYI I just moved over my local repo as well, so that's one less hurdle if we do end up needing to collaborate. Thanks for taking over @elitree!

stuartmorgan-g · 2023-05-16T19:47:59Z

Update from triage: waiting for @ditman to have review bandwidth for maps PRs.

elitree · 2023-05-17T19:11:04Z

Rebased on main to resolve conflicts.

stuartmorgan-g · 2023-07-13T18:57:49Z

@ditman Is this still in your review queue?

ditman · 2023-07-14T00:17:55Z

Absolutely

ditman

I want to tinker with this PR, is there any example that uses the tile overlays feature so I can see it working? Maybe the google_maps_flutter example app?

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart

packages/google_maps_flutter/google_maps_flutter_web/pubspec.yaml

ditman · 2023-07-15T00:28:17Z

PS: There seems some tests breaking, it seems that the Mocks need to be regenerated (and committed):

https://cirrus-ci.com/task/5045655211606016?logs=drive#L55-L73

(Mocks can be (re)generated with: dart run build_runner build --delete-conflicting-outputs inside the example directory IIRC; there should be a build.yaml file with a little bit of config for the build runner)

elitree · 2023-07-17T16:31:53Z

PS: There seems some tests breaking, it seems that the Mocks need to be regenerated (and committed):

https://cirrus-ci.com/task/5045655211606016?logs=drive#L55-L73

(Mocks can be (re)generated with: dart run build_runner build --delete-conflicting-outputs inside the example directory IIRC; there should be a build.yaml file with a little bit of config for the build runner)

I updated the mocks today as well, thanks for the reminder.

ditman · 2023-07-19T06:00:09Z

I did some small changes to the _getTile method that should make it slightly faster, I'll publish this ASAP (I got a little bit distracted with the _DebugTileProvider on the example app that looked quite bad in the implementation (it wasn't respecting the 256px logical tile size, and it looked all stretched/scaled when testing on the web... I made it dpi-aware so it kinda looks sharper!)

ditman · 2023-07-19T07:51:29Z

I had some code ready to remove package:collection, but I just noticed that it's already used in the plugin here, so I guess it can stay 😭

ditman · 2023-07-19T07:55:17Z

packages/google_maps_flutter/google_maps_flutter_web/example/integration_test/overlay_test.dart

This PR used to expect that img.hidden was true before load, and false afterwards. Is hidden really required? In my testing I didn't see any difference so I removed the attribute. @AsturaPhoenix / @elitree do you remember why was the attribute added in the first place, is it required?

I think it was to prevent the image decoding from blocking the main thread, but I can't find a version without hidden + decode so I'm not sure I ever ran a comparison. In any case we still have the bottleneck of populating the image data on the Dart/Flutter side, which has to happen on the main thread on web today.

I do think there's a difference in the case of bad image data though; if Tile.data is not a valid image, I think adding the img straight and setting the src will eventually make it display a browser-dependent broken image whereas hiding and decoding wouldn't, but I don't think that was the primary motivation.

I guess it can be added back if it becomes a problem. I think a "broken image" (maybe on windows?) is even useful for you to see that there's something fishy going on with your TileProvider!

As for rendering on the main thread, you're right. The only way to avoid that on the web, however, is to have a web-only TileProvider that knows how to delegate the getTile call to a service worker and an offscreen canvas (in that case the drawing might get more complicated, or you end up with a very big service worker that also includes flutter :P)

ditman · 2023-07-19T08:05:44Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlay.dart

I am very curious about this logicalTileSize. From the API it seems that both a TileOverlay and each Tile can specify their size, so this implementation can do away with this logicalTileSize const.

Why is this fixed to 256, and do we need it?

(If this is 256, how come the example is creating 100x100 tiles? They look blurry on the web as they're scaled up to fit!)

((Also: can tiles be non-square?))

logicalTileSize is used to communicate to Google Maps the size of the tile for use in coordinate conversions and scaling. This is configurable in the JS API and can be rectangular, but I don't think it's configurable in the Android API. At a logical size of 256 x 256, the web impl appears consistent with the Android impl. I haven't tried it on iOS.

JS API: https://developers.google.com/maps/documentation/javascript/maptypes#MapTypeInterface
Android:
https://developers.google.com/maps/documentation/android-sdk/reference/com/google/android/libraries/maps/model/TileOverlay#tile-coordinates
https://developers.google.com/maps/documentation/android-sdk/tileoverlay#coordinates

The physical size can be different and will be scaled on web. I used to use this to display different levels of detail on NOAA charts from a WMS server, effectively requesting higher DPIs to show more features.

@AsturaPhoenix yes, I used the physical size being logicalSize * dpi to generate high DPI tiles too. Let's go with consistency for now, having a fixed logical size.

(PS: In iOS they also mention 256x256 tiles or 512x512 for "retina" displays: https://developers.google.com/maps/documentation/ios-sdk/tiles#high_dpi_tiles_for_retina_devices, I'm not sure if the iOS SDK supports sizes any other than those two (like a 3x tile))

packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlay.dart

ditman · 2023-07-20T21:05:47Z

When testing with the example app, the real performance hit happened here:

packages/packages/google_maps_flutter/google_maps_flutter/example/lib/tile_overlay.dart

Lines 146 to 150 in 442d9db

    
           final Uint8List byteData = await picture 
        
               .toImage(width, height) 
        
               .then((ui.Image image) => 
        
                   image.toByteData(format: ui.ImageByteFormat.png)) 
        
               .then((ByteData? byteData) => byteData!.buffer.asUint8List());

Not so much on this side of the implementation :)

(toImage and toByteData being very expensive)

AsturaPhoenix · 2023-07-20T21:07:39Z

Yeah, unfortunately the other performance changes probably won't pass muster since they involve changing the API to pass canvases as tiles instead of byte arrays. (There are also unrelated optimizations around serialization of polylines and friends, but unfortunately eventually I ran into an expensive serialization we didn't seem able to avoid within GMS core, so that was the end of that.)

This LGTM too; I have one nit comment that I'll add, but it's probably not of consequence.

AsturaPhoenix · 2023-07-20T21:09:53Z

When testing with the example app, the real performance hit happened here:

packages/packages/google_maps_flutter/google_maps_flutter/example/lib/tile_overlay.dart

Lines 146 to 150 in 442d9db

final Uint8List byteData = await picture

.toImage(width, height)

.then((ui.Image image) =>

image.toByteData(format: ui.ImageByteFormat.png))

.then((ByteData? byteData) => byteData!.buffer.asUint8List());

Not so much on this side of the implementation :)

(toImage and toByteData being very expensive)

Yes, it's quite painful. We can get a few ms back by using bmps instead of pngs, but we don't get jank-free until we pass canvases afaict. (flutter/flutter#116132)

AsturaPhoenix · 2023-07-20T06:02:32Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlays.dart

I'm the tiniest bit concerned that this might also cause any overlays on top of the overlay being updated to reload, but I haven't exercised this case, and my concern might be premature optimization.

This little bit of code concerns me a little bit as well, but we'll see in the real world :) I simplified the original if-else a little bit and ended up with this, and I might have over-simplified. I'll tag you whenever the first bug comes about this so you can tell me: "I TOLD YOU SO!" :p

Oh actually you know what, rereading this more carefully I think it does warrant another pass. For example, there actually seems to be a bug (that I almost doubt anyone would hit) where if you change the Z index of a layer that's not visible, it becomes visible. (Purely based on my reading of the code; I haven't run a test, so I might be wrong.)

It'd probably also be worthwhile to add a regression test for this case. (I can also take care of this if nobody gets around to it over the weekend.)

And, since this is in a loop, the impact of remove/insert vs. set for the stable z-index case, although probably still a premature optimization, could be compounded if it were ever actually an issue.

Basically the fix if we want to do it this way is just to remove the moved condition entirely, but you know what I'd do if it were up to me.

ditman · 2023-07-21T19:36:47Z

@AsturaPhoenix in this case I see two optimizations:

Do not generate the Uint8List in the main thread on the web (that means something like this: https://developer.chrome.com/blog/offscreen-canvas/)
Make Tile.data an Object?, so each platform can pass what's most efficient to its implementation (in mobile it might be a byte array, but as you said on the web it might be a straight-up canvas element)

AsturaPhoenix

Okay, I don't think I have permission to push to the remote, but here's the patch (also adding some additional assertions in related tests to try to catch similar regressions):

diff --git a/packages/google_maps_flutter/google_maps_flutter_web/example/integration_test/overlays_test.dart b/packages/google_maps_flutter/google_maps_flutter_web/example/integration_test/overlays_test.dart
index 4b95d8bbe..8b6b34694 100644
--- a/packages/google_maps_flutter/google_maps_flutter_web/example/integration_test/overlays_test.dart
+++ b/packages/google_maps_flutter/google_maps_flutter_web/example/integration_test/overlays_test.dart
@@ -68,6 +68,9 @@ void main() {
         tileProviders[1].getTile(any, any, any),
         tileProviders[2].getTile(any, any, any),
       ]);
+      verifyNoMoreInteractions(tileProviders[0]);
+      verifyNoMoreInteractions(tileProviders[1]);
+      verifyNoMoreInteractions(tileProviders[2]);
     });

     testWidgets('changeTileOverlays', (WidgetTester tester) async {
@@ -87,6 +90,8 @@ void main() {
         tileProviders[1].getTile(any, any, any),
       ]);
       verifyZeroInteractions(tileProviders[0]);
+      verifyNoMoreInteractions(tileProviders[1]);
+      verifyNoMoreInteractions(tileProviders[2]);

       // Re-enable overlay 0.
       controller.changeTileOverlays(
@@ -99,6 +104,22 @@ void main() {
         tileProviders[0].getTile(any, any, any),
         tileProviders[1].getTile(any, any, any),
       ]);
+      verifyNoMoreInteractions(tileProviders[0]);
+      verifyNoMoreInteractions(tileProviders[1]);
+      verifyNoMoreInteractions(tileProviders[2]);
+    });
+
+    testWidgets(
+        'updating the z index of a hidden layer does not make it visible',
+        (WidgetTester tester) async {
+      controller.addTileOverlays(<TileOverlay>{...tileOverlays});
+
+      controller.changeTileOverlays(<TileOverlay>{
+        tileOverlays[0].copyWith(zIndexParam: -1, visibleParam: false),
+      });
+
+      probeTiles();
+      verifyZeroInteractions(tileProviders[0]);
     });

     testWidgets('removeTileOverlays', (WidgetTester tester) async {
diff --git a/packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlays.dart b/packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlays.dart
index 464fb319a..aa6c19173 100644
--- a/packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlays.dart
+++ b/packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlays.dart
@@ -63,14 +63,13 @@ class TileOverlaysController extends GeometryController {

     final bool wasVisible = controller.tileOverlay.visible;
     final bool isVisible = tileOverlay.visible;
-    final bool moved = tileOverlay.zIndex != controller.tileOverlay.zIndex;

     controller.update(tileOverlay);

-    if (wasVisible || moved) {
+    if (wasVisible) {
       _remove(controller);
     }
-    if (isVisible || moved) {
+    if (isVisible) {
       _insertZSorted(controller);
     }
   }

AsturaPhoenix · 2023-07-22T17:43:40Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlays.dart

Basically the fix if we want to do it this way is just to remove the moved condition entirely, but you know what I'd do if it were up to me.

elitree · 2023-07-24T14:54:19Z

Hi @AsturaPhoenix, I sent an invite to collaborate, so maybe that will allow you to upload. I did just update the branch to resolve some conflicts, but didn't add your changes yet (~~I'm not sure I was looking at it right, but the integration test looks slightly different than what I'm seeing in my branch~~ edit: oops, I was looking at overlay_test instead of overlays_test, ha).

Please feel free to update as you see fit!

…ontrollers.

Remove/add based on visibility only.

ditman · 2023-07-24T18:22:38Z

@AsturaPhoenix, @elitree thanks for the fix over the weekend!

stuartmorgan-g

LGTM

…flutter/packages#3538)

flutter/packages@60e9a54...3dc00c1 2023-08-01 [email protected] Manual roll Flutter from 1d44fbd to 1d59196 (18 revisions) (flutter/packages#4621) 2023-07-31 [email protected] Update the cirrus key jul-31-2023 (flutter/packages#4618) 2023-07-31 [email protected] [path_provider_platform_interface] Add getApplicationCachePath() (flutter/packages#4614) 2023-07-31 [email protected] [google_maps_flutter_web] Initial support for custom overlays (flutter/packages#3538) 2023-07-31 [email protected] [camera] Removed the microphone permission request from availableCameras on Web (flutter/packages#4263) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added p: google_maps_flutter platform-web labels Mar 23, 2023

elitree force-pushed the gmaps-flutter-ap-fix branch 3 times, most recently from 5e536d5 to 026f4e4 Compare March 24, 2023 14:28

elitree commented Mar 27, 2023

View reviewed changes

elitree marked this pull request as ready for review March 27, 2023 13:26

elitree requested a review from ditman as a code owner March 27, 2023 13:26

elitree force-pushed the gmaps-flutter-ap-fix branch from f71eaf0 to b668a6f Compare April 3, 2023 17:31

elitree force-pushed the gmaps-flutter-ap-fix branch from 3435e6f to 1ba5ef7 Compare May 17, 2023 19:09

elitree force-pushed the gmaps-flutter-ap-fix branch from 1ba5ef7 to b7e167e Compare June 8, 2023 15:07

ditman reviewed Jul 14, 2023

View reviewed changes

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart Outdated Show resolved Hide resolved

packages/google_maps_flutter/google_maps_flutter_web/pubspec.yaml Outdated Show resolved Hide resolved

elitree force-pushed the gmaps-flutter-ap-fix branch 3 times, most recently from 179bff4 to 0b5faf3 Compare July 17, 2023 16:27

ditman reviewed Jul 19, 2023

View reviewed changes

AsturaPhoenix reviewed Jul 20, 2023

View reviewed changes

packages/google_maps_flutter/google_maps_flutter_web/lib/src/overlay.dart Outdated Show resolved Hide resolved

ditman force-pushed the gmaps-flutter-ap-fix branch from 18003b2 to a324854 Compare July 20, 2023 05:38

AsturaPhoenix reviewed Jul 20, 2023

View reviewed changes

AsturaPhoenix reviewed Jul 22, 2023

View reviewed changes

elitree added 3 commits July 24, 2023 10:43

[google_maps_flutter_web] Initial support for custom overlays

b9e54cd

update changelog, tweak test

c53c48b

rebased and regenerated mocks (and fixed submitting user)

bae3ab7

elitree force-pushed the gmaps-flutter-ap-fix branch from a324854 to bae3ab7 Compare July 24, 2023 14:52

ditman and others added 5 commits July 24, 2023 09:34

Do not await img.decode. Async revoke blob url.

c262cd0

Do not assert the img.hidden attribute (removed)

7884981

Add hidden and decoding async

e160254

Make the TileOverlay controllers more similar to the other Geometry c…

a5cbfb4

…ontrollers.

Fix regression from z update simplification.

f6ea74f

Remove/add based on visibility only.

stuartmorgan-g approved these changes Jul 31, 2023

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 31, 2023

auto-submit bot merged commit 9e21922 into flutter:main Jul 31, 2023

engine-flutter-autoroll mentioned this pull request Aug 1, 2023

Roll Packages from 60e9a542f94e to 3dc00c1a32f6 (5 revisions) flutter/flutter#131672

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 1, 2023

9e21922 [google_maps_flutter_web] Initial support for custom overlays (…

cb65606

…flutter/packages#3538)

engine-flutter-autoroll mentioned this pull request Aug 1, 2023

Roll Packages from 60e9a542f94e to 3dc00c1a32f6 (5 revisions) flutter/flutter#131692

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 1, 2023

9e21922 [google_maps_flutter_web] Initial support for custom overlays (…

9f51100

…flutter/packages#3538)

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

elitree deleted the gmaps-flutter-ap-fix branch August 24, 2023 19:28

[google_maps_flutter_web] Initial support for custom overlays #3538

[google_maps_flutter_web] Initial support for custom overlays #3538

Uh oh!

Conversation

elitree commented Mar 23, 2023 • edited by stuartmorgan-g Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elitree commented Mar 27, 2023

Uh oh!

elitree commented Apr 3, 2023

Uh oh!

AsturaPhoenix commented Apr 3, 2023

Uh oh!

stuartmorgan-g commented May 16, 2023

Uh oh!

elitree commented May 17, 2023

Uh oh!

stuartmorgan-g commented Jul 13, 2023

Uh oh!

ditman commented Jul 14, 2023

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ditman commented Jul 15, 2023

Uh oh!

elitree commented Jul 17, 2023

Uh oh!

ditman commented Jul 19, 2023

Uh oh!

ditman commented Jul 19, 2023

Uh oh!

ditman Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ditman commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AsturaPhoenix commented Jul 20, 2023

Uh oh!

AsturaPhoenix commented Jul 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AsturaPhoenix Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman commented Jul 21, 2023

Uh oh!

AsturaPhoenix left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elitree commented Mar 23, 2023 •

edited by stuartmorgan-g

Loading

ditman Jul 19, 2023 •

edited

Loading

ditman Jul 19, 2023 •

edited

Loading

ditman commented Jul 20, 2023 •

edited

Loading

AsturaPhoenix Jul 22, 2023 •

edited

Loading

AsturaPhoenix left a comment •

edited

Loading

elitree commented Jul 24, 2023 •

edited

Loading