Decode tile features to `dart:ui` and render in tile space #12

blaugold · 2022-02-07T12:04:47Z

With this change, tile feature geometry is decoded from the raw vector tile geometry to Path and Offset. This means that Paths and Offsets don't have to be constructed during each render pass. Also, no itermediate lists need to be created during decoding.

The decoded geometry is in tile space. By rendering in tile space the projection to screen space is performed by the render engine, which can take advantage of GPU acceleration.

Before this change:

RenderPicture(zoom: 0.0, preprocessTile: false)(RunTime): 98281.90476190476 us.
RenderPicture(zoom: 0.0, preprocessTile: true)(RunTime): 35279.333333333336 us.
RenderPicture(zoom: 12.0, preprocessTile: false)(RunTime): 75429.37037037036 us.
RenderPicture(zoom: 12.0, preprocessTile: true)(RunTime): 37234.166666666664 us.
RenderPicture(zoom: 24.0, preprocessTile: false)(RunTime): 84570.0 us.
RenderPicture(zoom: 24.0, preprocessTile: true)(RunTime): 43621.52173913043 us.

After this change:

RenderPicture(zoom: 0.0, preprocessTile: false)(RunTime): 54389.18918918919 us.
RenderPicture(zoom: 0.0, preprocessTile: true)(RunTime): 8515.063829787234 us.
RenderPicture(zoom: 12.0, preprocessTile: false)(RunTime): 46599.09302325582 us.
RenderPicture(zoom: 12.0, preprocessTile: true)(RunTime): 11793.017647058823 us.
RenderPicture(zoom: 24.0, preprocessTile: false)(RunTime): 57811.0 us.
RenderPicture(zoom: 24.0, preprocessTile: true)(RunTime): 18780.168224299065 us.

greensopinion · 2022-02-07T14:58:56Z

This is really cool. Thanks for the PR!

Great to see the performance metrics.

Have you looked into whether this will work with isolates? I had avoided any dart:ui references in the model intentionally, thinking that it wouldn't work due to lack of dart:ui bindings.

What are your plans with this PR? There are a few things that could use some attention. e.g. CI build is failing and there is at least one spot that looks unfinished (feature.paths.first)
I'll leave some comments in the code where I have questions.

greensopinion

Nice work!

greensopinion · 2022-02-07T15:03:12Z

lib/src/features/feature_renderer.dart

+  final double pixelsPerTileUnit;
+
+  @override
+  LabelSpace get labelSpace => _context.labelSpace;


It's unfortunate that we need all of this boilerplate so that we can pass around pixelsPerTileUnilt. Would it make sense to split out a separate class, e.g. PixelSpaceMapper that encapsulates the new logic, and make it a member of Context?

greensopinion · 2022-02-07T15:04:05Z

lib/src/features/feature_renderer.dart

+  }
+
+  bool isPathWithinTileClip(Path path) {
+    return tileClip.overlaps(rectFromTileToPixels(path.getBounds()));


This will get called quite a lot while rendering. How about having a pixelTileClip member that is converted from tile to pixels once, then we can check overlaps directly without converting.

greensopinion · 2022-02-07T15:07:36Z

lib/src/features/line_renderer.dart

+      logger,
+    );
+
+    final effectivePaint = style.linePaint!.paint(evaluationContext);


we could use ?. here

greensopinion · 2022-02-07T15:08:17Z

lib/src/features/symbol_line_renderer.dart

-    final lines = feature.lines;
-    logger.log(() => 'rendering linestring symbol');
+    // What if the feature has multiple paths?
+    final path = feature.paths.first;


What's the plan here? Will we ever have multiple paths?

Thinking on how LabelSpace works, this might have the same effect if the first line ends up placing the label (since labels can only appear once in LabelSpace) but if the first path ends up with a collision with some other label, then another path might cause the label to be rendered.

I'm not sure if this ever worked before.

My bad. I can just do this in the same the previous version did.

