-
Notifications
You must be signed in to change notification settings - Fork 620
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
Add support for tiles to geoshape mark #8885
base: main
Are you sure you want to change the base?
Conversation
Nice work already @binste! Regarding the required lifting of parameters to top-level, I would prefer to move the mechanism we introduced in Altair, vega/altair#2702 to Vega-Lite so this lifting can be done when serializing it (as macro?) into extended Vega-Lite. This also would mean that Vega-Lite can better controls the naming definition of parameter views. |
Sounds good to me! I'll pause the development here and wait for further feedback regarding the parameters question. @mattijn I noticed that in some settings, there is a slight gap between the tiles: Do you know how the placement of the tiles should be adjusted to close it? |
So far I only saw the gap in the Vega Editor, not in VS Code with the same settings. Maybe the placement of the images is sometimes slightly different. If I add 1 pixel to the height and width of the image marks then the gap is also closed in the Vega Editor. |
I started working on https://github.com/binste/altair_basemap to implement this on the Altair level. I still think it makes sense to implement it in Vega-Lite but until we can resolve the open issues and questions here it could be nice to have a Python implementation to experiment and to bridge over the time until this PR is merged. |
This PR is far from being ready to be merged as many corner cases need to be handled, tests and documentation written, etc. But I have a few questions on how to proceed which are probably easier to answer if you can see the code. The goal is to add a
tile
property to thegeoshape
mark which adds tiles from arbitrary tile XYZ tile servers asimage
marks. See #8767 for more context and all the credits for the calculations goes to @mattijn who provided the initial specs!Current status
I already got the layering to work so that the following spec (notice the
"tile": true
in"mark"
):produces an extended VL spec in the Vega Editor which is very close to what @mattijn originally came up with. The main change is that I rewrote the calculations so that they do not depend on an input zoom level but instead the zoom level is determined based on
scale
inprojection
:This VL spec renders fine:
Questions
There are two related issues around the parameters in the above spec where I'd be very grateful for guidance from @domoritz or any VL maintainer.
LayerSpec
. I managed to push them to the top-level of the processed view as in the example above, but this does not work if the above spec is wrapped into anhconcat
chart. Any ideas (existing mechanisms?) how I could push these parameters to the top of any spec, independent of how deeply nested the chart spec is which has"tile": true
?vega.parse
. The spec which is eventually passed lost the parameters section. I think this happens here https://github.com/vega/vega-lite/blob/main/src/compile/compile.ts#L122.spec
has the parameters,vgSpec
does not. You can see the behavior I described in this screen recording:Screen.Recording.2023-05-06.at.17.06.10.mov
If it's not possible to do what I want with the parameters, I could maybe rewrite the generated spec so that all expressions are inlined but that would lead to a lot of duplicated calculations and makes the resulting VL and Vega specs less readable in my opinion.
Any help is greatly appreciated! Thanks.
Linked issue: Closes #8767.