-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Introduce a deriveIntegerId vector source option #8448
Conversation
@mourner how does the developer experience change? Can we simply remove the type restriction for the id property, or does the user have to specify that hashing is required? (might need a docs update) |
@peterqliu oh, good point — updated the description. You have to explicitly enable it and specify a property name to use. |
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.
First of all - this is a mind-blowingly simple solution!
Couple thoughts:
- naming:
deriveIntegerId
doesn't seem very intuitive. geojson-vt usespromoteId
for the same option. - source-layer. The tileset from the example below uses different property names for the
state
andcounty
source layers. Should this option be exposed per source-layer? Would that be necessary for composited tiles?
It works great with the vector source used in: https://docs.mapbox.com/mapbox-gl-js/example/updating-choropleth/
@asheemmamoowala I initially named it Or do you think we should change the semantics in
Good point. We could either accept a string value (if we want the same property name for all source layers) or an object of the form |
I think they should be consistent - so adding the murmur hashing would allow generating the id from property of any type, which is highly requested. Having the same name for the option would also make it easy for developers to use it in all contexts.
I agree,
👍 |
@mourner this is excellent! If we were to expose Here's an example of how a public method would allow developers to convert existing data-join maps (like our public example) to use feature state, while simplifying their code: map.on('load', function () {
// Add source for state polygons hosted on Mapbox, based on US Census Data:
// https://www.census.gov/geo/maps-data/data/cbf/cbf_state.html
map.addSource("states", {
type: "vector",
url: "mapbox://mapbox.us_census_states_2015",
deriveIntegerId: 'STATE_ID'
});
// Set the color for the feature ID using featureState
data.forEach(function (row) {
map.setFeatureState({
source: 'states',
sourceLayer: 'states',
id: mapboxgl.getIntegerId(row["STATE_ID"])
},
{
unemployment: row["unemployment"]
});
};
// Add layer from the vector tile source, styled based on the `unemployment` value
// in featureState with an expression
map.addLayer({
"id": "states-join",
"type": "fill",
"source": "states",
"source-layer": "states",
"paint": {
"fill-color": ['interpolate',
['linear'],
["feature-state", "unemployment"],
0, '#e5f5f9',
5, '#b2e2e2',
10, '#66c2a4',
15, '#238b45'
]
}
}, 'waterway-label');
}); Advantages:
Disadvantages:
|
I have the same question as @peterqliu. Can we remove this restriction on the type and then have a The external API is simpler if the user can provide the value directly in the feature state api ( |
@ansis this is what I was hoping for with the suggestion in #8448 (comment): BTW - should we combine this conversation with #6019 ? |
Note that the vector tile spec only supports uint ids, so ideally we would fist need to adopt a new major spec version, otherwise this could get confusing. E.g. you can use Also, there can be major performance implications to supporting arbitrary IDs — e.g. currently FeauturePositionMap relies on ids in typed arrays for fast sorting and transferring. So @asheemmamoowala's suggestion (converting to integers internally but exposing as original types) might be more feasible, although I would still prefer explicit conversion for a much simpler, less magical implementation. |
Yep, that sounds good!
We could work around this, right? by having a special property ( |
@mourner @chloekraw This is excellent! I have a couple points of feedback, mostly of things to keep in mind for product management.
|
@samgehret, ID generation will also be a part of MTS; we prototyped this as another interim solution that will hopefully unblock some customer use cases. Are there specific challenges/drawbacks you have in mind with using this feature in GL-JS? |
@chloekraw I think it will be important for people to be able to translate both ways between the feature ID, and the attribute it was generated from. This will be important when doing client side data join. Often times developers will need to take an attribute from their geojson (say some FIPS ID or Post code ID), use that attribute to generate feature IDs (this is doable in this PR). Their local data will still only have the FIPS ID, so how can they reference the feature ID in |
@samgehret Thanks for the feedback! #8448 (comment) and #8448 (comment) are considering ways to make the |
@mourner I've tried this change out and it's going to work for us. We will be able to reduce our footprint and improve performance. |
bbc5cbf
to
b59df3c
Compare
Alternative proposal in #8987 |
A proof of concept that introduces a
deriveIntegerId
option for vector tile sources. It allows you to specify an existing feature property (that may be a string) to generate a corresponding integer feature ID using murmur3 hashing, enablingfeature-state
for vector tile sources without integer IDs but with unique string ids in properties (e.g. administrative boundary ids like"CN.A.230000.1"
).To use it, add a
deriveIntegerId: <propertyName>
option to the source:Seems to work well for an admin boundaries sample I tested this on. If the concept looks good, I'll work on adding tests and addressing any review comments.
TODO:
querySourceFeatures
not including derived ids.feature-state
with derived IDs.Launch Checklist