Do you remember why you implemented _findMiddleMetric they way you did? Why not just iterate throuh all PathMetrics and use the first one that can be used. I could not find anything on the ordering of the result of Path.computeMetrics.

But yes I found instances of symbol linestrings with multiple lines.

greensopinion · 2022-02-07T15:11:34Z

lib/src/features/symbol_line_renderer.dart

+      return;
+    }
+
+    final textAbbr = TextAbbreviator().abbreviate(text);


I'd prefer to spell things out in full to avoid creating a barrier to others reading the code. We should go wth text or textAbbreviation

lib/src/features/symbol_line_renderer.dart

greensopinion · 2022-02-07T15:13:15Z

lib/src/features/symbol_point_renderer.dart

+
+    logger.log(() => 'rendering symbol points');
+
+    final textApprox =


same re: abbreviations

lib/src/model/geometry_decoding.dart

lib/src/model/tile_model.dart

blaugold · 2022-02-07T15:19:47Z

Have you looked into whether this will work with isolates? I had avoided any dart:ui references in the model intentionally, thinking that it wouldn't work due to lack of dart:ui bindings.

With this approach decoding the feature geometry does not work in normal isolates that are spawned by the UI isolate. If decoding in a different isolate is still necessary flutter_isolate could be a path. The raw vector tiles and feature properties could still be decoded in a normal isolate.

What are your plans with this PR? There are a few things that could use some attention. e.g. CI build is failing and there is at least one spot that looks unfinished (feature.paths.first)

I think CI is failing because it uses a new version of Flutter which has a few new analyzer rules. Might be best to fix that first and rebase.

greensopinion · 2022-02-07T15:59:14Z

I couldn’t find docs on it either. I did it this way initially to try it out. The results were good enough so I didn't come back to it.

…

On Mon, Feb 7, 2022 at 7:56 AM Gabriel Terwesten ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/src/features/symbol_line_renderer.dart <#12 (comment)> : > final textPaint = style.textPaint; final textLayout = style.textLayout; if (textPaint == null || textLayout == null) { logger.warn(() => 'line symbol does not have a text paint or layout'); return; } - final lines = feature.lines; - logger.log(() => 'rendering linestring symbol'); + // What if the feature has multiple paths? + final path = feature.paths.first; My bad. I can just do this in the same the previous version did. Do you remember why you implemented _findMiddleMetric they way you did? Why not just iterate throuh all PathMetrics and use the first one that can be used. I could not find anything on the ordering of the result of Path.computeMetrics. — Reply to this email directly, view it on GitHub <#12 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEKDQ7QMK25WBEG3H626AM3UZ7TRJANCNFSM5NXH7KAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.*** com>

blaugold · 2022-02-07T16:33:41Z

I have addressed most of your comments but still have to add some tests.

greensopinion

This is really great. Thanks for adding tests.

I've given it a try with my latest flutter-vector-map-tiles experiment and so far the results look good subjectively, and in the profiler.

The implication of this change is that TileFactory and tileset preprocessing must occur on the UI thread. This will potentially affect latency of tile loading, and it means that we will need to do some expensive work on the UI thread.

Tile loading latency occurs anyways, so a small addition in delay there probably won't matter. It could contribute to flashing when one tile is unloaded and others are not yet loaded. Previously I mitigated this issue by rendering the previous tiles until the new ones are loaded, but ran into a memory issue that caused crashes due to excessive RSS growth. We can continue to explore that path. Another option to mitigate this is improving how the map background is rendered.

As for performing expensive work on the UI thread, a couple of options come to mind that might help to mitigate. We could be careful about when to schedule that work, for example if it's done one at a time in between frames it may not be problematic. Alternatively we could look at running with ui bindings on an isolate, but I'd prefer to avoid that if possible.

Have you had any further ideas about potential impact of this change?

One minor issue below, see comment inline.

greensopinion · 2022-02-09T02:08:43Z

lib/src/themes/theme_layers.dart

