Add projection expression syntax #888
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## projection-expression-collecting-branch #888 +/- ##
==========================================================================
Coverage ? 92.63%
==========================================================================
Files ? 107
Lines ? 4749
Branches ? 1346
==========================================================================
Hits ? 4399
Misses ? 350
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
The problem with the style spec is that we need to support it "forever", so I don't want to introduce something that later on will cause maintenance issues. |
|
@HarelM , I understand what you mean.
It's the Taking a more conservative, forward looking, approach, I've changed this PR to remove the globe-to-mercator, meaning that there's just the I've opened a PR here to align with that strategy |
birkskyum
changed the title
globe projectionglobe and mercator expression syntax
birkskyum
changed the title
globe and mercator expression syntax
birkskyum
force-pushed
the
add-non-adaptive-globe
branch
2 times, most recently
from
06496eb to
d1cad9d
Compare
|
I've designed an expression based on the step. It doesn't interpolate, so that has to happen in the implementation, which it does already. Is it something like this you have in mind? Each transition between projections would be a hard cut untill a specific adapter is introduced, like the globe-to-mercator adapter we have. |
|
I think this is in the right direction, thanks! |
|
So since it's a step, there's no interpolation in the expression. If we can get an interpolation value between each step from 0-1 that would be cool, i just don't know how to do it because it says "interpolation not allowed for strings", but I imagine it just need it's own type defined. We need adapters for the combination, so say it's: Then we have to look up if we have an adapter, like If we don't have an adapter, it would make a hard cut from one to the other. If we do have, then we can interpolate and apply the transition, which here is named globeness. We might be able to generalize the transitions, by making a transition from each layer to mercator, and then they can go through that to each other, like an Internal Representation. If we have Then a style like this should be possible by going through mercator. There would ofc be a speedup by having a direct globe < - > equalEarth, but I think it's for good reason that Google Translate and LLVM has translation layers, and mercator seem like an obvious choice albeit more because of tradition than technical reasons . |
|
Besides the comment above this looks good. |
|
@HarelM , I have some troubles running the mkdocs. It seems like recursion error in some social plugin. |
|
Ah, I had an old |
|
I think I had it too, don't remember how I solved it, you can comment out the social part in the mkdocs config to overcome this locally, just make sure you don't commit this change. |
|
@HarelM , I've implemented a syntax in the style-spec that I'm satisfied with for the time being: string - No transition:step - Non-adaptive transitionWill output at a given zoom level i.e. "mercator" interpolate-projection - Adaptive transitionsWill at any point output i.e. for zoom 11.5: ["globe", "mercator", 0.75] ( altertively ["globe-to-mercator", 0.75] ) We have room in the implementation to take shortcuts, like if we see zoom 5 as My challenge now is that I don't know how to retrieve the cameraexpression output in gl js |
birkskyum
force-pushed
the
add-non-adaptive-globe
branch
from
240a59a to
782609d
Compare
|
You should be able to call I'm not sure about the return value though. In theory (not relevant to current implementation) we might consider an object with all the field that are described in proj4js: Right now it can be a "named" protection to simplify things. |
|
@HarelM , the return type is a ProjectionTransition for ( |
| @@ -170,6 +173,10 @@ class Interpolate implements Expression { | |||
|
|
|||
| const outputLower = outputs[index].evaluate(ctx); | |||
| const outputUpper = outputs[index + 1].evaluate(ctx); | |||
|
|
|||
| if (this.type.kind == 'projection') { | |||
There was a problem hiding this comment.
I think this can be added to the below switch-case?
There was a problem hiding this comment.
A type guard on this.type.kind !== 'projection' is needed, so it turns the switch into this which I think looked worse:
switch (this.operator) {
case 'interpolate':
if (this.type.kind !== 'projection') {
return interpolate[this.type.kind](outputLower, outputUpper, t);
}
case 'interpolate-hcl':
return interpolate.color(outputLower, outputUpper, t, 'hcl');
case 'interpolate-lab':
return interpolate.color(outputLower, outputUpper, t, 'lab');
case 'interpolate-projection': {
return interpolate.projection(outputLower, outputUpper, t);
}
}
```There was a problem hiding this comment.
This is what I'm dealing with
It's been a problem since 'projection' was added as a possible value to .kind:
There was a problem hiding this comment.
The stupid part is that this, or insertion of any of the other possible string values for kind, doesn't have a type problem
There was a problem hiding this comment.
case 'interpolate':
return interpolate[this.type.kind.toString()](outputLower, outputUpper, t);
case 'interpolate-hcl':
return interpolate.color(outputLower, outputUpper, t, 'hcl');A kind.toString() resolves it
There was a problem hiding this comment.
I think this removes the type guard from the interpolate object, which I'm not sure is what we want.
I generally try to avoid using object[property] as much as possible due to these limitations and the fact that it's harder to read the code this way.
There was a problem hiding this comment.
This is an option too, that doesn't ignore any types
switch (this.operator) {
case 'interpolate':
switch (this.type.kind) {
case 'number':
return interpolate.number(outputLower, outputUpper, t);
case 'color':
return interpolate.color(outputLower, outputUpper, t, 'rgb');
case 'array':
return interpolate.array(outputLower, outputUpper, t);
case 'padding':
return interpolate.padding(outputLower, outputUpper, t);
case 'variableAnchorOffsetCollection':
return interpolate.variableAnchorOffsetCollection(outputLower, outputUpper, t);
}
case 'interpolate-hcl':
return interpolate.color(outputLower, outputUpper, t, 'hcl');
case 'interpolate-lab':
return interpolate.color(outputLower, outputUpper, t, 'lab');
case 'interpolate-projection': {
return interpolate.projection(outputLower, outputUpper, t);
}
}There was a problem hiding this comment.
I like this one better to be honest.
There was a problem hiding this comment.
By the way, if this is possible for other types, why do we need an extra case for interpolate-projection then?
| @@ -180,6 +181,8 @@ function typeToMarkdownLink(type: string): string { | |||
| case 'paint': | |||
| case 'layout': | |||
| return ` [${type}](layers.md#${type.toLocaleLowerCase()})`; | |||
| case 'projectionconfig': | |||
| return ' [projectionConfig](projection.md)'; | |||
There was a problem hiding this comment.
Is this really needed here?
This is used for the docs to allow link to an object type related to the expressions I think, and I'm not sure projectionConfig can be a return type of an expression, right?
There was a problem hiding this comment.
This is an overwrite I made so that the url of the main Projection page doesn't get a /projectionConfig url, which I thought was undesirable, compared to staying with /projection. I can let it generate the /projectionConfig if preferred.
This is the warning I got on mkdocs-build, when it makes the root pages from the v8, before I added it:
WARNING - Doc file 'root.md' contains a link 'projectionConfig.md', but the target is not found among documentation files.
There was a problem hiding this comment.
Ah, I see...
I'm trying to think if there's a way to solve this elegantly, but I can't think of a good idea at the moment.
There was a problem hiding this comment.
I'm not sure this is a good idea, but in proj4js they use the term ProjectionDefinition for the values related to the projection:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/proj4/index.d.ts
I'm not sure what I think about it though - this is for the "internal" type, the one used in the expressions, not the root projection type.
|
This looks great!!! thanks! |
|
Can you create a branch in this repo and merge this PR to this new branch so I'll see if I can try out some things in order to solve the parts that still annoys me? |
|
Please also checkout the code coverage report, there are other stuff that are not covered there, which we need to think how to address, maybe out of scope for this PR. |
birkskyum
changed the base branch from
main
to
projection-expression-collecting-branch
|
I've made a |
birkskyum
merged commit 107bd0e
into
maplibre:projection-expression-collecting-branch
|
Yes, thanks! |
This reverts commit 107bd0e.
merging branch here
Upcoming syntax:
See Video examples here
projection: { type: VALUE }
VALUE can be either of below
stepExpressioninterpolate-projectionExpressioninterpolate-projectionexpression like above.Just one of many motivations for this feature is the need for a non-adaptive
globefor the reasons hereGenerally, we need a way to specify adaptive and non-adaptive transitions in the style spec, with stops, and we need much more flexibility regarding projections, where this syntax is future proof for +proj and WKT strings support.