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

Implement clustering properties to ShapeSource #1745

Merged
merged 8 commits into from
Apr 25, 2022

Conversation

ivari
Copy link
Contributor

@ivari ivari commented Feb 17, 2022

Description

Years ago, Mapbox implemented clusterProperties attribute for ShapeSource, which enables declaring custom aggregated properties for clustered features on the map.

Android: mapbox/mapbox-gl-native#15425
iOS: mapbox/mapbox-gl-native#15515

I wanted to see this implemented, as we have a use case in our app where we want to open a new view on cluster tap, which shows information for all features clustered under the icon. Additionally, we want to show information on the icon which is more complex than simple {point_count}.

So, finally, we got around to implementing it. Here's the PR.

The fork is already running in production for our apps as well.

Relevant feature request: #693

Checklist

  • I have tested this on a device/simulator for each compatible OS
  • I formatted JS and TS files with running yarn lint:fix in the root folder
  • I updated the documentation with running yarn generate in the root folder
  • I mentioned this change in CHANGELOG.md
  • I updated the typings files (index.d.ts)
  • I added/ updated a sample (/example)

Screenshot OR Video

See the video from our implementation in our B2B app.
https://user-images.githubusercontent.com/5424427/154564667-1ea37d1e-e9e7-4377-9faf-c5c6090c7530.mov

@ivari ivari force-pushed the implement-clustering-properties branch 2 times, most recently from a93b1c9 to 37f8c71 Compare February 17, 2022 20:32
Copy link
Contributor

@mfazekas mfazekas left a comment

Choose a reason for hiding this comment

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

👍 Thanks very much, looks great, to me. Can you update with current main?!

@ivari ivari force-pushed the implement-clustering-properties branch from 37f8c71 to 3229c32 Compare March 6, 2022 17:03
@ivari
Copy link
Contributor Author

ivari commented Mar 6, 2022

👍 Thanks very much, looks great, to me. Can you update with current main?!

Thank you! Rebased to latest main.

@tiagotwistag
Copy link

Hey, any clue when we can get this merged in?

@mfazekas
Copy link
Contributor

@ivari I'm sorry can you rebase again?! Also can you extend a note in README that it was implemented on maplibre/mapbox-gl

@naftalibeder
Copy link
Collaborator

@ivari If you don’t mind doing the couple of things @mfazekas mentioned, we can merge this right away. Thanks!

ivari and others added 7 commits April 25, 2022 19:15
For some reason the URL fetching no longer worked.
We were unable to support the simplified syntax properly on iOS.
Changed documentation to only suggest the advanced syntax.
We would be happy to see someone add to our PR.
@ivari ivari force-pushed the implement-clustering-properties branch from 3229c32 to cd32098 Compare April 25, 2022 16:22
@ivari ivari force-pushed the implement-clustering-properties branch from cd32098 to dfbea09 Compare April 25, 2022 16:27
@ivari
Copy link
Contributor Author

ivari commented Apr 25, 2022

@mfazekas, @naftalibeder, my apologies, was on sick leave for a week. Done as you requested, now.

@naftalibeder
Copy link
Collaborator

Thanks @ivari!

@naftalibeder naftalibeder merged commit cd9d940 into rnmapbox:main Apr 25, 2022
@woodybury
Copy link

@ivari amazing thanks for this!

Do you mind sharing a basic example of usage. Im unable to get it to work. Here's what I got:

<ShapeSource
  id="source"
  hitbox={symbolHitbox}
  shape={featureCollection}
  cluster={true}
  clusterMaxZoom={14}
  clusterRadius={50}
  clusterProperties= {{
	  "sum": ["+", ["get", "count"]] // count is a features.properties key on faetureCollection geojson. value is type int
  }}
  >
  <SymbolLayer
	  id="avatarPoints"
	  style={symbolStyle}
	  filter={['!', ['has', 'point_count']]}
  />
  <SymbolLayer
	  id="pointCount"
	  style={pointCountStyle}
  />
  <CircleLayer
	  id="clusteredSymbol"
	  belowLayerID="pointCount"
	  filter={['has', 'point_count']}
	  style={clusterSymbolStyle}
  />
</ShapeSource>

and pointCountStyle:

const pointCountStyle = {
	textField: '{point_count}-{sum}', // sum is does not exist here
	textSize: 12,
	textColor: 'white',
	textPitchAlignment: 'map'
};

@ivari
Copy link
Contributor Author

ivari commented May 18, 2022

@WoodburyShortridge please see example at example/src/examples/SymbolCircleLayer/EarthQuakes.js

From first glance, the problem you are having is that you are using the short-hand ["+", ["get", "count"]]. Please instead use the full accumulator definition for adding up the values, as in the example.

I'll be honest here and say that my teams Obj-C skills were not enough to implement the short-hand on iOS. I'd love to see someone take our code and fix that, but until then you can get the exact result you need with just a bit of extra boilerplate.

@woodybury
Copy link

thanks @ivari makes sense. I've changed it to use the accumulated def sum: [["+", ["accumulated"], ["get", "sum"]], ["get", "count"]] however I'm still not getting the cluster property sum on the symbol layer at all.

I'm at the latests from rnmapbox/maps#main and $RNMapboxMapsVersion = '~> 10.5.0-beta1'

Is there something else I am missing here? Really appreciate your help and thanks again for the PR and pointing me to the example app 👍 !

Feature collection:

const featureCollection = {
'type': 'FeatureCollection',
'features': data.map(d => ({
	'type': 'Feature',
	'properties': {
		'iconSize': .5,
		'count': d.count, // int
	},
	'geometry': d.loc,
	'id': d.id,
})),
};

Shape source:

<ShapeSource
  id="source"
  hitbox={symbolHitbox}
  shape={featureCollection}
  cluster={true}
  clusterMaxZoom={14}
  clusterRadius={50}
  clusterProperties= {{
	  sum: [["+", ["accumulated"], ["get", "sum"]], ["get", "count"]]
  }}
  >
  <SymbolLayer
	  id="avatarPoints"
	  style={symbolStyle}
	  filter={['!', ['has', 'point_count']]}
  />
  <SymbolLayer
	  id="pointCount"
	  style={pointCountStyle}
  />
  <CircleLayer
	  id="clusteredSymbol"
	  belowLayerID="pointCount"
	  filter={['has', 'point_count']}
	  style={clusterSymbolStyle}
  />
</ShapeSource>

Point count style:

const pointCountStyle = {
	textField: ['get', 'sum'], // nothing here
	// textField: ['get', 'point_count'], // this works
	textSize: 12,
	textColor: 'white',
	textPitchAlignment: 'map',
};

@ivari
Copy link
Contributor Author

ivari commented May 19, 2022

@WoodburyShortridge sorry to disappoint you even further. We use $RNMapboxMapsVersion = '~> 5.9.0'. Never tried on 10. I can only assume it doesn't work on 10 as is. 🙁

@woodybury
Copy link

ahhh gotcha thanks @ivari 🙁. @mfazekas any insight into why this may not work on more recent versions?

@mfazekas
Copy link
Contributor

@WoodburyShortridge can you add an issue for that?! It's just not yet implemented for v10 mapbox versions yet. Shouldn't be hard to implement.

@ahardy42
Copy link

is this implemented on the v8 branch? I don't see this clusterProperties prop used in the Earthquake example file. I can't get it to work in my project, and I'm not sure if it's just that I'm terrible with Mapbox expressions.

@mfazekas
Copy link
Contributor

@ahardy42 I'm sure it's on #main so test with that

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.

7 participants