-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[FEATURE] Vector tile layer - part 1 #35341
Conversation
@wonder-sk , this is wonderful! I have a fair amount of thoughts on the styling front, I'll write more tomorrow. But the general line being, IMHO we need to be trying to match tile styling as much as we can with that of non tiled vector layers. And we need to make sure some of the basic QGIS environment expectations, such as feature based data defined properties, are met. Great work! And thanks to the funders |
{ | ||
if ( tmpPoints.size() > 0 ) | ||
{ | ||
outputLinestrings.append( new QgsLineString( tmpPoints ) ); |
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.
Note that if you're after maximum speed, the fastest like string constructor is the one which accepts seperate double vectors for X and y
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.
Good point... but from some timing test I did the decoding phase is quite fast - it is actually evaluation of filter expressions that's the slowest bit. If you don't mind I would leave this unchanged. (+ I find that separation of vectors for X and Y in QgsLineString is very strange - it should be more cache friendly if they were together, and just one array instead of two.)
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.
Mmm possibly? But it'd need to be an array of doubles and not QgsPoint (don't want to store all those unused z/m values). And then you'd need to upgrade all the existing performance sensitive code which uses the two vector constructor... 😜
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.
Yeah an array of doubles is what I had in mind too... and yes, it would be quite some work to get all that converted... maybe one day :-)
Will there be data providers for vector tiles? |
I'm curious to know if there any visible artefacts under these lines - does the antialiasing of the clip rectangles cause visible edge effects? |
I'd also tend towards conversion vs custom renderer |
Thanks for the review @nyalldawson Regarding colors - I was unsure how set up default renderer. In theory we would want to show each sub-layer with a different color, but given that we may not even know all possible sub-layer names, I thought it would be best to use the same color for all layers. For the particular choice of default colors - I will be happy with any better colors people suggest :-)
I did not need the concept of data providers so far, so I have not introduced them... It would make most sense to create data providers when we know what kind of abstraction do we need.
I have not seen any artifacts so far! I was worried about that initially, but it seems that Qt is doing very good job with clipping (and I tried it also with reprojection when tile boundaries were not aligned with axes). |
Could you call QgsColorSchemeRegistry::fetchRandomStyleColor() ? that should give one not-random-but-pretend-random (nice) color per call.
Failing all else, I'd pick the first color from: QgsColorSchemeRegistry::randomStyleColorScheme()
Great to hear! I was concerned given the long-standing issue/limitation regarding neighbouring polygon bounds showing through due to antialiasing of the edges. (Maybe clipping paths completely skip the antialiasing on the boundary?) |
c37b235
to
8d24e17
Compare
I think this PR should be ready for merging... |
@@ -123,6 +123,15 @@ Sets renderer for the map layer. | |||
QgsVectorTileRenderer *renderer() const; | |||
%Docstring | |||
Returns currently assigned renderer | |||
%End | |||
|
|||
void setTileBorderRenderingEnabled( bool enabled ); |
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.
Wouldn't this be better as a render context flag?
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.
So it would be a global flag applied to all layers? And probably to raster tiles as well?
I don't know... I would probably prefer to keep it as is, maybe it can be moved to render context later. There are few more things like having tile XYZ numbers printed or border line/color configuration that would be nice to have too, but not sure if they all belong to render context.
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.
Yeah, it just struck me that it applies to other layers/providers too so would be nice to have something less ad-hoc to handle it. Could we hide from sip at least so it's not stable API?
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.
Hmm... removing it from sip would kind of remove the whole point of having it in API at all - I wanted to be able to turn this flag on/off from python console. I would be also fine with removal of this flag before the release if you don't like it, but now it is helping me with development... :-)
As a side note - it would be probably reasonable to mark vector tile layer related stuff as experimental API for now and pronounce it stable e.g. for 3.16 ...?
+1 to merge after above comments are resolved |
Oh, you'll also need to get the azure builds fixed before merging! |
Co-Authored-By: Nyall Dawson <nyall.dawson@gmail.com>
It turns out different versions of protobuf library may not necessarily work with different versions of code generated by protoc (for example, these files worked fine on Ubuntu 18.04 but they do not work on 19.10 which has newer version of protobuf library). Also, we should be fine with using just "lite" version of the library which is about an order of magnitude smaller (lite 0.3mb vs full 2.3mb).
Hmmmm...I now get this error when I try to compile (even from a fresh build dir): |
It may be in a separate package, e.g. on Ubuntu there is protobuf-compiler
This needs protobuf in osgeo4w to be compiled as a dynamic library. Right now it is built as a static library and the build fails on Windows... |
- remove protobuf-devel from explicit list - should be included in deps - only use the extra #define where needed - disable vector tile test on azure for now (can't debug it)
This is the initial work on vector tile layer support 🎉
The implementation is based on what has been previously discussed in Vector Tiles QEP:
qgis/QGIS-Enhancement-Proposals#162
(The red lines are not rendering artifacts, they are drawn intentionally to show tile borders.)
Summary
So far this only includes changes to qgis_core library.
Main additions to the public API:
New private classes (may be added to public API if needed):
Some new classes that may be shared by vector and raster tiles:
(also QgsTileMatrixSet to be added later when we support other tile matrix sets than just GoogleCRS84Quad)
Testing
You can load a vector tile layer from Python console, e.g.:
It is also possible to load vector tiles from a local MBTiles file - for "type" use "mbtiles" and for "url" use a local path (e.g. "/home/martin/x.mbtiles").
Rendering / Styling
We will probably need to have a discussion how the default renderer should work (I call it "basic"). It is currently based on rules ("styles") that refer to sub-layer name, geometry type and filter expression, and they have a QgsSymbol assigned. This is similar to what other rendering engines support. In theory in QGIS we could do more and support any vector renderer (categorized/graduated/rule-based/...) but for now I have kept things simple.
The other consideration is how to deal with existing styles - at this point most existing styles are done using Mapbox GL JSON style format. It is a question whether we should have a custom tile renderer which would try to emulate the native rendering as much as possible -- or to just have an importer that would convert those styles to our "basic" renderer (likely having somewhat lower fidelity). I have first started with a custom renderer, but over time I am thinking that using our "basic" renderer everywhere would be better also for the future.
Next Steps
Once this PR is merged, there will be more PRs coming to integrate vector tile layers into QGIS application: browser integration, styling GUI, identify tool support, attribute table.
Also there is currently no support for labeling, which should be added as well.
Thanks
Huge thanks to all funders who have contributed to the crowdfunding and made this possible 👏 👏 👏
https://www.lutraconsulting.co.uk/blog/2020/04/02/vectortiles-donors/