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

refactor!: migrate tile rendering from widgets to CustomPainter [replaced by #1908] #1853

Closed
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/src/layer/tile_layer/tile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import 'package:flutter/widgets.dart';
import 'package:flutter_map/flutter_map.dart';

/// The widget for a single tile used for the [TileLayer].
/// If proceed with the implementation of the CustomPainter and the TileModel this Widget should be removed.
Copy link
Member

Choose a reason for hiding this comment

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

We should deprecate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deprecated it initially, but tests didn't pass anymore, as class _TileState extends State<Tile> was using deprecated Tile class. So I am not sure how to manage this? Just delete class altogether?

Copy link
Member

@JaffaKetchup JaffaKetchup Mar 24, 2024

Choose a reason for hiding this comment

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

Deprecation is transitive in some way. Mark both classes as deprecated, and it should work. However, just don't worry about this for now, and I'll do it at the end to make sure it's correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tile is not exported publically so it should just get removed with this pr. It got completely replaced it with TileModel

@immutable
class Tile extends StatefulWidget {
/// [TileImage] is the model class that contains meta data for the Tile image.
Expand Down
75 changes: 60 additions & 15 deletions lib/src/layer/tile_layer/tile_layer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@ import 'package:collection/collection.dart' show MapEquality;
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter_map/flutter_map.dart';
import 'package:flutter_map/src/layer/tile_layer/tile.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_bounds/tile_bounds.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_bounds/tile_bounds_at_zoom.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_image_manager.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_model.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_painter.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_range.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_range_calculator.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_scale_calculator.dart';
Expand Down Expand Up @@ -227,6 +228,39 @@ class TileLayer extends StatefulWidget {
/// disabled and the tile layer will update as soon as possible.
final Duration loadingDelay;

///Allows to set different paint parameters that are used in CustomPainter to draw tiles.
///If provided, it is recommended to set filterQuality to high. Setting isAntiAlias to true is not recommended as it may introduce ghost lines between tiles.
///
///Example: Setting a color filter to draw tiles in grayscale.
///```dart
/// paint: Paint()..colorFilter = const ColorFilter.mode(Colors.grey, BlendMode.saturation)..filterQuality = FilterQuality.high,
/// ```
///
/// Example: Inverting the colors of the tiles.
/// ```dart
/// paint: Paint()..invertColors = true ..filterQuality = FilterQuality.high,
/// ```
///
/// It is possible to change map colors in various ways using the ColorFilter.matrix.
///
/// Example: Toning map with sepia color.
/// ```dart
/// paint: Paint()..colorFilter = const ColorFilter.matrix([
/// 0.393, 0.769, 0.189, 0, 0,
/// 0.349, 0.686, 0.168, 0, 0,
/// 0.272, 0.534, 0.131, 0, 0,
/// 0, 0, 0, 1, 0,
/// ])..filterQuality = FilterQuality.high,
/// ```
///
/// If not specified, the default paint parameters are:
/// ```dart
/// Paint()
/// ..isAntiAlias = false
/// ..filterQuality = FilterQuality.high;
/// ```
final Paint? paint;

/// Create a new [TileLayer] for the [FlutterMap] widget.
TileLayer({
super.key,
Expand Down Expand Up @@ -259,6 +293,7 @@ class TileLayer extends StatefulWidget {
this.reset,
this.tileBounds,
this.loadingDelay = Duration.zero,
this.paint,
TileUpdateTransformer? tileUpdateTransformer,
String userAgentPackageName = 'unknown',
}) : assert(
Expand Down Expand Up @@ -543,26 +578,28 @@ class _TileLayerState extends State<TileLayer> with TickerProviderStateMixin {
// cycles saved later on in the render pipeline.
final tiles = _tileImageManager
.getTilesToRender(visibleRange: visibleTileRange)
.map((tileImage) => Tile(
// Must be an ObjectKey, not a ValueKey using the coordinates, in
// case we remove and replace the TileImage with a different one.
key: ObjectKey(tileImage),
scaledTileSize: _tileScaleCalculator.scaledTileSize(
map.zoom,
tileImage.coordinates.z,
),
currentPixelOrigin: map.pixelOrigin,
tileImage: tileImage,
tileBuilder: widget.tileBuilder,
))
.map(
(tileImage) => TileModel(
// Must be an ObjectKey, not a ValueKey using the coordinates, in
// case we remove and replace the TileImage with a different one.
key: ObjectKey(tileImage),
scaledTileSize: _tileScaleCalculator.scaledTileSize(
map.zoom,
tileImage.coordinates.z,
),
currentPixelOrigin: map.pixelOrigin,
tileImage: tileImage,
tileBuilder: widget.tileBuilder,
),
)
.toList();

// Sort in render order. In reverse:
// 1. Tiles at the current zoom.
// 2. Tiles at the current zoom +/- 1.
// 3. Tiles at the current zoom +/- 2.
// 4. ...etc
int renderOrder(Tile a, Tile b) {
int renderOrder(TileModel a, TileModel b) {
final (za, zb) = (a.tileImage.coordinates.z, b.tileImage.coordinates.z);
final cmp = (zb - tileZoom).abs().compareTo((za - tileZoom).abs());
if (cmp == 0) {
Expand All @@ -573,7 +610,12 @@ class _TileLayerState extends State<TileLayer> with TickerProviderStateMixin {
}

return MobileLayerTransformer(
child: Stack(children: tiles..sort(renderOrder)),
child: CustomPaint(
size: Size.infinite,
willChange: true,
painter: TilePainter(
tiles: tiles..sort(renderOrder), tilePaint: widget.paint),
),
);
}

Expand Down Expand Up @@ -602,6 +644,9 @@ class _TileLayerState extends State<TileLayer> with TickerProviderStateMixin {
onLoadError: _onTileLoadError,
onLoadComplete: (coordinates) {
if (pruneAfterLoad) _pruneIfAllTilesLoaded(coordinates);
setState(() {
//Refresh the widget to display the loaded tile
});
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a workaround; maybe it isn't though? Why is it required now and not before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously each tile rebuilt it's own state when image was loaded. Needed a place to call setState() to refresh TilePainter.
Seems like onLoadComplete is appropriate place to call that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now looking deeper into this I noticed that fadeIn animation is running, but not animating opacity. Perhaps can rebuild state within animation controller listener and set opacity to image in TilePainter?

Copy link
Contributor Author

@ReinisSprogis ReinisSprogis Mar 24, 2024

Choose a reason for hiding this comment

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

Opacity is updating, but if there is no rebuild it gets stuck on that opacity level. Unless rebuilt by inertia or other map interaction.
To get opacity in TilePainter added (tilePaint ?? defaultTilePaint)..color = (tilePaint ?? defaultTilePaint).color.withOpacity(tile.tileImage.opacity),
https://github.com/fleaflet/flutter_map/assets/69913791/f12260bf-bd5a-4e4e-9849-dd865896f98b

Copy link
Contributor

Choose a reason for hiding this comment

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

I think an if (mounted) should be added in case the onLoadComplete is called after the widget has been destroyed.

},
tileDisplay: widget.tileDisplay,
errorImage: widget.errorImage,
Expand Down
39 changes: 39 additions & 0 deletions lib/src/layer/tile_layer/tile_model.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import 'dart:math';
import 'package:flutter/widgets.dart';
import 'package:flutter_map/flutter_map.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_painter.dart';

/// Model for tiles displayed by [TileLayer] and [TilePainter]
class TileModel {
/// Controls how this tile is replaced by another tile
///
/// See also:
///
/// * [Widget.key]
/// * The discussions at [Key] and [GlobalKey].
final ObjectKey key;
ReinisSprogis marked this conversation as resolved.
Show resolved Hide resolved

/// [TileImage] is the model class that contains meta data for the Tile image.
final TileImage tileImage;

/// The [TileBuilder] is a reference to the [TileLayer]'s
/// [TileLayer.tileBuilder].
final TileBuilder? tileBuilder;
Copy link
Member

Choose a reason for hiding this comment

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

TileBuilder is never evaluated, and has been replaced by tilePaint. It should be deprecated and removed from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes


/// The tile size for the given scale of the map.
final double scaledTileSize;

/// Reference to the offset of the top-left corner of the bounding rectangle
/// of the [MapCamera]. The origin will not equal the offset of the top-left
/// visible pixel when the map is rotated.
final Point<double> currentPixelOrigin;

/// Creates a new instance of TileModel.
const TileModel({
required this.key,
required this.scaledTileSize,
required this.currentPixelOrigin,
required this.tileImage,
required this.tileBuilder,
});
}
62 changes: 62 additions & 0 deletions lib/src/layer/tile_layer/tile_painter.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter_map/src/layer/tile_layer/tile_model.dart';

/// Draws [TileModel]s onto a canvas at the correct position
class TilePainter extends CustomPainter {
/// List of [TileModel]s to draw to the canvas
List<TileModel> tiles;

/// Paint to use when drawing each tile
///
/// Defaults to [defaultTilePaint].
final Paint? tilePaint;
Copy link
Member

Choose a reason for hiding this comment

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

Does tilePaint give enough flexibility? TileBuilder used to give loads of flexibility, and users could do basically anything they wanted. We might want to consider what users want to do a bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what would be missing comparing with TileBuilder. User could get image from tile and modify it?

Copy link
Member

Choose a reason for hiding this comment

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

Modifying an incoming tile's bytes can be done with a TileProvider, so there's no gap there - but previously you could 'overlay'/'wrap' widgets, etc. See https://demo.fleaflet.dev/tile_builder.

I understand that positioning widgets over this kind of defeats the point of a canvas, and is one of the reasons we previously didn't use canvas, to allow this kind of easy tweaking. However, we should really provide the ability to overlay widgets at tile positions. Note that it's not possible to paint a widget to a canvas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I looked into it, then there is now way to avoid Stack. I suppose could then preserve Stack and Tile approach and use that if TileBuilder is not null else use CustomPainter?

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily. I suppose a good compromise would be to allow a point in the painting process that can be influenced by the user, ie. they can paint to the same canvas, but are constrained to a smaller Size, for each tile. Although much less convienient for the user, this will allow as much flexibility as possible.

I guess it's also worth verifying those performance improvements on a few different devices, just to see if we're making a correct decision here, and the pros outweigh the cons, IYSWIM.

Copy link
Contributor

@josxha josxha Apr 2, 2024

Choose a reason for hiding this comment

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

tile_builder has been nice to debug tile loading. I wouldn't prefer to have widgets mixed in with the canvas. Most if not everything that has been done with widgets before should be possible directly on the canvas too.
In addition to the tilePaint parameter I'd suggest to add a callback that let's users draw custom things on the canvas that gets called after the tiles draws.


/// Default [tilePaint]er
static Paint get defaultTilePaint => Paint()
JaffaKetchup marked this conversation as resolved.
Show resolved Hide resolved
..isAntiAlias = false
..filterQuality = FilterQuality.high;

/// Create a painter with the specified [TileModel]s and paint
///
/// [tilePaint] indirectly defaults to [defaultTilePaint].
TilePainter({
required this.tiles,
this.tilePaint,
});

@override
void paint(Canvas canvas, Size size) {
for (final tile in tiles) {
// Draw tiles if they have an image
if (tile.tileImage.imageInfo != null) {
// Simplify tile positions and sizes
final left = tile.tileImage.coordinates.x * tile.scaledTileSize -
tile.currentPixelOrigin.x;
final top = tile.tileImage.coordinates.y * tile.scaledTileSize -
tile.currentPixelOrigin.y;
final width = tile.scaledTileSize;
final height = tile.scaledTileSize;
final image = tile.tileImage.imageInfo!.image;

canvas.drawImageRect(
image,
// Source rectangle
Rect.fromLTWH(
0,
0,
image.width.toDouble(),
image.height.toDouble(),
),
Rect.fromLTWH(left, top, width, height), // Destination rectangle
tilePaint ?? defaultTilePaint,
);
}
}
}

@override
bool shouldRepaint(covariant TilePainter oldDelegate) {
return oldDelegate.tiles != tiles;
}
}
1 change: 0 additions & 1 deletion test/flutter_map_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ void main() {
await tester.pumpWidget(TestApp(markers: markers));
expect(find.byType(FlutterMap), findsOneWidget);
expect(find.byType(TileLayer), findsOneWidget);
expect(find.byType(RawImage), findsWidgets);
expect(find.byType(MarkerLayer), findsWidgets);
expect(find.byType(FlutterLogo), findsOneWidget);
});
Expand Down
Loading