Pass userProperties to midpoints and vertices

[JamesLMilner]

This PR brings @drewbo PR for this issue (#847) up to speed with master. The aim here is to pass userProperties to midpoints and vertices, and in turn allow them to be styled.

Resolves #846

[asheemmamoowala]

LGTM!

[JamesLMilner]

@asheemmamoowala thanks for the review, much appreciated! What would be the next steps to get this merged? I can see #944 is also in a similar state. Would be amazing to get these two approved PRs merged if possible as they'd be very beneficial for us and the community at large 😄

[JamesLMilner]

@kkaefer thanks for merging the other PR. I saw you merged master into this PR, so thanks for keeping this up to date too. Any update on the steps that need to happen to get this merged? :)

[usb248]

+1

[awulkan]

Has this PR been put on hold? There has been pull requests for this since 2018 and they always get stuck. Are you waiting for something specific before it can be merged and released? Or is this a feature Mapbox don't want to support?

[caldwellc]

+1

[adrianababakanian]

Thanks for your contributions @JamesLMilner and @drewbo ! This PR is working well on a few small test cases I put together locally, and I agree that having the ability to style midpoints and vertices via userProperties is a great feature.

However, before moving forward with merging, I believe it's worth considering the performance implications introduced. Since parent.properties is actually being copied to child vertices and midpoints, the amount of duplicated data associated with each feature can grow quickly as midpoints are "recursively" added between vertices. This likely won't have too much of a negative impact if a relatively small number of userProperties are introduced. However, since any given GeoJSON feature can have an arbitrary number of user-defined properties added, performance could be noticeably affected when a large number of midpoints + vertices are combined with many userProperties.

Perhaps the same result can be achieved by still passing only the parent ID to midpoints and vertices, and then looking up the relevant parent.properties when needed (i.e. when styling relevant layers). Do you see this approach as a valid solution for your intended use case?

[adrianababakanian]

It's worth noting that, per MDN's browser compatibility docs, IE does not support spread syntax.

[JamesLMilner]

That's a good spot, I think a for ... in loop might cover it for IE support

[JamesLMilner]

Hi @adrianababakanian, thanks for your follow up, really happy to hear you had a chance to test it locally.

Performance isn't something I had considered as I'd mostly taken @drewbo code and brought it in line with main. I can see why that might be problematic however as it grows.

I am pondering about your suggestion about looking up the parent properties using the ID, and the one thing I'm not sure about is if that would be possible using the Mapbox GL style specification? Maybe an alternative solution is to keep the parent as a reference to the parent object then you could do a match lookup on the parent properties? That might have added knock on effects and added complexities however. I guess I am trying to determine how it would be possible to style the vertices based on the parent properties using only the parent ID, if that makes sense?

[JamesLMilner]

@adrianababakanian I experimented with using a parentProperties property on the vertex/midpoints which should allow for you to write a style like this:

{
    id: 'gl-draw-polygon-and-line-midpoint-halo-parent-prop',
    type: 'circle',
    filter: [
        'all',
        ['match', ['geometry-type'], 'Point', true, false],
        ['match', ['get', 'meta'], 'midpoint', true, false],
        ['match', ['get', 'parentProperties', ['get', 'someParentProp'], true, false]
    ],
    paint: {
        'circle-radius': 5,
        'circle-color': '#FFF'
    }
}

You can check it here:
main...JamesLMilner:846-midpoint-user-props-parent-props

Perhaps this would be a more performant approach as we don't copy properties in this instance?

[usb248]

@adrianababakanian I experimented with using a parentProperties property on the vertex/midpoints which should allow for you to write a style like this:

{
    id: 'gl-draw-polygon-and-line-midpoint-halo-parent-prop',
    type: 'circle',
    filter: [
        'all',
        ['match', ['geometry-type'], 'Point', true, false],
        ['match', ['get', 'meta'], 'midpoint', true, false],
        ['match', ['get', 'parentProperties', ['get', 'someParentProp'], true, false]
    ],
    paint: {
        'circle-radius': 5,
        'circle-color': '#FFF'
    }
}

You can check it here:
main...JamesLMilner:846-midpoint-user-props-parent-props

Perhaps this would be a more performant approach as we don't copy properties in this instance?

This approach doesn't allow us to do dynamic style. Mapbox GL style specification doesn't allow to get properties in another geojson. It means that you can't get user's properties in a vertex while they are defined in (a parent) feature (linestring) for example.

Copy properties seems the only way

[Zirak]

Would it be feasible to use Object.create or similar to leverage inheritance, instead of copying?

[adrianababakanian]

Thanks for looking into this further @JamesLMilner . I agree that trying to make this work with only a reference to the parent ID adds more complexities than just copying the parent's properties. @usb248 brings up a good point that copying properties would be necessary to support fully dynamic styling.

In my opinion, introducing the Feature State API to GL Draw will be beneficial in the long term. Starting to use feature state will involve a larger refactor of GL Draw as a whole, but will bring many additional benefits beyond just the feature in this PR. I opened #994 to start a discussion around a few of these benefits. Using feature state would enable giving child features (i.e. midpoints and vertices) the same id as the parent feature for styling purposes.

[jaybo]

Is it correct to assume this same issue exists with 'Points' also? I'm trying to use:
layout: { ... "text-field": ['get', 'name'], ... },
or alternatively:
layout: { ... "text-field": "{name}", ... },
But it appears properties.name is not available to the style. Is this a generic issue with Draw, that properties are not available for styling?

[jbeuckm]

One year later... Can we merge this and add a warning in the md that adding a larger number of properties could introduce performance problems, especially with complicated shapes that render many midpoints and vertices? Not being able to style midpoints and vertices is holding us back.

[chenlevy4]

@adrianababakanian Can you please merge this PR?

[ThomasAribart]

@adrianababakanian @asheemmamoowala Need this PR for my project as well 🙏 Could you merge and release it please ?

[julietteff]

@adrianababakanian @asheemmamoowala This PR would also be very useful for my project ! 🤩

[igamidanes]

This would be amazing for us as well