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

Add clustering #542

Merged
merged 12 commits into from
May 15, 2024
Merged

Add clustering #542

merged 12 commits into from
May 15, 2024

Conversation

Helium314
Copy link
Owner

This is still very basic, it only increases quest dot radius with point_count.
Since we want to get rid of quest dots, this is obviously only some sort of demo / playground.

Currently

  • I don't understand what withClusterProperty is doing
  • I don't have an idea how the clusters should look like
  • I don't know what should happen when selecting a cluster (Zoom in? Open the highest priority quest? Show all contained quests? Nothing?

Help for continuing:
https://maplibre.org/maplibre-gl-js/docs/examples/cluster/
https://github.com/maplibre/maplibre-native/blob/main/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/style/GeoJsonClusteringActivity.kt

@westnordost
Copy link
Collaborator

I don't know what should happen when selecting a cluster (Zoom in? Open the highest priority quest? Show all contained quests? Nothing?

Zoom in sounds reasonable. However, note that there can be several quests on the same element, i.e. on the exact same position. The most practical solution would probably be to not show any clusters starting a certain zoom level anymore but only show quests.

I don't have an idea how the clusters should look like

I'd expect circles with a number in the middle. Maybe optionally colored by the magnitude of that number on a gradient, but since this would necessitate coming up with a good (best color-blind-friendly) palette, could do that some time later and just start with white.

@Helium314
Copy link
Owner Author

Now it's working in a very basic way. I'd like the zoom-in to contain all clustered pins, will have a look on how to do that later.

and make size dependent on number of quests (to avoid number being larger than circle)
@Helium314
Copy link
Owner Author

I think the basics are working now.
The number is inside a white circle, and the circle size slightly increases with number of quests. I did this because otherwise for more than 100 quests the number is too close to circle edge, or the circle for small numbers looks empty.

When clicking, the map is centered on the click position, and zoomed to the "cluster expansion zoom", i.e. the zoom at which the cluster splits into pins or smaller clusters. This could possibly be adjusted to zoom to the bbox that contains all children of the cluster.

Copy link
Collaborator

@westnordost westnordost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this change, I noticed that it would be cleaner if the click behavior (i.e. what if anything is returned when clicking) was handled in the components. That way, getClusterExpansionZoom doesn't need to be on the interface. (But anyways, I can also look into it after this is merged.)

Note that you added the cluster logic to the PinsMapComponent, but the click behavior only to the quests. I.e. the changes history view will have clusters but clicking on them does nothing.

