-
-
Notifications
You must be signed in to change notification settings - Fork 867
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
Conversation
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
There was a problem hiding this 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)?
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. |
@JaffaKetchup I've just fixed the fling gesture bug. The problem was that:
Remarks:
|
There was a problem hiding this 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.
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work! 🎉
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 |
Co-authored-by: mootw <spencer.mein@gmail.com>
I don't think we should put too much effort in optimization in those cases, at least for the moment. |
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! |
Thank you @mootw for your feedbacks! |
Don't know if something specific is blocking the current PR. |
There was a problem hiding this 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! 🎉
There was a problem hiding this 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.
Thank you @JaffaKetchup @mootw for your reviews! My polar account is https://polar.sh/monsieurtanuki/ |
@JaffaKetchup 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). |
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 :) |
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. |
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:
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 inMapControllerImpl.dragUpdated
but that was less generictile.dart
: additionalpositionCoordinates
fieldtile_coordinates.dart
: new factorykey
which returns the same value for the same tile in all world replicationstile_image.dart
: typo fixtile_image_manager.dart
: additional_positionCoordinates
fieldtile_image_view.dart
: additionalpositionCoordinates
fieldtile_layer.dart
: now using aTileRenderer
instead of aTileImage
tile_range.dart
: now using a zoom modulo when checking if a coordinate fits into a range