flutter_map icon indicating copy to clipboard operation
flutter_map copied to clipboard

v3.0.0: Simplify External & Internal Layer Handling

Open mootw opened this issue 1 year ago • 11 comments

What is it?

This PR removes LayerOptions and Plugins entirely. This has multiple benefits.

LayerOptions were used as a way to pass down map state like this:

class CircleLayerOptions extends LayerOptions {

	final List<CircleMarker> circles;
	CircleLayerOptions({
		Key? key,
		this.circles = const [],
		Stream<void>? rebuild,
	}) : super(key: key, rebuild: rebuild);
}
class CircleLayer extends StatelessWidget {

	final CircleLayerOptions circleOpts;
	final MapState map;
	final Stream<void>? stream;

	CircleLayer(this.circleOpts, this.map, this.stream) : super(key: circleOpts.key);

Then inside of the build the class would need to use a StreamBuilder

Widget _build(BuildContext context, Size size) {
	return StreamBuilder<void>(
		stream: stream, // a Stream<void> or null
			builder: (BuildContext context, _) {

This is a lot of boilerplate code; confusing, and also requires a plugin registration to handle the layer options.

Removing LayerOptions entirely makes everything easier and more intuitive.

class CircleLayerWidget extends StatelessWidget {
	final List<CircleMarker> circles;

	const CircleLayerWidget({super.key, this.circles = const []});

Now, the stream is not required. You can easily get map state within the build method.

@override
Widget build(BuildContext context) {
	final map = MapState.maybeOf(context)!;

Migration of built in layers (This should be the most common migration)

Anything that uses a plugin will require more effort, but I have already migrated some community plugins. BEFORE

layers: [
	TileLayerOptions(
		urlTemplate: 'examplexyz',
		tileProvider: NetworkTileProvider(),
	),
	MarkerLayerOptions(markers: markers)
],

AFTER

children: [
	TileLayer(
		urlTemplate: 'examplexyz',
		tileProvider: NetworkTileProvider(),
	)),
	MarkerLayer(markers: markers)
],

TODO:

  • [ ] Docs still need to be updated.
  • [ ] TileLayer still listens to events and calls setState on itself. It could be migrated to only update on build.

Other changes

  • MarkerLayer widget has been improved with about a 10% performance increase while zooming with 5000 markers.
  • removed map onReady
  • removed streamGroups (internal and not functional)
  • Fixed a bunch of state inconsistencies
  • MapState is now FlutterMapState
  • Map now initializes during initState which means no need to "wait" for the first build
  • https://docs.fleaflet.dev/faqs/performance-issues this entire page is now incorrect (minus the last two points)

closes #1334, closes #1308

Plugin specific migration

  • .getPixelOrigin() is now pixelOrigin

I have also created a PR for some plugins migrating them preemptively:

Everything is breaking unless the plugin already used children and did not depend on another plugin

  • [x] flutter_map_tile_caching JaffaKetchup/flutter_map_tile_caching#61
  • [x] flutter_map_marker_cluster (requires flutter_map_marker_popup) lpongetti/flutter_map_marker_cluster#128
  • [x] flutter-vector-map-tiles greensopinion/flutter-vector-map-tiles#82
  • [x] flutter_map_line_editor (requires flutter_map_dragmarker) ibrierley/flutter_map_line_editor#28
  • [X] flutter_map_location_marker tlserver/flutter_map_location_marker#39
  • [x] flutter_map_marker_popup rorystephenson/flutter_map_marker_popup#51
  • [ ] map_elevation
  • [ ] flutter_map_floating_marker_titles
  • [ ] flutter_map_tappable_polyline
  • [x] flutter_map_dragmarker ibrierley/flutter_map_dragmarker#17
  • [ ] flutter_map_move_marker
  • [ ] lat_lon_grid_plugin
  • [ ] line_animator

Extra Notes

I noticed almost a 1.5x performance increase in debug mode, I did not see much or any improvement in profile mode.

State is now handled "by flutter" using setState; which rebuilds all of the map children. Event stream is now a callback for widgets above FlutterMap

Consider any other breaking changes to make at the same time?

mootw avatar Aug 03 '22 06:08 mootw

So far it looks good to me (this isn't related as happens with existing version, but when testing I wonder why on the home page all 3 markers don't display immediately..).

Can you explain the Dart 2.17 super thing a bit more. I'm using that version currently....

ibrierley avatar Aug 03 '22 10:08 ibrierley

https://dart.dev/guides/language/language-tour#super-parameters Notice in how all of the LayerWidgets, Key is now a super parameter.

JaffaKetchup avatar Aug 03 '22 10:08 JaffaKetchup

Sorry, I misread, you said older than 2.17 didn't you, not 2.17 itself. Yes, sure, good point that makes sense (unless there's some real useful reason to force it).

ibrierley avatar Aug 03 '22 10:08 ibrierley

@TheLastGimbus on your issue #824 and PR #826 you were able to fix by adding caching. I am working on a re-write of the state system to improve performance overall and found in my testing with 5000 markers on the example app that performance is better without the cache now due to how state is now handled with panning and zooming. Would you be willing to check this on your project to confirm that performance is as good or better now with my PR? Migration should be fairly easy and I can help if you have any issue.

mootw avatar Aug 03 '22 19:08 mootw

Made breaking changes to state management. Behavior is almost the same as before, but the name has changed and internally it is much different. This means I will need to migrate plugins again.

Largest changes:

  • MapState is now FlutterMapState
  • Streams are now gone; except for map events. Map events may move or get changed still. They are still bulky and make state management more difficult.
  • onMoved stream is now a callback for widgets above flutter map, and children (map layers) are automatically updated of state through build context.

Example app should have identical behavior to before; as I have not made any intentional modifications to behavior, other than ensuring state always updates, and the map now properly gets initialized in setState which means anything inside of state that is non-nullable should be initialized after initState, except for size which gets initialized after build. I have not made it nullable however.

mootw avatar Aug 04 '22 05:08 mootw

Hi @MooNag - thanks for this effort! I didn't check up on my project for some time, but I can try to see this in a few days :eyes:

See, because we had that many markers, we created our own marker layer: https://github.com/KanarekApp/flutter_map_fast_markers

fast_markers are drawn on pure canvas - and currently, we are just drawing literally two dots as marker :neutral_face: - and when I benchmarked it at the time, the map had problems even if i disabled the drawing! That's why i had issues to marker calculations themselves - but maybe things changed from then...

But the question is, now that plugins are gone, how can i test my project/add my plugin/markers to the map?

TheLastGimbus avatar Aug 06 '22 09:08 TheLastGimbus

@MooNag Does the above change anything?

JaffaKetchup avatar Aug 06 '22 17:08 JaffaKetchup

@TheLastGimbus

More or less all you need to to do migrate is remove the plugin wrapper class; then in your widget that is put inside the map children, do FlutterMapState.maybeOf(context) inside of build to get the latest map state! The map should (clearly there are still some bugs to iron out), automatically rebuild your plugin whenever the map state is changed. Just like standard widgets in flutter.

I can help you migrate your plugin to this PR, which branch of it should I migrate? I see there are multiple branches!

mootw avatar Aug 06 '22 19:08 mootw

I re-implemented the eventStream in the mapcontroller, however I might revert that change later. I did it exclusively for the "map_controller.dart" example page due to how it handles state. But it can easily cause bad practice especially with plugins. Ideally the mapcontroller can provide a registerCallback function to register multiple callbacks; however I have not figured out how to implement that yet.

@JaffaKetchup Point To LatLng hero issue is in 2.2.0 for me. I did not fix the issue with this commit but I made sure the behavior on the page was the same in both versions. The other issues are fixed.

I don't really see any significant performance changes with this PR other than maybe improved jank and more potential for flutter to optimize. I noticed less jank in debug mode but not any performance difference in release mode. I did optimize the MarkerLayer (by removing the caching process entirely) to improve the frame time while zooming by about 10% and the frame time in general by 1%. I haven't done anything with the other layers, and still need to look at the fast marker plugin. Waiting on info for which branch to migrate.

mootw avatar Aug 07 '22 05:08 mootw

@MooNag thanks for asking, but I propably need to sit with my project to see how it is doing now - i don't know when i'll have the time :confounded: . I will let you know if i will need any help

But if it interests you and you want to try on your own, canary branch is our own internal "master"

TheLastGimbus avatar Aug 07 '22 11:08 TheLastGimbus

Now available as a prerelease on pub.dev: see v3.0.0-beta.1.

JaffaKetchup avatar Aug 13 '22 11:08 JaffaKetchup

I'm fine with all of this, thanks again.

I've created beta version/branches of my plugins as a bit of a test (line_editor and dragmarker and my own vector test stuff, line_animator doesn't have any flutter_map dependency). Whilst it's a bit of work (hadn't spotted your conversions ready!), relatively painless.

I guess this is one we should give people a lot of notification and try hard to encourage people to do some testing. Things that are a bit more integrated (maybe vector tile plugin) I suspect may need a bit more work, but nothing crazy.

ibrierley avatar Aug 14 '22 20:08 ibrierley

Alright I am happy with where this PR is and am happy to merge. I will work on migrating plugins over the next day or two. Still need to write some docs and check for any last breaking changes before publishing. I have gotten 3 people to test in their apps and I have it working on my app, it should be quite stable at this point and closes 4 distinct issues.

mootw avatar Aug 17 '22 04:08 mootw

Ok, in that case, I'm happy to merge. Thanks for all your hard work!

This is my plan of action:

  1. Release as v3.0.0-beta.2
  2. Give notice on Discord
  3. a. Wait for feedback and conduct more testing if necessary b. Try to merge the other PRs that are waiting
  4. Write CHANGELOG and publish new docs
  5. Release as v3

JaffaKetchup avatar Aug 17 '22 09:08 JaffaKetchup