Comment on lines 35 to 40
// todo:
// avoid clustering for multiple quests on the same element
// maybe replace circles with pins with the number written on them?
val layers: List<Layer> = listOf(
CircleLayer("pin-dot-layer", SOURCE)
.withFilter(gte(zoom(), 14f))
.withFilter(all(gte(zoom(), 14f), lte(zoom(), 17f), gt(toNumber(get("point_count")), 1)))
Copy link
Collaborator

@westnordost westnordost May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The circles completely replace the dots now, yet, they are only visible until zoom 17. So, quests that collide with each other are now completely hidden starting with zoom 17.

avoid clustering for multiple quests on the same element

Right. But there are also quests that are almost on top of each other in pretty much all zoom levels. So, I would say, that

  1. we do indeed need to keep limiting the clusters to a max zoom and thus because it is possible that two quests remain a cluster even at highest zoom level
  2. we do still need to display the quest dots on zooms that exceed that max zoom

And since many elements that do have quests, actually have multiple quests, it is probably inconvenient for users to effectively not being able to tap any quests before exceeding that max cluster zoom. So, it looks like the solution for this is

  1. for each unique pin position, only add the first quest pin to the geojson (after sorting by importance - I notice the sorting could also be put into the Manager - I think the Pin doesn't even need that property anymore the way it works now).

// maybe replace circles with pins with the number written on them?

Why, does it look better? Anyway, it looks like we can't get rid of quest dots, so the circles then will also likely not look out of place no?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do still need to display the quest dots on zooms that exceed that max zoom

Done

for each unique pin position, only add the first quest pin to the geojson (after sorting by importance

Done. Note that now the number of elements with quests is shown, instead of the number of quests. Not sure what users would expect.
This change makes it harder to go for high quest density areas, e.g. a 102 quest cluster got changed to 38, while a nearby 16 quest cluster changed to 13.

Why, does it look better?

Don't know, but my assumption was there would not be any dots any more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Note that now the number of elements with quests is shown, instead of the number of quests. Not sure what users would expect.

Okay, but we don't have any choice anyway, right? This is what is technically possible.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm just anticipating issues that have a high chance of the author being @mnalis 😁

I guess technically it would be possible to add a property containing how many quests are at this specific position... but that could also be added later.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm just anticipating issues that have a high chance of the author being @mnalis 😁

Well, if the question is about "now the number of elements with quests is shown, instead of the number of quests", I would probably indeed prefer number of quests @Helium314 , but for once 😄 , that doesn't look like a deal-breaker to me, so if the other option looks much easier, go for it! (and if it is about something else, please specify, as this PR feels a li'l bit over my head)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure it's not a dealbreaker, but the number of quests would probably be more interesting for the user....

@Helium314
Copy link
Owner Author

what if anything is returned when clicking

What should be returned by the component? A camera update?

@westnordost
Copy link
Collaborator

What should be returned by the component? A camera update?

I didn't completely think this through, but I was thinking along the lines of that the OverlayComponent would return the ElementKey, the QuestPinComponent the... hmm, I guess the properties. And in case of clicking on a cluster, do the zoom-in itself.

@westnordost
Copy link
Collaborator

westnordost commented May 13, 2024

(Maybe it's a bit dirty that the PinsComponent is used by two different data sources and then switched forth-and-back. It could also be solved by it being a class with a name parameter or an abstract superclass and result in having two completely separate components)

@Helium314
Copy link
Owner Author

Helium314 commented May 13, 2024

I was thinking along the lines of that the OverlayComponent would return the ElementKey, the QuestPinComponent the... hmm, I guess the properties. And in case of clicking on a cluster, do the zoom-in itself.

So this is more general than the scope of this PR then?
I guess it would be better to do it after the PR is done.

Then for this I'd play a little with trying to get a better zoom / target bbox out of getClusterChilden or getClusterLeaves, and maybe try playing with background color depending on number of elements with quests.

[edit: FeatureCollection has a bbox, which would be really convenient here, but it seems to always return null]

@westnordost
Copy link
Collaborator

So this is more general than the scope of this PR then?

Yes

@westnordost
Copy link
Collaborator

[edit: FeatureCollection has a bbox, which would be really convenient here, but it seems to always return null]

There should be a function in SphericalEarthMath somewhere that calculates the bbox of a set of latlons.

@Helium314
Copy link
Owner Author

There should be a function in SphericalEarthMath somewhere that calculates the bbox of a set of latlons.

I have it already working, but it's a bit annoying to deal with. Will commit the current state and continue working.

@Helium314
Copy link
Owner Author

So the idea of returning a CameraUpdate from the cluster Feature will not work, because we need MapLibreMap.getCameraForLatLngBounds.

@Helium314 Helium314 marked this pull request as ready for review May 13, 2024 19:23
@Helium314
Copy link
Owner Author

the sorting could also be put into the Manager

Not sure whether it's worth the work, doesn't look like a straightforward change to me.

@westnordost
Copy link
Collaborator

Really? It looked to me like a straightforward change. No matter. I just got an idea that may or may not look nice:

I offset the quest dots a bit up so that they are hidden behind the pins. The same technique could be used to move the cluster circle up so that it's circular area is where the pin circle has it's center. This may look quite nice when zooming in, because the circle may look kind of like it turns into a pin.

@Helium314
Copy link
Owner Author

I just tried offsets, and it looks really awkward when zooming, because the circles move relative to the surrounding map geometry.
Also the cluster circles are typically located somewhere between pins, so they will not turn into a pin anyway.

Btw it looks like the circleTranslate moves the circles on the map, and not on the screen. Try rotating the map by 180° and you will see the circles appearing from behind the pins.

@Helium314
Copy link
Owner Author

Though maybe the circleTranslate issue can be fixed using circleTranslateAnchor

@westnordost
Copy link
Collaborator

westnordost commented May 13, 2024

Oh right, probably circleTranslateAnchor=viewport

Did that now on maplibre branch.

@Helium314
Copy link
Owner Author

I tried playing with colors, essentially copying from the example in first post, just using point_count and different stop values and colors.

There was nothing I really liked so far, and possibly simple white just fits best into SC.

Is there anything else you would like to change or try? Otherwise I think the PR would be ready.

@westnordost
Copy link
Collaborator

No, looks fine!

If colors don't look nice, another angle could be to play with bold font vs normal font / text size.

@westnordost
Copy link
Collaborator

I've finally taken a look and tuned a small thing. Merging!

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.

3 participants