qgis / QGIS-Enhancement-Proposals

QEP's (QGIS Enhancement Proposals) are used in the process of creating and discussing new enhancements for QGIS
118 stars 37 forks source link

Allow QgsVectorDataProviders to create QgsFeatureRenderers #111

Closed nyalldawson closed 4 years ago

nyalldawson commented 6 years ago

QGIS Enhancement: Allow QgsVectorDataProviders to create QgsFeatureRenderers

Date 2018-02-027 Author Nyall Dawson (@nyalldawson)

Contact nyall dot dawson at gmail dot com

maintainer @nyalldawson

Version QGIS 3.2

Summary

Currently, QGIS has support for saving and restoring QML based layer styles to a vector data provider via the saveStyle and loadStyle library hooks. These methods work well for storage and retrieval of user created QGIS styles into the data provider backends, and for retrieval of default layer styles.

These methods require an existing QML document string, based on predefined QGIS layer styles. This is often not available from the data providers, which will utilise a wide variety of different style formats and standards. E.g. an ArcGIS REST layer (accessible through the afs QGIS provider) makes layer styles available through a JSON format response, encoded within the "drawingInfo" value.

This proposal covers suggested changes to the QgsVectorDataProvider API to allow creation of QgsFeatureRenderers directly by the data provider, allowing data providers to create vector layer renderers utilising backend-specific styling information.

Adding this capability will dramatically increase the usability of some QGIS vector data providers, where substantial styling rules have been put into place by an external data provider and which currently require users to manually "recreate" within QGIS symbology (e.g. layers provided by ArcGIS Feature Servers with complex categorized symbology containing many classes).

Proposed Solution

The QgsVectorDataProvider base class has the following API additions:

QgsVectorDataProvider::Capability gains a new flag CreateRenderer ("Provider can create feature renderers using backend-specific formatting information").

A new virtual method is added:

/**
 * Creates a new vector layer feature renderer, using provider backend specific information.
 *
 * The \a configuration map can be used to pass provider-specific configuration maps to the provider to
 * allow customisation of the returned renderer. Support and format of \a configuration varies by provider.
 *
 * When called with an empty \a configuration map the provider's default renderer will be returned.
 *
 * This method returns a new renderer and the caller takes ownership of the returned object. 
 *
 * Only providers which report the CreateRenderer capability will return a feature renderer. Other
 * providers will return nullptr.
 *  
 * \since QGIS 3.2
 */
virtual QgsFeatureRenderer* createRenderer( const QVariantMap& configuration = QVariantMap() ) const SIP_FACTORY
{
    return nullptr;
}

Then, QgsVectorLayer::setDataSource will be modified so that after setting the layer's data provider, and BEFORE the call to loadDefaultStyle(), the provider's capabilities will be checked for the CreateRenderer capability and, if found, the vector layer's renderer will be set to a new renderer obtained from a call to the providers createRenderer() implementation.

Following this, the existing logic for loading the default layer style (i.e. call to loadDefaultStyle() ) will remain -- this will allow users to override the default provider renderer by setting a new default style to use for layers from the provider.

Provider support

Initially, only the ArcGIS Feature Server provider will be upgraded to handle this capability, through parsing of the JSON drawInfo value and conversion to the corresponding QGIS feature renderer.

In future other providers could have support for this added, e.g. via the OGR feature style specification.

Affected Files

QgsVectorDataProvider QgsVectorLayer QgsAfsProvider

Performance Implications

Minimal - the provider renderers are only created on layer creation, which is an infrequent operation.

Backwards Compatibility

N/A

Issue Tracking ID(s)

https://issues.qgis.org/issues/13349

Votes

(required)

luipir commented 6 years ago

I'm trying to understand the interoperability effects of such modification. As far as I can see this solution delegates to the provider the task to convert his default renderer style definition in a QGIS renderer. Seems your proposed api is only in one way... make sense to: 1) subclass renderer witth the specific styling creator/exporter 2) exporter would allow to gets the spcific renderer options and to return to the original syling information.

just my 2c

nyalldawson commented 6 years ago

@luipir

As far as I can see this solution delegates to the provider the task to convert his default renderer style definition in a QGIS renderer. Seems your proposed api is only in one way

Correct.

I'm sorry, but I don't understand your changes, e.g. why we would subclass QgsFeatureRenderer. Can you please elaborate?

luipir commented 6 years ago

