-
-
Notifications
You must be signed in to change notification settings - Fork 871
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: replicate Marker
s across all worlds
#2000
Conversation
New file: * `multi_worlds.dart`: Example dedicated to replicated worlds and related objects (e.g. Markers). Impacted files: * `camera.dart`: added the `getWorldWidthAtZoom` method * `main.dart`: added references to new page `MultiWorldsPage` * `marker_layer.dart`: now tries to add/subtract world width to marker abscissa for replication * `menu_drawer.dart`: added references to new page `MultiWorldsPage`
Btw I've just detected a bug in the current version (even before this PR):
Basically, the Marker is projected to the "allegedly most relevant" world, i.e. the world that matches the map center. The side-effect (literally) is that sometimes it's not in a visible part. That's not really a problem with the current PR for multiple worlds (minor commit to come, though), but:
|
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.
LGTM, thanks :)
I've made some changes to 'marker_layer.dart' (9591ee6
(#2000)), let me know what you think. I've changed some variable names so they're hopefully a little clearer, added some comments, etc.
I've also split the nested while-loop into two separate for-loops - it's still infinite, but means the shift
is now in the scope of the loop. This does mean a little extra code duplication, but it's only 3 short lines so hopefully the improved readability is worth it. Let me know what you think.
EDIT: Separately, I've also noticed that onTap
(and probably the rest of the handlers in MapOptions
) callback in the Markers page in the example does not add the marker to the location tapped, if that location is not in the main world. It adds it to the closest point in the main world.
Marker
s across all worlds
Thank you @JaffaKetchup for your review!
I have trouble understanding final main = getPositioned(0);
if (main != null) yield main; in order to echo what we do later with "additional": final additional = getPositioned(shift);
if (additional == null) break;
yield additional;
I'm OK with that code duplication, it makes the code easier to read and understand. Still, you removed my
I've just tested again the "Markers" example, and could replicate the bug. For the record this example is blocked on a single world. I'd test a mix of the "Markers" example in multi-world, but flutter_map upgraded to the latest version of flutter, which isn't convenient for me at the moment. |
Whilst it's not common, we do use it in the codebase already (9a13031#diff-6a0e5babb19fc7244d14a999a3fb70c338926f0c64bae5e9cba5a66c313dd6a7R127). Whilst I would agree that the question mark would make me think it allows it to be nullable. I didn't do it for if (getPositioned(shift) case final additional?) {
yield additional;
} else {
break;
} I've changed it back to without a if-case statement anyway.
Removing the TODO was accidental, but I'm also unsure of what implementations we could use here. However, I don't think my changes changed the flow at all - it checks the same number of times as before AFAIK. |
Thank you @JaffaKetchup!
That's the general idea of a TODO: flagging something as "to do", not being sure how to do it, or even if it's feasible or a good idea after all.
Indeed, you didn't change "my" flow. But compared to before the current PR, now we systematically check at least 2 more times, east and west. Because of the Tokyo/Los Angeles bug. |
I've added the TODO back with that little bit more explanation. Tbh, I'm not sure it's totally necessary. |
You're probably right, a couple of extra tests shouldn't be a problem regarding performances. Or maybe with very specific cases, like all the cities in the UK: many markers in just one centered portion of the world. But then we'd probably have other performance issues ;) |
This PR replicates Markers in all visible world copies.
New file:
multi_worlds.dart
: Example dedicated to replicated worlds and related objects (e.g. Markers).Impacted files:
camera.dart
: added thegetWorldWidthAtZoom
methodmain.dart
: added references to new pageMultiWorldsPage
marker_layer.dart
: now tries to add/subtract world width to marker abscissa for replicationmenu_drawer.dart
: added references to new pageMultiWorldsPage