Skip to content

perf!: add simplification and segment culling to PolylineLayer & simplification to PolygonLayer#1704

Merged
JaffaKetchup merged 35 commits intofleaflet:masterfrom
mootw:master
Jan 13, 2024
Merged

perf!: add simplification and segment culling to PolylineLayer & simplification to PolygonLayer#1704
JaffaKetchup merged 35 commits intofleaflet:masterfrom
mootw:master

Conversation

@mootw
Copy link
Copy Markdown
Contributor

@mootw mootw commented Oct 25, 2023

adds an optional (enabled by default with conservative values) simplification step to polygon and Polyline layers.

the default value is 1 decimal degree at zoom level 0, which is scaled based on the floored zoom level. this is similar to leaflets default simplification of 1 logical pixel. i would rather this value be based on logical pixels, but i did not know an easy way to adjust the pixel scale to be static based on the zoom level. leaflet only calculates this value after the projection and uses pixels, but as far as im aware they avoid the issue of in-between zoom levels by not calculating offsets between them and enforcing whole zoom levels. I attempted to calculate the pixel scale using the camera context but was not successful.

using logical pixels lead to the lines jittering too much, so only having them simplified in incremental steps is less obvious.

@JaffaKetchup
Copy link
Copy Markdown
Member

As discussed, we should probably also think about fixing the culling code to work within elements, particularly for polylines.
For example, a long polyline currently needs to be manually broken into separate Polylines to benefit from culling. It may be a good idea to leave one 'path' (ie. a segment of a polyline) either side of the camera's viewport outside of bounds, so that a false end can never be seen.
Not sure how it could work with polygons? Is drawing a straight line when out of bounds good enough?

@ignatz
Copy link
Copy Markdown
Contributor

ignatz commented Oct 27, 2023

Skimming the PR, IIUC you're simply merging neighboring points if they'd end up round about on the same pixel?

Right now you're re-simplifying on every draw. Since the merging is zoom-level dependent could we do some caching, so that there's less work to be done when panning/rotating?

FWIW, I'm personally already doing quite a few zoom-level-dependent gymnastics, e.g. fading certain polygons in and out. I'd be interested in having the tooling standalone as opposed to just integrated into the poly layers. In that case I can have a single merging pass with Polygon' = f(Polygon). Is that something you've considered?

@ignatz
Copy link
Copy Markdown
Contributor

ignatz commented Oct 27, 2023

One more drive-by comment, I realized that your merging logic computes the distance between two map coordinates as cartesian. While that's a reasonable approximation at the equator, it's not so much towards the poles. You might run into some funny business. At least whatever threshold you set, will apply very differently.

@JaffaKetchup JaffaKetchup marked this pull request as draft October 30, 2023 16:00
@mootw
Copy link
Copy Markdown
Contributor Author

mootw commented Nov 7, 2023

i really want the simplification and culling margin to be based on the render pixel rather than the scaled latlng, but there are a couple trade-offs on both sides. definitely a TODO item though.

Comment thread lib/src/layer/polyline_layer.dart Outdated
Comment thread lib/src/layer/polyline_layer.dart Outdated
Comment thread lib/src/misc/simplify.dart
Comment thread lib/src/layer/polyline_layer.dart Outdated
Comment thread lib/src/layer/polygon_layer/polygon_layer.dart Outdated
Comment thread lib/src/layer/polyline_layer.dart Outdated
@JaffaKetchup JaffaKetchup changed the title implementation of polyline and polygon simplification perf: Add simplification and improve culling for Polylines and Polygons Nov 10, 2023
@JaffaKetchup JaffaKetchup changed the title perf: Add simplification and improve culling for Polylines and Polygons perf: add simplification and improve culling for Polylines and Polygons Nov 10, 2023
@mootw mootw marked this pull request as ready for review November 12, 2023 23:42
Copy link
Copy Markdown
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.

Just these few things left (2 carried over from my last review).

Comment thread lib/simplify.dart
Comment thread lib/src/layer/polygon_layer/polygon_layer.dart Outdated
Comment thread lib/src/layer/polyline_layer.dart Outdated
@JaffaKetchup JaffaKetchup requested a review from josxha November 13, 2023 17:54
@mootw
Copy link
Copy Markdown
Contributor Author

mootw commented Nov 21, 2023

still want to look into using pixels instead of zoom levels + degrees; unless we want to merge fairly quickly which I'm totally ok with how it is. Technically there is not a ton of difference between the two values, because map size is based on virtual pixels so in the end the simplification is more or less based on pixels anyways (minus any weird virtual pixel scaling). It's just easier to explain: simplify with a tolerance of 1.5 virtual pixels versus 1.5 decimal degrees at zoom level zero / (2^zoom) which when you multiply it back out gets you about 1.456623 virtual pixels or whatever.

JaffaKetchup
JaffaKetchup previously approved these changes Nov 21, 2023
Copy link
Copy Markdown
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, thanks! I've made a couple of changes to the example app, let me know if you like them or not, and I've hopefully fixed the Windows app build.
Happy to leave this to the end of the week if you wanted to get the pixel-based tolerance stuff in, but do want to merge relatively soon so we can properly re-assess performance, etc.

@fleaflet fleaflet deleted a comment from codecov Bot Dec 10, 2023
mootw and others added 4 commits January 4, 2024 18:38
Removed high quality simplification option (and always use it)
Added `Polyline.copyWithNewPoints` method
Added temporary performance monitors
Minor syntactic improvements
Formatted
Improved documentation
Improved Polylines page in example app
Comment thread lib/src/layer/polyline_layer.dart Outdated
…ading to multiple issues when hit testing was used in conjunction with simplification

PARTIALLY fixed issue where simplified and culled `Polyline`s would be notified from hit testing instead of original, leading to multiple issues with the recommended documented method of interactivity handling
Improved `_PolylineLayerState.didUpdateWIdget` logic efficiency
Improved documentation
Removed debugging/performance code
Improved Polyline interaction example
…nge elsewhere

Removed polyline simplification precomputer
Reorganised polyline-related source files
Fixed bugs and improved efficiency
Copy link
Copy Markdown
Contributor

@TesteurManiak TesteurManiak left a comment

Choose a reason for hiding this comment

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

LGTM! I don't know if you've been able to fix the performance issue, but I didn't seem to encounter any while testing the example. (I might not be testing in the correct conditions though)

@JaffaKetchup
Copy link
Copy Markdown
Member

If you can't spot the poor performance, then it's worked :D

Comment thread lib/src/misc/simplify.dart Outdated
Copy link
Copy Markdown
Contributor

@josxha josxha left a comment

Choose a reason for hiding this comment

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

lgtm, awesome pr! 👍

 to Polyline example
@JaffaKetchup JaffaKetchup changed the title perf!: add simplification and improve culling for Polylines and Polygons perf!: add simplification and segment culling to PolylineLayer & simplification to PolygonLayer Jan 13, 2024
@JaffaKetchup JaffaKetchup merged commit 24ceb32 into fleaflet:master Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants