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: unbounded horizontal scroll #1948

Merged
merged 8 commits into from
Sep 10, 2024

Conversation

monsieurtanuki
Copy link
Contributor

@monsieurtanuki monsieurtanuki commented Aug 18, 2024

Fixes #1582 (potentially eligible for bounty).

In a first approach I reused part of @ReinisSprogis' code regarding the camera using a -180,180 longitude.

The flash effect was due to tiles being somehow recreated when considered as from one world or another. The recreation is quick, but causes the flash effect.
The solution is to consider tiles in a unique -180,180 world AND to maintain a list of tile positions at the same time.

For instance, scrolling from Europe to Alaska:

  • Alaska is first considered as from the next world
  • scrolling again, Alaska is considered as part of the current world, as the center of the map is now in America
  • the flash fix is about reusing the same tile image for both positions (next world, current world)

I tested only in the Polygon example (that has no constraints), and manually dragging to the East (and tested to the West too).
Let me know if there are typical examples that do not work with that PR.

I've shot a video but it seems too big for github.

New file:

  • tile_renderer.dart: Display of a [TileImage] at given [TileCoordinates].

Impacted files:

  • camera.dart: reused @ReinisSprogis' code - first I implemented in MapControllerImpl.dragUpdated but that was less generic
  • tile.dart: additional positionCoordinates field
  • tile_coordinates.dart: new factory key which returns the same value for the same tile in all world replications
  • tile_image.dart: typo fix
  • tile_image_manager.dart: additional _positionCoordinates field
  • tile_image_view.dart: additional positionCoordinates field
  • tile_layer.dart: now using a TileRenderer instead of a TileImage
  • tile_range.dart: now using a zoom modulo when checking if a coordinate fits into a range

New file:
* `tile_renderer.dart`: Display of a [TileImage] at given [TileCoordinates].

Impacted files:
* `camera.dart`: reused @ReinisSprogis' code - first I implemented in `MapControllerImpl.dragUpdated` but that was less generic
* `tile.dart`: additional `positionCoordinates` field
* `tile_coordinates.dart`: new factory `key` which returns the same value for the same tile in all world replications
* `tile_image.dart`: typo fix
* `tile_image_manager.dart`: additional `_positionCoordinates` field
* `tile_image_view.dart`: additional `positionCoordinates` field
* `tile_layer.dart`: now using a `TileRenderer` instead of a `TileImage`
* `tile_range.dart`: now using a zoom modulo when checking if a coordinate fits into a range
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 @monsieurtanuki,
This looks like a great addition, thanks!

There are still a couple of issues, which I guess occur because it's just camera trickery, and not actually repeating the map.

The first is an issue with the gestures. When flinging, it gets stuck at a boundary:

Fling.Gesture.Issue.mp4