+
+    final layer = layers.first;
+
+    context.tileSpaceMapper = TileSpaceMapper(


is there a reason we can't pass the TileSpaceMapper in the constructor?

I wanted to avoid making tileSpaceMapper nullable because when it is actually used it is never null. We have to set it in the ThemeLayer because before that, we don't know what extent the ~~layer~~ tile has we are rendering. A tileset could have tiles with different extents.

blaugold · 2022-02-09T16:09:52Z

The implication of this change is that TileFactory and tileset preprocessing must occur on the UI thread. This will potentially affect latency of tile loading, and it means that we will need to do some expensive work on the UI thread.

In a next step, we could refactor TileFeature a bit to remove the decoded dart:ui geometry so that is only has the raw geometry list from the raw tile. Then we can still do all the protobuf and feature property decoding in another isolate, as well as the preprocessing. The UI isolate than only has to decode the geometry. One idea would be to use an Expando to attach the decoded geometry to the TileFeature in the UI isolate.

As for performing expensive work on the UI thread, a couple of options come to mind that might help to mitigate. We could be careful about when to schedule that work, for example if it's done one at a time in between frames it may not be problematic. Alternatively we could look at running with ui bindings on an isolate, but I'd prefer to avoid that if possible.

Carefully scheduling when to decode the geometry and when to paint new tiles would be my first choice as well.

greensopinion · 2022-02-09T16:41:31Z

we could refactor TileFeature a bit to remove the decoded dart:ui geometry so that is only has the raw geometry list from the raw tile. Then we can still do all the protobuf and feature property decoding in another isolate, as well as the preprocessing. The UI isolate than only has to decode the geometry.

That's an excellent idea. If decoding were done on-demand (i.e. lazy), it could reduce the number of features that need to be decoded depending on the theme and whether it renders all of the features in the tile (e.g. with minzoom/maxzoom)

Do you know if it's possible to pass a null reference to a dart:ui object to an isolate? If so, we could decode geometry lazily without needing anything else special in the model. Alternatively, we could do the same with dynamic.

So we could end up with something like this:

class TileFeature {
  final TileFeatureType type;
  final Map<String, dynamic> properties;
  List<int>? _geometry;
  List<dynamic>? _paths;
  List<dynamic>? _points;

  TileFeature({
    required this.type,
    required this.properties,
    List<int> geometry
  })  : _geometry = geometry;

  get List<dynamic> paths {
    if (_paths == null) {
       _paths = decodePaths(_geometry!);
       _geometry = null;
    }
    return _paths!;
  }

   ...
}

What do you think?

I'm reluctant to merge this PR as-is since just yet since it locks us into a specific threading model that would be hard to undo. To move this forward I think we either need to improve it so that we can use isolates, or some concrete metrics related to its use in a map so that we can understand the impact of this change and a plan to address any shortcomings.

What do you think about taking the next step on this PR?

blaugold · 2022-02-09T16:54:32Z

Based on this issue (flutter/flutter#10647 (comment)) I think that importing and using types from dart:ui works, but you just cannot use them.

That's an excellent idea. If decoding were done on-demand (i.e. lazy), it could reduce the number of features that need to be decoded depending on the theme and whether it renders all of the features in the tile (e.g. with minzoom/maxzoom)

I didn't even think about that, but that's a nice side effect :)

I'll go ahead and refactor TileFeature like you suggested.

greensopinion

This change is fantastic. Great that it was so easy to make it workable with isolates.

To make it work correctly with flutter maps this commit is needed, otherwise we end up attempting to pass ui objects to an isolate.

See a minor question below.

greensopinion · 2022-02-09T18:50:40Z

lib/src/model/tile_model.dart

-}
+  List<Offset> get points {
+    if (type != TileFeatureType.point) {
+      throw Exception('Feature does not have points');


why Exception here and StateError below?

Good catch. They should both be StateError.

greensopinion · 2022-02-10T15:39:41Z

nice work, thanks for the contribution!

blaugold · 2022-02-10T15:56:52Z

Did you mean to squash when merging? Right now all the fixup commits are in the main branch :)

greensopinion · 2022-02-10T16:36:14Z

Oops :D I've force-pushed the branch with a single commit, so you may need to reset your local if you've already pulled from main Thanks, David

…

On Thu, Feb 10, 2022 at 7:57 AM Gabriel Terwesten ***@***.***> wrote: Did you mean to squash when merging? Right now all the fixup commits are in the main branch :) — Reply to this email directly, view it on GitHub <#12 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEKDQ7X47BVJN2DRVT5M6CLU2PN47ANCNFSM5NXH7KAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you modified the open/close state.Message ID: ***@***.*** com>

