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

[FEATURE] Is it possible to re-make LatLng as a first-party class? #1452

Closed
3 tasks done
znromonk opened this issue Feb 27, 2023 · 15 comments · May be fixed by #1750
Closed
3 tasks done

[FEATURE] Is it possible to re-make LatLng as a first-party class? #1452

znromonk opened this issue Feb 27, 2023 · 15 comments · May be fixed by #1750
Labels
feature This issue requests a new feature P: 4 (far future) S: core Scoped to the core flutter_map functionality

Comments

@znromonk
Copy link

What do you want implemented?

LatLng used by Flutter Map is currently from the latlong2 plugin. One major limitation of the plugin is the assertion of the latitude/longitude bounds. The original leaflet JS library has it's own LatLng class and does not have bounds on latitude/longitude values and allows for wrapping based on CRS.

Eliminating the restrictions on the latitude/longitude values, opens up possibility to solve the issues #1338 and #468 which relate to the polylines across the +/-180 longitude and repeating the maps across the +/-180 longitude.

It is a relatively light codebase to port and could be made in accordance with official leaflet source.

What other alternatives are available?

I have been able to modify the latlong2 plugin code and remove the restrictions mentioned above. I also made the necessary changes to the FlutterMap code to follow the official leaflet JS with respect to wrapping the tiles.

With this I can pan across the +/-180 boundaries and also show polylines across them without splitting them up.

Can you provide any other information?

Demo:

MapPanTest.mov

Platforms Affected

Android, iOS, Web, Windows, MacOS, Linux

Severity

Annoying: Currently have to use workarounds

Requirements

@znromonk znromonk added the feature This issue requests a new feature label Feb 27, 2023
@ibrierley
Copy link
Collaborator

I personally don't have an issue with this, if anyone wants a bash.

@TesteurManiak
Copy link
Collaborator

TesteurManiak commented Feb 28, 2023

Don't have any issue either but this will be quite the breaking change I think 🤔

@ibrierley
Copy link
Collaborator

Indeed, a lot of file changes, and people may have their own other chunks of code elsewhere that rely on it, so not a trivial one in terms of changes (logic probably generally unchanged on the whole I suspect).

I'm not sure if there's a middle way to allow either, but feels like it may be a bit clunky to do that, not sure!

@TesteurManiak
Copy link
Collaborator

TesteurManiak commented Feb 28, 2023

Yeah and it will also break all the plugins depending on flutter_map. If we want to do it iteratively we'll have to create methods that would use our new latlong object but keep and deprecate all the public methods relying on the LatLng object from latlong2.

For example, we would keep for a while the MapController.center getter but deprecate it to the profit of a new MapController.centerCoordinates.

@ibrierley
Copy link
Collaborator

Another approach could be to ask latlng2 folk to allow an option where a > +/-90/180 is allowed. Just thinking out loud.

@TesteurManiak
Copy link
Collaborator

That would also be a solution but, seeing that the package hasn't been updated for the past 17 months even with a few issues and PR open I don't know if such option would be added anytime soon 🤔

@znromonk
Copy link
Author

The lack of update on the latlong2 package was also the reason the suggestion was made to make it first-party. It was initially forked from the original package to make it null-safe and not much update since then.

I do realize this would be significant breaking change and could possibly be relegated to a major version change.

One thing to note about the demo: The official leaflet JS does not support polylines spanning the meridian boundary. There are two methods to overcome the limitation.

  • One is to wrap the longitude value when the user reaches the boundary with the worldCopyJump boolean. This does require splitting the polyline and only half of it visible at a time. Not ideal.
  • The other is to go beyond the +/- 180 limit in LatLng. This would mean adding/subtracting 360 when crossing.
Polyline(
  points: [
    LatLng(37, -100),
    LatLng(57, 100),
  ],
),

becomes

Polyline(
  points: [
    LatLng(37, -100),
    LatLng(57, 100 - 360),
  ],
),

Using the second method, the map does not blink as it would with worldCopyJump when panning across the boundary. The polylines/markers are only present at those coordinates. All other world 'copies' will not show those polylines/markers.

PS:
Another question I had was whether flutter_map is intended to keep leaflet JS feature/functionality parity? A lot of the code is ported to dart but there are still few that are not. If LatLng is going to be ported, then the rest of it could be as well (in time of couse).

@ibrierley
Copy link
Collaborator

I think the parity with leaflet was broken some time ago (if it was ever fully there), but I personally do try and always match leaflet.js if its something that exists there already if that makes sense, and try and hedge anyone that way (also to check if something has been done by leaflet.js as inspiration. It's difficult to keep an eye on another repo though.

If there are features in leaflet that aren't in flutter_map and anyone wants a bash at adding those features, I'm sure it would go down well.

I think the key with major changes, is not to do them too often, and have a good reason, rather than never to do them.

@znromonk
Copy link
Author

Understood and it makes sense. I do not think this is an extremely high priority feature rather as something that can trickle down with intermediate fixes like @TesteurManiak had suggested. The developers could be given some deprecation warnings on upcoming changes when they test/run their existing code.

@ibrierley
Copy link
Collaborator

it may still be worth asking the author of LatLng2 first, see if he is interested in updating with the feature as a first step.

@JaffaKetchup
Copy link
Member

As pointed out on the Discord server (https://discord.com/channels/951867686378409984/1092883585943486544), the fromJson constructor doesn't have any limitations - bearing in mind that this is probably a bug/not intended. I wonder if there's some kind of workaround there 🤔?

@JosefWN
Copy link
Contributor

JosefWN commented Jun 4, 2023

I agree with @ibrierley, first step should be trying to find a fix upstream. Submitting a PR to that repo would be much less work for everyone than implementing our own LatLng.

The lack of update on the latlong2 package was also the reason the suggestion was made to make it first-party.

That alone is not an indication of whether the package is maintained or not, it could also be because the package is fairly mature. There doesn't seem to be a large backlog on issues or PR:s. They have 3 open issues, all of which seem like quite minor improvements.

With the release of Dart 3 they did update the package, v0.9.0 was released two weeks ago.

@r0b3rth4ns3n
Copy link

i've opened an issue with latlong2
jifalops/dart-latlong#11

@monsieurtanuki
Copy link
Contributor

From v0.9.1, there are no longer assertions in latlong2 regarding lat/long validity, cf. https://pub.dev/packages/latlong2/changelog and jifalops/dart-latlong#13
Perhaps this issue is not relevant anymore, then.

@JaffaKetchup
Copy link
Member

For the time being, I'm going to close this issue. I think the time is better spent working with what we've got here to introduce cross-boundary lines and such like.

@JaffaKetchup JaffaKetchup closed this as not planned Won't fix, can't repro, duplicate, stale Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue requests a new feature P: 4 (far future) S: core Scoped to the core flutter_map functionality
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants