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

v3.0.0: Simplify External & Internal Layer Handling #1333

Merged
merged 32 commits into from Aug 17, 2022
Merged

v3.0.0: Simplify External & Internal Layer Handling #1333

merged 32 commits into from Aug 17, 2022

Conversation

mootw
Copy link
Collaborator

@mootw mootw commented Aug 3, 2022

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, closes #1322

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

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?

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

As discussed on the Discord server. Also, we need to support older than Dart 2.17, so we (unfortunately) cannot use the super. constructor properties.

@ibrierley
Copy link
Collaborator

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....

@JaffaKetchup
Copy link
Member

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

@ibrierley
Copy link
Collaborator

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).

@mootw
Copy link
Collaborator Author

mootw commented Aug 3, 2022

@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
Copy link
Collaborator Author

mootw commented Aug 7, 2022

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 mootw requested a review from JaffaKetchup August 7, 2022 05:17
@TheLastGimbus
Copy link
Contributor

@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 😖 . 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"

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

Ok, I think I'm happy with this. Because it is such a big update, I will not merge it until @ibrierley also approves after he's tested.

We also need to update the documentation for this. I'm happy to do it, or you can write something and I'll upload it.

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

Retracted approval, as discussed in Discord server.

@JaffaKetchup JaffaKetchup changed the title [Breaking] Remove LayerOptions and Plugins v3.0.0: Simplify External & Internal Layer Handling Aug 12, 2022
Copy link
Collaborator

@ibrierley ibrierley left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this MooNag, all the examples test fine for me so far. I'll try and have a rummage through the code over the next day or so, but all looking good so far for me.

@JaffaKetchup
Copy link
Member

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

@ibrierley
Copy link
Collaborator

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.

@JaffaKetchup JaffaKetchup linked an issue Aug 16, 2022 that may be closed by this pull request
6 tasks
@JaffaKetchup JaffaKetchup mentioned this pull request Aug 16, 2022
6 tasks
@mootw
Copy link
Collaborator Author

mootw commented Aug 17, 2022

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.

@JaffaKetchup
Copy link
Member

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

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM for @ibrierley and I.

@JaffaKetchup JaffaKetchup merged commit 3eda247 into fleaflet:master Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants