-
-
Notifications
You must be signed in to change notification settings - Fork 859
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
refactor!: migrate tile rendering from widgets to CustomPainter
[replaced by #1908]
#1853
Conversation
These gaps are a known issue with Flutter, see
For lots of discussion. However https://verygood.ventures/blog/solving-super-dashs-rendering-challenges-eliminating-ghost-lines-for-a-seamless-gaming-experience might be the most useful for resolving this. |
Thanks @bramp Seems like just setting isAntiAlias = false resolved it. |
CustomPainter
CustomPainter
CustomPainter
@JaffaKetchup Hi. Any thoughts/progress on this? |
Converted `_defaultPaint` to `defaultTilePaint` (as a static getter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, sorry for the delay. I think this looks good overall, but there's some things to consider.
|
||
/// The [TileBuilder] is a reference to the [TileLayer]'s | ||
/// [TileLayer.tileBuilder]. | ||
final TileBuilder? tileBuilder; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
/// Paint to use when drawing each tile | ||
/// | ||
/// Defaults to [defaultTilePaint]. | ||
final Paint? tilePaint; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
setState(() { | ||
//Refresh the widget to display the loaded tile | ||
}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should deprecate this.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
(Just a thought, need to test whether this fixes #1669.) |
There are still some problems with how I implemented animated opacity. It works, but listeners must be removed properly. Not sure how to manage this. |
Need help with anything to get ahead? |
Hi @Alexays I am bit busy with project I am currently working on. I would love to get back on this, but not sure when. Feel free to take a look what you can improve. Need to figure out how to rebuild painter for Tile fade animation or just take a look what can be improved. I have been using it in production and didn't have any issues so far. So that's a good sign I guess. |
Also just need to sort out the details around the replacement for |
I'm going to try to work on this next, whenever I get time. I think this is a really promising improvement, thanks @ReinisSprogis! Is it OK to work directly on this fork? @josxha I think providing a callback for users to draw on a small part of the canvas over each tile is an adequate solution as well. |
Hi. Sounds good! I did experimenting some time ago with TileBuilder, and got it somewhat working. If TileBuilder was not null, then I used Stack and stacked it over CustomPainter (TilePainter) otherwise used TilePainter directly. But not sure if this is the best solution. |
Replaced `TileBuilder` with `TileOverlayPainter` Updated examples
CustomPainter
CustomPainter
~~
CustomPainter
~~CustomPainter
[replaced by #1908]
To bring this up to date without having to resolve conflicts, and to preserve this branch, I've opened #1908 seperately. I'll add @ReinisSprogis as a collaborator before merging. Thanks for getting this started! |
Overview
This pull request introduces a significant optimization to the map rendering process within the Flutter map. By migrating the rendering of map tiles from a Stack widget with Positioned children to a CustomPainter implementation, was achieved a remarkable improvement in performance. Initial tests demonstrate up to a 50% reduction in CPU load, leading to a smoother user experience even without the use of debouncers.
Changes Made
Map Tile Rendering: Transitioned from using Stack and Positioned widgets for displaying map tiles to drawing them directly in a CustomPainter. This approach leverages the same parameters previously used for the Stack implementation, ensuring a straightforward migration with minimal adjustments required.
Next Steps
While the initial experiment has yielded positive results, I recognize the importance of comprehensive testing and validation. The next steps will involve:
Extended Testing: Conducting a broader range of tests to validate performance improvements across various devices and use cases.
Code Review and Refinement: Get feedback from the team to refine the implementation and address any potential issues or improvements.
Documentation: Updating relevant documentation to reflect the new map rendering approach and guide future development.
Edited:
Including performance measurement. Top is using CustomPainter, bottom is Stack with Positioned widgets from demo.fleafleat.dev
Some times on demo I was getting 100% CPU load, with CustomPainter that is reduced to less than 50%