Batch canvas draw operations and minimize redraws on map pans for the polygon and polyline layers to significantly improve performance. #1442
Conversation
…ion works correctly for all paint styles. It also avoid reordering the draw calls, i.e. polylines are drawn in the correct order given by the user.
…e displayed polylines don't change. Don't use the repaint boundary for web rendering.
…oonag's work. The hole drawing should work too.
…s on screen remains unchanged.
|
Reviewed this and tested the polygon layer in a a second app. I have made some improvements for performance and code quality. Ill try and commit them as a suggestion or a PR to your branch (not sure the exact right way to do that yet so i might cause some chaos. I might need to just do it as a review). There is a chance that this will have an issue with culling enabled and caching the layer render, but i was not able to get it to consistently occur without toggling off isComplex in the CustomPaint. Performance test in my real world app: Id be happy to submit an approving review after taking a look at my recommendations. |
There was a problem hiding this comment.
Looks good so far! Just a few things.
Is there a specific reason we are using zoom and rotation in the shouldRepaint functions vs just using map.hashCode? We dont have to change it, it just seems like it would be cleaner code with the same results and less susceptible to breaking in the future.
Let me know if you have any questions on the feedback!
I'm not sure I understand the suggestion of using map.hashCode correctly. shouldRepaint tells the repaint boundary on when to invalidate the cash, i.e. when two Painter objects should be considered visually different. If by map.hashCode you mean the FlutterMapState's dart-Object hashCode (which doesn't seem to be overridden), i'm not sure how that would achieve this. |
|
Yeah you're correct here. We would want to override hashCode in FlutterMapState. Im not sure what I was thinking since by default its only an identity hash. That might have side-effects that are hard to identify. I think its fine to keep this way for now. It should work fine and we can always change it later if we override hashCode in FlutterMapState later. |
|
Just going to have 1 more person check since its a fairly large rendering change and we should get it merged in soon. Please send me a ping if we don't get it merged in by next Friday (Feb 17th)! |
That makes sense. I appreciate all the eyes we can get. Given the testing situation, I'm certainly not convinced that we're not introducing any new edge cases. I'd not be surprised if something bubbles up and we'll have to iterate. The less we need to, the better. Thanks |
Fixed linter warnings
There was a problem hiding this comment.
As far as I can see, everything is OK. As you said, there might be issues further down the road, but I think everything is pretty good.
In terms of the actual performance gain on my device, the UI thread is better, but the raster thread is the same, if not a little worse (@moonag).
(master vs fast_poly in that order):
Screenrecorder-2023-02-16-16-01-50-641.mp4
Thank you for taking the time to actually validate. Unfortunately for you, your observation is very much expected. The performance should be the same. The two optimizations kick in when you have:
In your case you're having few polylines with different properties + you're rotating/zooming. If you're still up for further validation, consider drawing some thousands polylines with the same color. Sorry, I should have been clearer.. |
|
Ah no worries. As far as I'm concerened, two people saying performance has improved is enough, and this shows that normal performance has remained the ~same. |
|
Just checked the video, the performance regression seems insignificant enough. We will merge and watch for any issues! |
|
Thanks @ignatz :) |
The batching is based on @moonag 's proposal.
Based on a makeshift benchmark and my phone, I'm seeing:
On the raster thread:
On the UI thread:
Before:
After:
