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

feat: allow polylines & polygons to cross world boundary #1969

Merged
merged 16 commits into from
Dec 3, 2024

Conversation

monsieurtanuki
Copy link
Contributor

@monsieurtanuki monsieurtanuki commented Sep 22, 2024

What

  • This PR deals with polylines and polygons (with or without holes) across longitudes +-180 (cf. [BUG] Crossing W/E Extremity With Polyline Results In Unexpected Lines #1338)
  • The main issue was that longitudes -179 and 179 were projected in opposite sides of the world, although they are geographically nearby.
  • The solution is to build incrementally the list of projected points: if two consecutive projected points are obviously too far away (more than half a world away), we add/remove one world width to the latter point.
  • Doing so, we avoid jumping around -180 and +180, and we ensure the continuity among the points.
  • The PR doesn't deal with the "teleportation" effect. This will be done in another PR, once we're confident with that "add/remove a world width" strategy.

Screenshots

Polyline: before / after
Screenshot_1727023511
Screenshot_1727023556
Polygon with hole: before / after
Screenshot_1727023664
Screenshot_1727023641

Impacted files

  • crs.dart: new methods getHalfWorldWidth and projectList
  • painter.dart: refactored using pre-computed List<Double>
  • polygon.dart: added an example around longitude 180
  • polyline.dart: added an example around longitude 180
  • polyline_layer.dart: we don't cull polylines that go beyond longitude 180
  • projected_polygon.dart: using new method Projection.projectList
  • projected_polyline.dart: using new method Projection.projectList

monsieurtanuki and others added 3 commits September 22, 2024 18:51
Impacted files
* `crs.dart`: new methods `getHalfWorldWidth` and `projectList`
* `painter.dart`: refactored using pre-computed `List<Double>`
* `polygon.dart`: added an example around longitude 180
* `polyline.dart`: added an example around longitude 180
* `polyline_layer.dart`: we don't cull polylines that go beyond longitude 180
* `projected_polygon.dart`: using new method `Projection.projectList`
* `projected_polyline.dart`: using new method `Projection.projectList`
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.

Thanks for the pull request @monsieurtanuki! I did some quick testing and your added examples work like intended (thanks for adding them).

Some additional thoughts:
The main use case of line wrapping are paths that cross the -180/180 longitude for example when we want to display flights. We need to figure out a solution when the user follows a polyline or polygon accross the world border. Currently the line suddenly disapears. This, of course, is not in scope for this this pull request. (:

line.wrapping.mp4

lib/src/layer/polyline_layer/projected_polyline.dart Outdated Show resolved Hide resolved
@monsieurtanuki
Copy link
Contributor Author

Some additional thoughts: The main use case of line wrapping are paths that cross the -180/180 longitude for example when we want to display flights. We need to figure out a solution when the user follows a polyline or polygon accross the world border. Currently the line suddenly disapears. This, of course, is not in scope for this this pull request. (:

Oh it's a bug then! Probably related to the "teleportation" issue.
I'll have a look at it, and I may fix that in the current PR.
I keep you posted.

@josxha
Copy link
Contributor

josxha commented Sep 27, 2024

Oh it's a bug then! Probably related to the "teleportation" issue.
I'll have a look at it, and I may fix that in the current PR.

I actually don't think it's an bug. The line just moves to the new world that comes into center. So the line is still active but basically on the other side of the world. If you plan to look into this as well how it can get improved that's awesome of couse. (:

@monsieurtanuki
Copy link
Contributor Author

@josxha Your description is correct: it's the "teleportation" issue (I can see a unique polyline jumping from world to world on the same zoom 0 map) but when the zoom is high - the "other" world is not visible.

I still think it's a bug, probably not a new bug, but a fair user expectation is to see at least one polyline.

Working on it. The current PR is relatively small (7 files changed), so I'll see how review-able it would become when including that bug fix.

Impacted files:
* `offsets.dart`: new method `getAddedWorldWidth`, used to add/subtract a world width in order to display visible polylines
* `painter.dart`: minor fix, as now we may unproject coordinates from the wrong world
@monsieurtanuki
Copy link
Contributor Author

@josxha Fixed! One visible copy of the polyline, with a small or a large zoom.

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.

LGTM in general! Some documentation about what the whole 'half world width' stuff is about would be nice. I'll leave to @josxha to give final review :) Thanks!

@JaffaKetchup JaffaKetchup changed the title fix: 1338 - longitude +-180 with correct polylines and polygons feat: allow polylines & polygons to cross world boundary Sep 30, 2024
@JaffaKetchup JaffaKetchup linked an issue Sep 30, 2024 that may be closed by this pull request
5 tasks
@JaffaKetchup
Copy link
Member

Just a thought, what happens with CircleMarkers?

@monsieurtanuki
Copy link
Contributor Author

Just a thought, what happens with CircleMarkers?

Nothing.
This PR is specific to polylines (make them connect beyond the limits of the world, and make them visible if possible).
Unfortunately, all the "many worlds/teleportation" code improvements have to be done on each "geo widget" (marker, polygon, polyline, circlemarker, ...).
We may refactor elegantly at the end, but I wouldn't count on that. And we're not at the end anyway.

Impacted files:
* `crs.dart`: replaced "half world width" with "world width", in order to avoid answering to the question "why HALF?"
* `offsets.dart`: now we display the occurrence closer to the screen center; minor refactoring
* `painter.dart`: minor fix regarding side-effects on `_metersToStrokeWidth`
* `polyline_layer.dart`: now computes the limits projected from -180 and 180 instead of "half world width"
* `projected_polyline.dart`: moved code to `polyline_layer.dart`
@monsieurtanuki
Copy link
Contributor Author

@JaffaKetchup @josxha I've just pushed some changes that make the code (somehow) more readable.
In particular, I've switched to getWorldWidth (no more "half") and I've added comments:

  /// Returns the width of the world in geometry coordinates.
  ///
  /// Is used at least in 2 cases:
  /// * my polyline crosses longitude 180, and I somehow need to "add a world"
  /// to the coordinates in order to display a continuous polyline
  /// * when my map scrolls around longitude 180 and I have a marker in this
  /// area, the marker may be projected a world away, depending on the map being
  /// centered either in the 179 or the -179 part - again, we can "add a world"
  double getWorldWidth() {
    final (x0, _) = projectXY(const LatLng(0, 0));
    final (x180, _) = projectXY(const LatLng(0, 180));
    return 2 * (x0 > x180 ? x0 - x180 : x180 - x0);
  }

@JaffaKetchup JaffaKetchup requested review from mootw, TesteurManiak and josxha and removed request for mootw, TesteurManiak and josxha October 3, 2024 16:08
@JaffaKetchup

This comment was marked as resolved.

@monsieurtanuki

This comment was marked as resolved.

@JaffaKetchup

This comment was marked as resolved.

@monsieurtanuki

This comment was marked as resolved.

@JaffaKetchup

This comment was marked as resolved.

@monsieurtanuki
Copy link
Contributor Author

The review should probably be put on hold until the fix for #1976 is coded (by me) (like, today), reviewed and merged.

Now back to polygons and polylines in multi-worlds!

@monsieurtanuki
Copy link
Contributor Author

Ping

@JaffaKetchup
Copy link
Member

Oops, apologies, thought you needed more time to work on this. I'll review now.

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.

LGTM, and some nice removal of duplications as well :) One question, then ready to merge.


This also appears on master, but is caused by the infinite scroll feature. Not sure if its a bug exactly? Using a cameraConstraint with the entire world will cause the map to stick against the edges during a fling animation, but will jump once the fling is complete. A drag does not work at all (causes sticking).

cameraConstraint: CameraConstraint.contain(
	bounds: LatLngBounds(
		const LatLng(-90, -180),
        const LatLng(90, 180),
    ),
),
Recording.2024-12-02.214029.mp4

example/pubspec.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@mootw mootw left a comment

Choose a reason for hiding this comment

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

looks good pending the one dependency comment!

@JaffaKetchup JaffaKetchup merged commit b81d6db into fleaflet:master Dec 3, 2024
7 checks passed
@monsieurtanuki
Copy link
Contributor Author

@JaffaKetchup I'm not an expert on cameraConstraint and fling effect, but I think the possible bug you're mentioning goes beyond this PR and would deserve its own issue.
@mootw Removed the useless new explicit package dependency.

@monsieurtanuki
Copy link
Contributor Author

Thank you @JaffaKetchup @mootw @josxha for your reviews!

@JaffaKetchup
Copy link
Member

Hey @monsieurtanuki, just plotting our next release schedule. Do you think you'll be able to work on the teleporting issue any time soon? Of course, no problem at all if not! Thanks for all your hard work so far :)

@monsieurtanuki
Copy link
Contributor Author

@JaffaKetchup I guess I should be able to fix the teleporting effect within a month - including reviews and feedbacks.

Keep in mind though that:

  • the teleporting is to be coded specifically for each widget (marker, circlemarker, polygon and polyline) - several potential sources of bugs
  • [BUG] unbounded horizontal scroll breaks most tile grids #1976 (reported by @arneke) about the multiworld feature having non webmercator maps not working anymore
  • the teleporting goes a bit beyond the "multiworld" and "pacific polygons" issues, adding a "low zoom" flavor

Imagining an arbitrary "let's publish a release before 2025" goal, I believe it would be safer not to jeopardize the next release, possibly to test again, and to keep the teleporting for afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG] Crossing W/E Extremity With Polyline Results In Unexpected Lines
4 participants