Support nested properties with the 'get' expression

[ahocevar]

Fixes #15505.

This pull request implements what was discussed in #15505.
Examples for the new features:

Nested properties:

{
  context: {properties: {deeply: {nested: {property: 42}}}},
  expression: ['get', 'deeply', 'nested', 'property'],
  expected: 42
}

Nested properties with array:

{
  context: {properties: {answer: {at: {index: [17, 42]}}},
  expression: ['get', 'answer', 'at', 'index', 1],
  expected: 42
}

This change is backwards incompatible, the second argument to the get expression is no longer interpreted as type hint.

[github-actions]

📦 Preview the website for this branch here: https://deploy-preview-15813--ol-site.netlify.app/.

[tschaub]

Nice! Thanks for taking this on.

I realize this may sound stubborn, but I’d like to see a real example where type hints are needed before introducing another operator that we might just remove later. By “a real example” I mean an expression used in a style object.

[ahocevar]

@tschaub Actually it's the ambiguity of size and scale properties, which use NumberType | NumberArrayType as type. This could be fixed by introducing a SizeType. Once we have that, we can get rid of the type hints. I'm going to create a separate pull request with that change.

[ahocevar]

@tschaub Actually it's the ambiguity of size and scale properties, which use NumberType | NumberArrayType as type. This could be fixed by introducing a SizeType. Once we have that, we can get rid of the type hints. I'm going to create a separate pull request with that change.

See #15818. I'll rebase and modify this pull request accordingly once that's in. And we could also make it so this no longer is a breaking change, by ignoring object literal keys at the end of the chain when the previous value is not an object.

[ahocevar]

Tests will pass once #15818 is merged and this pull request is rebased.

[ahocevar]

Rebased again, without a merge commit. Thanks for any review.

[tschaub]

Should we continue on null?

[ahocevar]

Good point, I‘ll add a null check tomorrow.

[M393]

Looks like with properties {"a": 1} and ["get", "a", "b"] it would currently return 1?
Same for properties {"a": [1]} and ["get", "a", "b"] would return [1]?

Should this return undefined instead?

[ahocevar]

Returning undefined in such cases was my initial approach, but then I thought it might be useful to allow data structures with variable nesting depth - and be backwards compatible with the removed type hint as last argument.

If people think there is no practical use of this, and it rather adds confusion, I'll be happy to go back to the initial approach and make this a breaking change.

[tschaub]

I think we should move on from the type hint.

[ahocevar]

Ok, updated. Now it's a breaking change.

[tschaub]

We aren't leveraging the properties of the parsing context right now in Canvas, but here is the idea. As we parse style expressions, we build up a lookup of all the data that is needed from a feature (feature id, geometry, properties). Then during the evaluation phase, we could supply a minimal set of data in the evaluation context.

Currently, we don't do this in Canvas. Instead we add all the feature properties to the evaluation context. If we were passing the data around to/from workers, we might instead want to encode a subset of the data for that purpose.

In the WebGL renderer, the properties from the parsing context is passed to the evaluation context and then used when building shaders and uploading attributes to the gpu (based on a limited set of feature properties instead of all feature properties).

To support working with a subset of feature properties in the Canvas renderer in the future, we want to keep track of these "property paths" that are used in style expressions. Now that we support deeply nested paths, instead of the parsing context properties being Set<string> it could be Array<Array<string>> (where a single entry might be ['path', 'to', 'property']. Since deeply nested paths aren't supported in WebGL yet, this would break things on that side. For now, since we aren't using the parsing context properties in Canvas, we could just store a dot delimited path to all the properties (e.g. 'path.to.property'). This isn't the best long-term solution if people do crazy things like put dots in their property names, but it could be fine for now.

The tl;dr is that we don't want to add 'path', 'to', and 'property' to this context.properties set. Instead we could add 'path.to.property' (and then later change the type when we want to fully support nested properties in both Canvas and WebGL).

[tschaub]

Thanks for your work on this, @ahocevar. It feels like a simpler and more functional solution now (together with #15818).

Hope my comment above makes sense. Since we aren't currently using the parsing context properties set on the Cavnas side, it doesn't break anything to leave the code as you have it. So feel free to merge with or without the suggested change.

[ahocevar]

Thanks for the thorough review, @tschaub. Your suggested change sounds interesting. I think it makes sense as a follow-up change at a later time, and maybe it also makes sense to solve the problems a dot delimited properties path would impose before doing that.

[santilland]

Great feature for flat styles, thanks for this!

[tschaub]

I think it makes sense as a follow-up change at a later time

I'm working on support for accessing nested properties in WebGL here: main...tschaub:openlayers:nested (this reworks the property handling as described above).