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

fix: prevent crash when zooming far into Polygons #1854

Merged
merged 5 commits into from
Mar 29, 2024
Merged

fix: prevent crash when zooming far into Polygons #1854

merged 5 commits into from
Mar 29, 2024

Conversation

ReinisSprogis
Copy link
Contributor

fix #1829
Problem was that as zooming in, path was getting very long and many dots was calculated eventually getting into millions and crashing. This resolves it by drawing points only on visible segment. Directly painting circles rather them on path. Thought this might be better. Any suggestions for improvement are welcome. Would appreciate some testing as I haven't done much.

@JaffaKetchup JaffaKetchup changed the title fix #1829 High zoom results in increasing frame times when using Polygons fix: prevent crash when zooming far into Polygons Mar 17, 2024
JaffaKetchup
JaffaKetchup approved these changes Mar 29, 2024
View reviewed changes
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.

Hey @ReinisSprogis,
I think this works great for polygons, thanks. LGTM!

I've also realised that the same issue occurs with polylines, we'll need to apply the same fix there.

@JaffaKetchup JaffaKetchup merged commit 149f847 into fleaflet:master Mar 29, 2024
7 checks passed
@ReinisSprogis
Copy link
Contributor Author

Hi. @JaffaKetchup Yes! I suppose would be better if @monsieurtanuki could look into it as he would navigate better in code he just wrote.

@monsieurtanuki
Copy link
Contributor

@ReinisSprogis @JaffaKetchup Sounds reasonable. I have to refactor part of the code into a class.

@ReinisSprogis ReinisSprogis deleted the polygon_fix branch April 3, 2024 19:21
@monsieurtanuki
Copy link
Contributor

Problem was that as zooming in, path was getting very long and many dots was calculated eventually getting into millions and crashing. This resolves it by drawing points only on visible segment.

Makes sense. Probably relevant specifically for dotted lines, where the dots have little space between them, but should be true for dashed lines too.

Directly painting circles rather them on path. Thought this might be better.

To be tested indeed about displaying dots: which is better/faster between

  • path.addOval
  • path.moveTo / path.lineTo on the same spot
  • canvas.drawCircle

Same performance question for lines (path.lineTo vs. canvas.drawLine). Would deserve a specific issue.

@ReinisSprogis
Copy link
Contributor Author

Problem was that as zooming in, path was getting very long and many dots was calculated eventually getting into millions and crashing. This resolves it by drawing points only on visible segment.

Makes sense. Probably relevant specifically for dotted lines, where the dots have little space between them, but should be true for dashed lines too.

Directly painting circles rather them on path. Thought this might be better.

To be tested indeed about displaying dots: which is better/faster between

  • path.addOval
  • path.moveTo / path.lineTo on the same spot
  • canvas.drawCircle

Same performance question for lines (path.lineTo vs. canvas.drawLine). Would deserve a specific issue.

Testing in https://demo.fleaflet.dev/polyline Same lag occurs while zooming in into dashed line and dotted line. Need to draw segment only up to canvas edge.

@monsieurtanuki
Copy link
Contributor

@ReinisSprogis @JaffaKetchup I've just found an elegant way to display only what matters.
Not sure if it will be easy to implement all PatternFits though - I may drop one of them (extendFinalDash).

Btw, with this new mehod:

  • it will be easier to check for performances between canvas.drawLine and path.lineTo
  • but as we don't display that much segments and dots we don't need to run perf tests about it in a first approach

monsieurtanuki added a commit to monsieurtanuki/flutter_map that referenced this pull request Apr 7, 2024
@monsieurtanuki
feat!: support of solid, dotted, dashed styles for polygons, with opt…
df60d0d 
…imized rendering

New files:
* `pixel_hiker.dart`: Pixel hikers that list the visible items on the way. Code used to be in `polyline_layer/painter.dart`, but was heavily refactored with fleaflet#1854 in mind
* `visible_segment.dart`: Cohen-Sutherland algorithm to clip segments as visible into a canvas. Code used to be in `polygon_layer/painter.dart`, and was lightly refactored.

Impacted files:
* `polygon_layer/painter.dart`: now using new file `pixel_hiker.dart` for optimized rendering; moved "clip code" to new file `visible_segment.dart`; minor refactoring about parameter order consistency
* `polyline_layer/painter.dart`: now using new file `pixel_hiker.dart` for optimized rendering; moved "pixel hiker" to new file `pixel_hiker.dart`
* `pages/polygon.dart`: replaced `bool isDotted` with `PolylinePattern pattern` and in one case replaced it with "dashed"
* `polygon_layer/polygon.dart`: BREAKING - replaced `bool isDotted` with `PolylinePattern pattern`
* `polygon_layer/polygon_layer.dart`: minor refactoring
* `polyline_layer/polyline_layer.dart`: minor refactoring
@monsieurtanuki monsieurtanuki mentioned this pull request Apr 7, 2024
Merged
JaffaKetchup added a commit that referenced this pull request Apr 10, 2024
@monsieurtanuki @JaffaKetchup @josxha
feat!: support of solid, dotted, dashed styles for polygons, with opt…
930e8b6 
…imized rendering (#1865)

* feat!: support of solid, dotted, dashed styles for polygons, with optimized rendering

New files:
* `pixel_hiker.dart`: Pixel hikers that list the visible items on the way. Code used to be in `polyline_layer/painter.dart`, but was heavily refactored with #1854 in mind
* `visible_segment.dart`: Cohen-Sutherland algorithm to clip segments as visible into a canvas. Code used to be in `polygon_layer/painter.dart`, and was lightly refactored.

Impacted files:
* `polygon_layer/painter.dart`: now using new file `pixel_hiker.dart` for optimized rendering; moved "clip code" to new file `visible_segment.dart`; minor refactoring about parameter order consistency
* `polyline_layer/painter.dart`: now using new file `pixel_hiker.dart` for optimized rendering; moved "pixel hiker" to new file `pixel_hiker.dart`
* `pages/polygon.dart`: replaced `bool isDotted` with `PolylinePattern pattern` and in one case replaced it with "dashed"
* `polygon_layer/polygon.dart`: BREAKING - replaced `bool isDotted` with `PolylinePattern pattern`
* `polygon_layer/polygon_layer.dart`: minor refactoring
* `polyline_layer/polyline_layer.dart`: minor refactoring

* Renamed `PolylinePattern` to `StrokePattern`
Re-organised file structure

* Review changes

Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>

* Update lib/src/layer/general/line_patterns/stroke_pattern.dart

Co-authored-by: Joscha <34318751+josxha@users.noreply.github.com>

* Update lib/src/layer/general/line_patterns/pixel_hiker.dart

Co-authored-by: Joscha <34318751+josxha@users.noreply.github.com>

* Update lib/src/layer/general/line_patterns/stroke_pattern.dart

Co-authored-by: Joscha <34318751+josxha@users.noreply.github.com>

* Minor file re-organisation

* Fixed bug

---------

Co-authored-by: JaffaKetchup <github@jaffaketchup.dev>
Co-authored-by: Joscha <34318751+josxha@users.noreply.github.com>
@josxha josxha added this to the v7.0 milestone Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaffaKetchup JaffaKetchup JaffaKetchup approved these changes

@ibrierley ibrierley Awaiting requested review from ibrierley

@mootw mootw Awaiting requested review from mootw

@TesteurManiak TesteurManiak Awaiting requested review from TesteurManiak

@josxha josxha Awaiting requested review from josxha

Assignees
No one assigned
Labels
None yet
Projects
Development
Archived in project
Milestone
v7.0
Development

Successfully merging this pull request may close these issues.

[BUG] High zoom results in increasing frame times when using dotted features
4 participants
@ReinisSprogis @monsieurtanuki @JaffaKetchup @josxha