I guess the second technically comes under the optional part of the bounty (so no worries if this is not something you want to bother with, especially since it's unlikely to affect many end users, but maybe it would be fixed by a resolution to the above issue):

(optional, but appriciated (no additional bounty)) implement infinite repetition of Markers in the MarkerLayer

Marker.Teleporting.Issue.mp4

I'm not sure what the best approach here is. I guess the least breaking one is to allow animations to persist across the boundary (just a drag will work across the boundary by the looks of it)?

@JaffaKetchup JaffaKetchup changed the title feat: 1582 - smooth scroll beyond the end of the world feat: unbounded horizontal scroll Aug 19, 2024
@monsieurtanuki
Copy link
Contributor Author

Thank you @JaffaKetchup for your comments!

I'll have a look at the fling gesture bug.

Regarding the "teleporting" issue, it's somehow related to #1338.
Currently, we just ask the projection: "hey, which is the one pixel that matches this LatLng?". And that's it.
A solution would be to say: "by the way, is the map bigger than the world? In that case, replicate the markers in all matching places".
And more specifically for #1338: "which is the closest LatLng-projected pixel to the previous polyline pixel, modulo the world size?" (instead of "which is the unique LatLng-projected pixel")

@monsieurtanuki
Copy link
Contributor Author

@JaffaKetchup I've just fixed the fling gesture bug.

The problem was that:

  • the fling animator generates a list of increments
  • for each increment, a new LatLng
  • in the current standard projection, LatLng unprojection needs a pixel "in the world", or else it will truncate the longitude to -180 or 180
  • that's why I introduced the size of the world, to be added or subtracted to the pixel X, during the fling effect

Remarks:

  • Is there such thing as the size of the world in all projections?
  • I haven't touched the standard unprojection method - that means that there may be other cases that would truncate the longitude and block to the end of the world.

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.

Thanks! I haven't had time to test yet, but just a couple of nitpicks.

If you want to earn the bounty (if we accept this PR), please follow the Contributing instructions on Polar.

lib/src/layer/tile_layer/tile.dart Outdated Show resolved Hide resolved
lib/src/layer/tile_layer/tile_renderer.dart Outdated Show resolved Hide resolved
@monsieurtanuki
Copy link
Contributor Author

@JaffaKetchup I've just pushed the changes you asked for.

Regarding the "teleporting" bug, I can work on it but it'd be better in a separate PR. There are already enough important code changes in the current PR to review, and from what I saw in the code each type of layer (marker, polygon, ...) would have to be individually impacted by the world repetitions: enough work for a distinct PR.

Copy link
Collaborator

@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.

awesome work! 🎉

lib/src/layer/tile_layer/tile_image_manager.dart Outdated Show resolved Hide resolved
lib/src/layer/tile_layer/tile_image_view.dart Outdated Show resolved Hide resolved
lib/src/layer/tile_layer/tile_image_view.dart Outdated Show resolved Hide resolved
test/layer/tile_layer/tile_range_test.dart Show resolved Hide resolved
@mootw
Copy link
Collaborator

mootw commented Aug 28, 2024

For handling multiple "worlds" with the other layers, I wonder if we could implement a buffer, then layer renders to a single buffer and then that buffer gets copied to the other world instances? that would cause trouble with culling for sure, but just throwing out the idea since its related to this PR

@monsieurtanuki
Copy link
Contributor Author

For handling multiple "worlds" with the other layers, I wonder if we could implement a buffer, then layer renders to a single buffer and then that buffer gets copied to the other world instances? that would cause trouble with culling for sure, but just throwing out the idea since its related to this PR

I don't think we should put too much effort in optimization in those cases, at least for the moment.
I mean, "marker duplication in multiple world map" is still a very rare case, and we shouldn't jeopardize the 99% of the use cases just for that.
That said, once the users are accustomed to those new features (unlimited scrolls + marker duplication) AND if they complain about performances, then we could have a look at it.

@mootw
Copy link
Collaborator

mootw commented Sep 2, 2024

agreed, there are also some other things we would have to consider with that, having the non-tile layers just jump between the centered world is acceptable for now!

@monsieurtanuki
Copy link
Contributor Author

Thank you @mootw for your feedbacks!
I've just pushed the latest changes (renamed variables).

@monsieurtanuki
Copy link
Contributor Author

Don't know if something specific is blocking the current PR.

Copy link
Collaborator

@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 great to me! Thank you for the contributions! 🎉

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.

Huge thanks, I think is production ready!

There is still one minor bug I can reproduce where a camera constraint on the normal range still allows the map to be forced to the other side when pushed against the constraint, but this does seem to verify at least that it respects the constraints (which is also backed up by other testing).

Performance also seems to have taken a slight hit, but that can be looked into at another time, as it doesn't seem too bad. It seems to be the biggest/worst difference on the web.

(Haven't checked the code, as I believe @mootw has gone through that.)

In terms of the bounty award, please set up a Polar account and connect it to Stripe, and I'll be happy to make the payout whenever you're ready. I believe the full amount after Polar takes its cut will be $47.50.

@JaffaKetchup JaffaKetchup merged commit 182c4fc into fleaflet:master Sep 10, 2024
7 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you @JaffaKetchup @mootw for your reviews!
I can now focus on the "teleporting" markers.

My polar account is https://polar.sh/monsieurtanuki/

@JaffaKetchup
Copy link
Member

I've marked the issue as complete and paid the invoice, so hopefully you should recieve the bounty soon enough!

image

@monsieurtanuki
Copy link
Contributor Author

@JaffaKetchup Polar-wise, I still haven't received any money. As you sent it two week ago, I wonder if something is blocked somewhere.

@JaffaKetchup
Copy link
Member

Polar-wise, I still haven't received any money. As you sent it two week ago, I wonder if something is blocked somewhere.

It sounds like you haven't yet linked a Stripe account. Payouts go to a linked Stripe payouts account. As you've already made one for GitHub Sponsors I believe it should be much quicker (will remember your details I think).

@monsieurtanuki
Copy link
Contributor Author

For the record I've just edited details for Github Sponsors, but the polar set-up looks ok.

image

image

@JaffaKetchup
Copy link
Member

I've contacted the guys over at Polar, and it looks like it wasn't quite linked properly (it was linked to the organisation you setup, but not your personal account, which is where payouts are made, or something like that). He said he's linked it for you and made the payout, so hopefully should be with you at some point soon :)

@monsieurtanuki
Copy link
Contributor Author

Thank you very much @JaffaKetchup, I'm a bit richer now!

For the record, the $50 became $46.60 after fees ($3.40), and if I want to transfer it to my bank account there are additional fees of $2.48, leaving me with $44.12. And then the conversion to €, I guess.

Reminds me of restaurants in the US, where the prices are not the prices ("plus tax plus tip"). I even remember a campsite close to Monument Valley where they also mentioned "additional taxes". Too American for me: we decided to drive and eventually found an accommodation in a town called (you can't invent that) "Mexican Hat". But that's another story.

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

Successfully merging this pull request may close these issues.

[BOUNTY] [FEATURE] Implement unbounded horizontal scroll
3 participants