blaugold mentioned this pull request Feb 7, 2022

Rendering in background isolates greensopinion/flutter-vector-map-tiles#21

Closed

greensopinion reviewed Feb 7, 2022

View reviewed changes

blaugold added 3 commits February 8, 2022 21:38

Decode tile features to dart:ui and render in tile space

af49e56

address change requests

a364ac0

Add tests for geometry decoding

af504e3

blaugold force-pushed the optimize-decoding branch from 4ca0d46 to af504e3 Compare February 8, 2022 22:28

Fixup

6b9d86b

blaugold requested a review from greensopinion February 8, 2022 23:06

greensopinion reviewed Feb 9, 2022

View reviewed changes

Lazily decode geometry in TileFeature

bde3227

greensopinion reviewed Feb 10, 2022

View reviewed changes

Fixup

b837785

greensopinion merged commit d572cca into greensopinion:main Feb 10, 2022

blaugold deleted the optimize-decoding branch July 28, 2022 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode tile features to `dart:ui` and render in tile space #12

Decode tile features to `dart:ui` and render in tile space #12

blaugold commented Feb 7, 2022 •

edited

greensopinion commented Feb 7, 2022

greensopinion left a comment

greensopinion Feb 7, 2022

blaugold Feb 7, 2022

blaugold Feb 7, 2022

greensopinion Feb 7, 2022

blaugold Feb 7, 2022

greensopinion Feb 7, 2022

blaugold Feb 7, 2022

greensopinion Feb 7, 2022

blaugold Feb 7, 2022

blaugold Feb 7, 2022

blaugold Feb 7, 2022

greensopinion Feb 7, 2022

blaugold Feb 7, 2022

greensopinion Feb 7, 2022

blaugold Feb 7, 2022

blaugold commented Feb 7, 2022

greensopinion commented Feb 7, 2022 via email

blaugold commented Feb 7, 2022

greensopinion left a comment

greensopinion Feb 9, 2022

blaugold Feb 9, 2022 •

edited

blaugold commented Feb 9, 2022

greensopinion commented Feb 9, 2022

blaugold commented Feb 9, 2022

greensopinion left a comment

greensopinion Feb 9, 2022

blaugold Feb 10, 2022

blaugold Feb 10, 2022

greensopinion commented Feb 10, 2022

blaugold commented Feb 10, 2022

greensopinion commented Feb 10, 2022 via email


		logger.log(() => 'rendering symbol points');

		final textApprox =


		final layer = layers.first;

		context.tileSpaceMapper = TileSpaceMapper(

Decode tile features to dart:ui and render in tile space #12

Decode tile features to dart:ui and render in tile space #12

Conversation

blaugold commented Feb 7, 2022 • edited

greensopinion commented Feb 7, 2022

greensopinion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaugold commented Feb 7, 2022

greensopinion commented Feb 7, 2022 via email

blaugold commented Feb 7, 2022

greensopinion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaugold Feb 9, 2022 • edited

Choose a reason for hiding this comment

blaugold commented Feb 9, 2022

greensopinion commented Feb 9, 2022

blaugold commented Feb 9, 2022

greensopinion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greensopinion commented Feb 10, 2022

blaugold commented Feb 10, 2022

greensopinion commented Feb 10, 2022 via email

Decode tile features to `dart:ui` and render in tile space #12

Decode tile features to `dart:ui` and render in tile space #12

blaugold commented Feb 7, 2022 •

edited

blaugold Feb 9, 2022 •

edited