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

Tracking: Performance Issues #1165

Closed
6 tasks done
JaffaKetchup opened this issue Feb 18, 2022 · 14 comments · Fixed by #1333
Closed
6 tasks done

Tracking: Performance Issues #1165

JaffaKetchup opened this issue Feb 18, 2022 · 14 comments · Fixed by #1333
Assignees
Labels
bug This issue reports broken functionality or another error

Comments

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Feb 18, 2022

This issue aims to help track the multiple performance issues, so it's easier to handle them by linking them together, fixing them, or closing them.

There are likely to be more added in the future.


Fixed/ignored (closed) issues:

@JaffaKetchup JaffaKetchup added bug This issue reports broken functionality or another error high priority labels Feb 18, 2022
@JaffaKetchup JaffaKetchup pinned this issue Feb 18, 2022
@grahamsmith
Copy link

Hello 👋 I am new to the repo so apologies if this is not the right place. Thanks for all the hard work thus far 👍

I have been reading through the code and having a play and noticed there are some performance improvements that could be made. I then noticed this issue.

Unsure if you are accepting PRs but would be happy to help. For example I have noticed we work with num a lot when in practice we parse from String to Double and then treat is as a num in the middle and then cast it via toDouble() at the end. Whilst individually probably not that expensive it probably adds up over time.

The WMS implementation looks like it has caused some ripple effects through the code too, for example when dealing with urlTemplate and then having to force unwrap it etc. I've seen notes in the code so I'm late to this party.

I think there's some general widget improvements (removing un-needed ones) and some newer language features to help out too.

Happy to help but didn't want to waste my time either if its not needed.

@ibrierley
Copy link
Collaborator

Help is always wanted :), and all suggestions and PRs welcome ! So feel free and great to have others interested.

Whilst all optimisations welcome, I would also say as a general programming thing, I wouldn't get too hung up on microoptimisations and let it eat up your previous dev time, there are normally bigger fish to fry with performance.

@mohammedX6
Copy link

I was experiencing very bad performance when I am showing some very basic lines (thickness only to 1), Then after researching I made this modification in the drawing method of the PolyLine layer

Instead of using path.lineTo
Use this :

//Assuming we have the offsets 
final path = ui.Path();
path.addPolygon(polyline.offsets, false); // Here is the magic
canvas.drawPath(path, paint);

polylines.add(CustomPaint(painter: PolylinePainter(polyline, false, false, path: path),size: size,)); 

@ibrierley
Copy link
Collaborator

I think that makes sense, and if we were to rewrite it (or write a new plugin for polylines/gons then I'd be tempted to do some caching of the offsets (and likely paths) similar to the recent marker changes.

I'm a little surprised that its so different in performance though for basic lines, or are there a lot of them ?

@mohammedX6
Copy link

i will try to make it as plugin

@ibrierley
Copy link
Collaborator

That's great, just shout if you get stuck.

@aitinsan
Copy link

i will try to make it as plugin

hi, what about that?

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@tomgilder
Copy link

Hello! Thank you for the amazing package.

Just wanted to mention that performance with Safari on macOS is pretty terrible, I think due to this issue: flutter/flutter#96869

Scrolling around on a map can easily cause Safari to use several gigabytes of memory, not fun ☹️

@ibrierley
Copy link
Collaborator

How are you trying to use it with a large image ?

@ibrierley
Copy link
Collaborator

Also a minimal example with an image would be useful.

@tomgilder
Copy link

I'm not trying to use it with an image; it happens with any usage of flutter_map in Safari, just from it loading map tiles.

It's a Flutter issue, unless flutter_map is somehow not releasing offscreen images.

@ibrierley
Copy link
Collaborator

Oh ok, I don't get any issues on my Safari (hovers around 62M, but I may be using an older version that I'm testing, I'll try and rebuild when I get chance). Have you tried building to html rather than canvas to see if that helps ?

@JaffaKetchup JaffaKetchup linked a pull request Aug 16, 2022 that will close this issue
15 tasks
@JaffaKetchup
Copy link
Member Author

JaffaKetchup commented Aug 16, 2022

Many common performance issues have been fixed, and are now a result of a mixture between misconfiguration and unclear documentation and implementation.

For example, one major factor seems to be the persistent usage of layers amongst the older members of the community, which is no longer recommended, and should be replaced by children. This is mostly the fault of the documentation, for still proffering layers.

This vagueness should be fixed by v3.0.0 (see #1333), which changes the internal and public implementations of map layers. This forces a more performance friendly implementation by removing places where mistakes can be made, as well as harnessing the full power of Flutter's widget system.
The documentation associated with this update should also help.

The situation will be re-reviewed after v3, if necessary.

PS. This is not to say there aren't any performance issues, such as some of the ones discussed in this thread, but many are due to the reasons explained above.

@JaffaKetchup JaffaKetchup unpinned this issue Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue reports broken functionality or another error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants