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.

Sign up for GitHub

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

Jump to bottom

perf!: add simplification caching to PolygonLayer & other performance improvements #1795

Merged
merged 7 commits into from
Jan 15, 2024
Merged

perf!: add simplification caching to PolygonLayer & other performance improvements #1795

merged 7 commits into from
Jan 15, 2024

Conversation

JaffaKetchup
Copy link
Member

@JaffaKetchup JaffaKetchup commented Jan 13, 2024 •
edited

Reflected the same changes made to PolylineLayer. Also includes some minor performance enhancements and consistency changes to both.

Updated example project and improved consistency and styling. Added PerformanceOverlay widget to performance stress testing pages.

Also includes some other minor performance improvements (eg. wrapped RepaintBoundary around widget tree), and deprecates Polygon.isFilled.

@JaffaKetchup
Added simplification caching to PolygonLayer (consistent with `Poly…
24a0f05 
…lineLayer`)

Moved `isClockwise` function into `Polygon` as a static method
Added `Polygon` equality and hashing methods
Added `Polygon.copyWithNewPoints` method
Minor performance improvements to `Polyline` equality and hashing methods
Minor syntactic changes to `PolylineLayer.build` method
Removed unnecessary `CustomPainter` methods/parts from `PolylineLayer` & `PolygonLayer`
Reorganized internal file structure of Polygon Layer
Improved Polygon example with 138k points from GeoJson file
Seperated `SimplificationToleranceSlider` in example into independent file
@josxha
Copy link
Contributor

josxha commented Jan 13, 2024

Nice documentation in the commit notes, thanks!

  1. I assume .geojson.noformat is indicate that the file should not get treatend as a browsable file by tools or the user?
    I'd suggest to gzip the geojson file. This shrinks the file size from 6.92MB down to 1.85MB, is build into dart:io and optimized for fast decoding.
    Caveats: While browsers use gzip to speed up the fetching of websites, dart:html does not support gzip. Maybe we can still find a way around this limitation? (If it's no quick fix, don't worry about it since it can be changed later.)
  2. The Polygon fill color currencly only works when simplification is disabled.
Without simplificationWith simplification
  1. The Polygon Layer screen showcased features like dotted borders, labels and so on before. While the polyline PR added simplification and culling to the existing page, this polygon PR removes the other polygons. Therefore could we showcase this feature inside a different screen?

@JaffaKetchup
Copy link
Member Author

I assume .geojson.noformat is indicate that the file should not get treatend as a browsable file by tools or the user?
I'd suggest to gzip the geojson file. This shrinks the file size from 6.92MB down to 1.85MB, is build into dart:io and optimized for fast decoding.
Caveats: While browsers use gzip to speed up the fetching of websites, dart:html does not support gzip. Maybe we can still find a way around this limitation? (If it's no quick fix, don't worry about it since it can be changed later.)

I did consider compressing it, but I think the effort required to keep it working on web and quickly could offset the benefit. The '.noformat' part is just a 'random' string to prevent auto-formatters from formatting the JSON and bloating the file size with empty whitespace.

The Polygon fill color currencly only works when simplification is disabled.

Good spot, will need to investigate that.

The Polygon Layer screen showcased features like dotted borders, labels and so on before. While the polyline PR added simplification and culling to the existing page, this polygon PR removes the other polygons. Therefore could we showcase this feature inside a different screen?

I wasn't considering what I was doing fully when I removed the original polygons :D. I'll bring them back to where they were.

@JaffaKetchup
Added RepaintBoundary widget around all other children
c7ff48f 
Improved performance stress testing example pages for polygons, polylines, many markers, and many circles
Fixed bug in `Polygon.copyWithNewPoints`
Deprecated & changed behaviour of `Polygon.isFilled`
Fixed unrelated linting/analysis issue
@JaffaKetchup JaffaKetchup changed the title perf: add simplification caching to PolygonLayer perf!: add simplification caching to PolygonLayer & other performance improvements Jan 14, 2024
@JaffaKetchup JaffaKetchup requested review from josxha and removed request for josxha January 14, 2024 13:09
@JaffaKetchup
Copy link
Member Author

@josxha

The Polygon fill color currencly only works when simplification is disabled.

Fixed, was an issue with the copyWithNewPoints method. Also fixed a related equality issue.

The Polygon Layer screen showcased features like dotted borders, labels and so on before. While the polyline PR added simplification and culling to the existing page, this polygon PR removes the other polygons. Therefore could we showcase this feature inside a different screen?

I've split out both the polyline and polygon stress tests into their own page, grouped them with the Many Markers/Circles pages, redesigned those last two for consistency, and added the PerformanceOverlay widget.

josxha
josxha requested changes Jan 15, 2024
View reviewed changes
Copy link
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.

Good pull request! I only found one small thing that could be changed.

example/lib/pages/many_circles.dart Outdated Show resolved Hide resolved
josxha
josxha approved these changes Jan 15, 2024
View reviewed changes
Copy link
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

@JaffaKetchup JaffaKetchup merged commit 202e320 into master Jan 15, 2024
8 checks passed
@JaffaKetchup JaffaKetchup deleted the polygon-simplification-caching branch January 15, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josxha josxha josxha approved these changes

@ibrierley ibrierley Awaiting requested review from ibrierley

@mootw mootw Awaiting requested review from mootw

@TesteurManiak TesteurManiak Awaiting requested review from TesteurManiak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@JaffaKetchup @josxha