well... any tech solution to separate qgis specific renderer options from that derived from the provider. Eg. in case a user modify some renderer parameters, for example adding some qgis specific, should be available a way to separate the original part from the new parameters.

nyalldawson commented 6 years ago

well... any tech solution to separate qgis specific renderer options from that derived from the provider. Eg. in case a user modify some renderer parameters, for example adding some qgis specific, should be available a way to separate the original part from the new parameters.

Actually createRenderer() always returns a new object, and doesn't keep a reference to it. So if the user edits the renderer than their changes are only applied to the QgsVectorLayer, and don't have any impact on the provider itself. The next call to createRenderer() will return another new instance of the provider's renderer without any of the user's changes they've made to the vector layer styling.

m-kuhn commented 6 years ago

Nice idea, sounds good to me.

I've got just a minor doubt about the API, why are there two virtual methods? Under what circumstances will provider.createDefaultRenderer() return something different than provider.createRenderer( QVariantMap() )?

nyalldawson commented 6 years ago

I've got just a minor doubt about the API, why are there two virtual methods? Under what circumstances will provider.createDefaultRenderer() return something different than provider.createRenderer( QVariantMap() )?

Mostly for flexibility. I was thinking if a backend had multiple different styles available for a layer, and that these were accessible through the createRenderer implementation, then it's possible that a provider may want to just pass some configuration map to its createRenderer implementation to retrieve the default layer style. E.g. something like:

QgsFeatureRenderer* createDefaultRenderer() const override
{
    QVariantMap config;
    config.insert("theme", "default" );
    return createRenderer( config );
} 

It's not strictly necessary, and we could potentially remove the two methods and instead just call createRenderer with an empty map when we want the default renderer (and leave it to the provider itself to treat an empty map as 'get the default style').

m-kuhn commented 6 years ago

Could we go for the "simple" approach first?

virtual QgsFeatureRenderer* createRenderer( const QVariantMap& configuration = QVariantMap() ) const SIP_FACTORY

if I don't miss anything we can add the other one any time in the future when we have a real world example where it's required and in the meantime we stick to a slicker API.

nyalldawson commented 6 years ago

Could we go for the "simple" approach first?

Done - I've updated the original proposal to reflect this.

alexbruy commented 6 years ago

Nice. That would be a great addition, there are some vector formats which also can benefit from this feature.

rduivenvoorde commented 6 years ago

Do I understand correct that this gives way to create 'native style/painting implementations'? So no more translations from one style-model to another (QGIS) style model?

Just brainwaving here: could this for example also be used to create a native 'SldRenderer' (if somebody would be interested in implementing all the sld magic)? And if so, could this Renderer then be used both in a OGR/WFS provider and in other providers? Or is the provider/renderer really glued together?

Another interesting topic: what about for example Vector tiles and for example the Mapbox json style spec? Would be cool if such a renderer could also be used for... dunno other vector layers. Which makes me ask: what is the difference between the old QgsFeatureRenderer and your proposal? Or is the actual difference mostly that these 'provider' renderers are specific to the renderer (so falsifying my re-use-usecase above...).

nyalldawson commented 6 years ago

@rduivenvoorde

Do I understand correct that this gives way to create 'native style/painting implementations'? So no more translations from one style-model to another (QGIS) style model?

No, that's not the case. It's a translation from some styling information available by the backend to the native internal QGIS renderer API (i.e. not via sld or other intermediate formats).

Just brainwaving here: could this for example also be used to create a native 'SldRenderer' (if somebody would be interested in implementing all the sld magic)? And if so, could this Renderer then be used both in a OGR/WFS provider and in other providers? Or is the provider/renderer really glued together?

Again, that's not what's being proposed here -- there's absolutely no changes to the existing renderers. What is being proposed here is a way for a provider to specify the default styling a layer should receive when it's loaded into QGIS.

Here's an example. If I currently load the AFS layer https://gisservices.scc.qld.gov.au/arcgis/rest/services/PlanningCadastre/PlanningScheme_SunshineCoast_Zoning_SCC/MapServer/5 into QGIS, it's assigned a default single-symbol random color fill:

image

With these changes, when I load the layer it is automatically styled with a categorized renderer matching the categories and symbol styles set on the server for that layer:

image

It's still just a plain old QGIS categorized renderer -- exactly the same as if I'd manually set the layer to categorized and recreated the same categories and colors by hand. It's just a short cut to avoid having to do this when the styling information is available from the server.

Another interesting topic: what about for example Vector tiles and for example the Mapbox json style spec? Would be cool if such a renderer could also be used for... dunno other vector layers.

What this proposal does do is open the way for providers with knowledge of styles to set these as the defaults. So yes, potentially if the OGR provider could interpret the vector tiles style spec and translate it to QGIS renderers then we could have automatic styling of vector tiles on load.

Same with geopackage -- we could utilise the geopackage extensions which allow storage of SLD styling and automatically style newly loaded geopackage layers with these styles.

Which makes me ask: what is the difference between the old QgsFeatureRenderer and your proposal?

Nothing -- there's no change at all to the renderers.

Or is the actual difference mostly that these 'provider' renderers are specific to the renderer (so falsifying my re-use-usecase above...).

Again, there's no new renderers being introduced here, or any provider-specific renderers.

Hope that clears things up!

nyalldawson commented 6 years ago

https://github.com/qgis/QGIS/pull/6495 implements the proposed changes.

NathanW2 commented 6 years ago

Massive +1 on this one for me.

On Thu, Mar 1, 2018 at 1:24 PM, Nyall Dawson notifications@github.com wrote:

qgis/QGIS#6495 https://github.com/qgis/QGIS/pull/6495 implements the proposed changes.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/qgis/QGIS-Enhancement-Proposals/issues/111#issuecomment-369462330, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXS3LE2B6gNWE6KjgXiWYdGBWbzUQDLks5tZ2nUgaJpZM4SUEmb .

rduivenvoorde commented 6 years ago

@nyalldawson thanks Nyall for taking the time to answer my questions (and the pics)!

I see why it is usefull for providers which mix data+style info, but I fail to understand why this is different from existing providers which open a dataset and then create a renderer based on data IN (like postgis en gpkg) or NEXT to it (like a qml file with the same name as the dataset).

Is this only for convenience? Isn't this 'glueing' together the 'parsing of some styling info doc' into the provider (looking at qgsarcgisrestutils.cpp)? Would it not been cleaner to have these 'parsers'/renderer-creators in separate classes? So other providers can also benefit from this? (and I understand that it will never work for other providers off course to use this specific parser, but more looking in general).

I think these 'style-model' to 'qgis-style-model' machinery is very important for compatibility between QGIS and other tools, so deserve an important place in code tree (I 've seen people fighting the sld-parser code, while yours here is nice and clean :-) ).

But feel free to ignore my bla bla :)

m-kuhn commented 6 years ago

As far as I can understand, this only introduces the possibility for providers to cleanly expose default styles.

How they create these styles is a different question. This might very well be code shared between different providers (let's suppose various providers use sld for this, they will certainly share the code that produces the default renderer)

nyalldawson commented 6 years ago

@m-kuhn's interpretation here is correct

I see why it is usefull for providers which mix data+style info, but I fail to understand why this is different from existing providers which open a dataset and then create a renderer based on data IN (like postgis en gpkg) or NEXT to it (like a qml file with the same name as the dataset).

Because in those cases the styles all still have to exist in QGIS specific style formats (i.e. qml). It's just either qml stored as a file next to the source OR qml stored as text in a database row.

So the difference here is that it exposes a way for non-QML styling to be applied by default. In other words, allows qgis to utilise other style standards apart from our own format.

I'd personally love to see someone utilise the changes proposed here to automatically load SLD styles contained within a geopackage source when loading those layers! (This PR is a pre-requisite for that kind of thing)

elpaso commented 6 years ago

Big +1 from me too, I think this open a whole lot of new opportunities and increases flexibility for new styling formats.

nyalldawson commented 6 years ago

What's the consensus here? Is everyone happy enough about this proposal to accept it and I can continue with https://github.com/qgis/QGIS/pull/6495?

NathanW2 commented 6 years ago

+1 from me.

On Mon, Mar 5, 2018 at 10:49 AM, Nyall Dawson notifications@github.com wrote:

What's the consensus here? Is everyone happy enough about this proposal to accept it and I can continue with qgis/QGIS#6495 https://github.com/qgis/QGIS/pull/6495?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/qgis/QGIS-Enhancement-Proposals/issues/111#issuecomment-370280926, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXS3OO0jXqfLUlX_gL6Tf8r69woHRPvks5tbIu3gaJpZM4SUEmb .

pcav commented 6 years ago

+1 Thanks

luipir commented 6 years ago

+1 :)

wonder-sk commented 6 years ago

Let me add my +1 too - looking good!

nyalldawson commented 6 years ago

Accepted and PR merged :) Thanks all!