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: replicate Markers across all worlds #2000

Merged
merged 10 commits into from
Jan 1, 2025

Conversation

monsieurtanuki
Copy link
Contributor

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

Screenshot_1734194431
Screenshot_1734194471

monsieurtanuki and others added 3 commits December 14, 2024 17:43
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`
@josxha josxha requested a review from a team December 14, 2024 17:55
@monsieurtanuki
Copy link
Contributor Author

Btw I've just detected a bug in the current version (even before this PR):

  • you have a Marker in Los Angeles
  • you have a Marker in Tokyo
  • your map spans from Japan to California
  • if the map center is "east" - higher than -180, you see the LA Marker, but not the Tokyo Marker
  • if the map center is "west" - lower than +180, you don't see the LA Marker, but you see the Tokyo Marker
  • of course both Markers should always be visible at the same time

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:

  • it may mean that in some projection cases (when there's no active multi-world setting), we cannot see Markers (but we can wait until someone opens an issue about it)
  • we may not be able to optimize the code, and always check for replicate Markers in both directions even in cases that wouldn't obviously be concerned

@monsieurtanuki
Copy link
Contributor Author

I've just added Tokyo, for additional tests:
Screenshot_1734277668

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

@JaffaKetchup JaffaKetchup changed the title feat: Markers replicated in all world copies feat: replicate Markers across all worlds Dec 19, 2024
@monsieurtanuki
Copy link
Contributor Author

LGTM, thanks :)

Thank you @JaffaKetchup for your review!

I've made some changes to 'marker_layer.dart' (9591ee6 (#2000)), let me know what you think.

I have trouble understanding if (getPositioned(0) case final main?) yield main;, it's not a common syntax and from what I'd imagine that would accept null too.
Given that it's an open source project, I'd be in favor of more readable syntaxes. Especially given that later in the code you write it slightly differently, with a more standard "if (x==null)" syntax. Would make the code more explicit doing similar things for "main" and "additional"s, wouldn't it?
Something like

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

I'm OK with that code duplication, it makes the code easier to read and understand.

Still, you removed my TODO: optimization - find a way to skip these tests in some obvious situations. (not related specifically to that PR, as said earlier), and that may have an impact in performances. Before, we checked just the "main" option. Now, for every marker, we systematically check at least 3 times.

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.

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.

@JaffaKetchup
Copy link
Member

I have trouble understanding if (getPositioned(0) case final main?) yield main;, it's not a common syntax and from what I'd imagine that would accept null too.

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 additional because it requires an else, which then expands the if/else to 5 lines. It could be done for consistency.

if (getPositioned(shift) case final additional?) {
    yield additional;
} else {
    break;
}

I've changed it back to without a if-case statement anyway.

Still, you removed my TODO: optimization - find a way to skip these tests in some obvious situations. (not related specifically to that PR, as said earlier), and that may have an impact in performances. Before, we checked just the "main" option. Now, for every marker, we systematically check at least 3 times.

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.

@monsieurtanuki
Copy link
Contributor Author

monsieurtanuki commented Dec 20, 2024

I've changed it back to without a if-case statement anyway.

Thank you @JaffaKetchup!
I really have problems with the previous syntax, and the standard if is readable by any developer.

Removing the TODO was accidental, but I'm also unsure of what implementations we could use here.

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.
Imagine we're in a map smaller than the world, and with east lower than west west lower than east [edited] - in that case we probably don't need to check eastern and western.
Is that very relevant, optimization-wise? This I don't know either.

However, I don't think my changes changed the flow at all - it checks the same number of times as before AFAIK.

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.

@JaffaKetchup
Copy link
Member

I've added the TODO back with that little bit more explanation. Tbh, I'm not sure it's totally necessary.

@JaffaKetchup JaffaKetchup requested a review from a team December 20, 2024 11:11
@monsieurtanuki
Copy link
Contributor Author

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 ;)
Thank you!

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

Successfully merging this pull request may close these issues.

[FEATURE] Replicate Markers in all world copies
3 participants