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

[Server] WMS getmap refactoring #4313

Merged
merged 3 commits into from Jun 1, 2017

Conversation

Projects
None yet
10 participants
@pblottiere
Member

pblottiere commented Mar 29, 2017

Description

The aim of this PR is to propose a cleaner version of the WMS getmap function defined in the server:

  • QgsProject singleton is not used anymore
  • XML parsers are not used either
  • WMS parameters are centralised in a new class QgsWmsParameters
  • clones methods for raster and vector layers are implemented. Thus, getmap is working on clones only, so it's completely stateless and there's no need for restorers (like QgsOWSServerFilterRestorer)

Obviously the final aim is to completely remove XML parsers (QgsWmsConfigParser, QgsWmsProjectParser, QgsSLDConfigParser, ...), restorers, ... but the same kind of work has to be done on getprint and getlegendgraphic requests. So a getMapOld function is still there to keep the getlegendgraphic function running. Moreover, the new class QgsWmsParameters should be used in these functions too.

Some new units tests have been added for SLD, hilight layers, rasters, ...

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Mar 29, 2017

Contributor

Yes !

Contributor

dmarteau commented Mar 29, 2017

Yes !

@rldhont rldhont requested review from elpaso, mhugent and rldhont Mar 29, 2017

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Mar 29, 2017

Contributor

Great stuff - I'm so happy to see that those fragile project parsers are on their way out!

Not sure if you saw the changes which landed in #4279, but these should make porting getprint much easier.

Contributor

nyalldawson commented Mar 29, 2017

Great stuff - I'm so happy to see that those fragile project parsers are on their way out!

Not sure if you saw the changes which landed in #4279, but these should make porting getprint much easier.

@rldhont rldhont referenced this pull request Mar 30, 2017

Open

QEP 74: QGIS server code refactoring for QGIS 3.0 #74

9 of 9 tasks complete
@elpaso

elpaso approved these changes Mar 31, 2017

This is a great contribution, thanks!
I just added some minor notes, please have a look.

Show outdated Hide outdated src/server/qgsconfigcache.cpp
@@ -0,0 +1,240 @@
/***************************************************************************

This comment has been minimized.

@elpaso

elpaso Mar 31, 2017

Contributor

Nice! Just wondering if this restricts in any way the addition of new vendor parameters from the plugins.

@elpaso

elpaso Mar 31, 2017

Contributor

Nice! Just wondering if this restricts in any way the addition of new vendor parameters from the plugins.

This comment has been minimized.

@pblottiere

pblottiere Apr 3, 2017

Member

Mmmm no I don't think so.

@pblottiere

pblottiere Apr 3, 2017

Member

Mmmm no I don't think so.

Show outdated Hide outdated tests/src/python/test_qgsserver.py
Show outdated Hide outdated tests/src/python/test_qgsserver.py
@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 2, 2017

Member

@pblottiere hey, about the clone() methods, would it be possible to just obtain a QgsAbstractFeatureSource? This has been created for a very similar purpose with multithreading I think and the approach seems to be much less invasive and connected with less cases that are hard to handle. @wonder-sk has a very good understanding of this topic.

Member

m-kuhn commented Apr 2, 2017

@pblottiere hey, about the clone() methods, would it be possible to just obtain a QgsAbstractFeatureSource? This has been created for a very similar purpose with multithreading I think and the approach seems to be much less invasive and connected with less cases that are hard to handle. @wonder-sk has a very good understanding of this topic.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 3, 2017

Member

Thank you @nyalldawson, I took into account your comments!

Member

pblottiere commented Apr 3, 2017

Thank you @nyalldawson, I took into account your comments!

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 4, 2017

Member

@rldhont I updated QgsWmsParameters to be more generic. Moreover, I improved the error management in case of invalid parameters. There's a bunch of new unit tests checking that a good message is raised in case of a bad parameter.

Member

pblottiere commented Apr 4, 2017

@rldhont I updated QgsWmsParameters to be more generic. Moreover, I improved the error management in case of invalid parameters. There's a bunch of new unit tests checking that a good message is raised in case of a bad parameter.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Apr 4, 2017

Contributor

@pblottiere thanks

Contributor

rldhont commented Apr 4, 2017

@pblottiere thanks

@rldhont

rldhont approved these changes Apr 4, 2017

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Apr 4, 2017

Contributor

It seems that spellcheck dislikes colour

Contributor

rldhont commented Apr 4, 2017

It seems that spellcheck dislikes colour

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 4, 2017

Member

It seems that spellcheck dislikes colour

Yes. And colour is a column's name coming from the helloworld.db database which is basically used in every tests on qgis server... I'm gonna take a look.

Member

pblottiere commented Apr 4, 2017

It seems that spellcheck dislikes colour

Yes. And colour is a column's name coming from the helloworld.db database which is basically used in every tests on qgis server... I'm gonna take a look.

@mhugo

This comment has been minimized.

Show comment
Hide comment
@mhugo

mhugo Apr 4, 2017

Contributor

@m-kuhn Hi. About QgsAbstractFeatureSource I am not sure to understand what you have in mind. If I'm correct, the rendering part of the server is not only about accessing features but unfortunately it needs to modify layers before rendering them. This was done prior to this PR by changing original layers and restoring their states after rendering.
We thought it would be better to have a stateless rendering. Thus the ability to "copy" layers.

Ideally all the rendering pipeline should be stateless and we might have something like a "layersettings", close to the current "mapsettings". But not for this PR yet ...

Contributor

mhugo commented Apr 4, 2017

@m-kuhn Hi. About QgsAbstractFeatureSource I am not sure to understand what you have in mind. If I'm correct, the rendering part of the server is not only about accessing features but unfortunately it needs to modify layers before rendering them. This was done prior to this PR by changing original layers and restoring their states after rendering.
We thought it would be better to have a stateless rendering. Thus the ability to "copy" layers.

Ideally all the rendering pipeline should be stateless and we might have something like a "layersettings", close to the current "mapsettings". But not for this PR yet ...

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 4, 2017

Member

@mhugo, yeah, a stateless rendering engine would be awesome. can you estimate what's missing for that?

mapsettings + feature source + some parts of map themes...?

Ideally all the rendering pipeline should be stateless and we might have something like a "layersettings", close to the current "mapsettings". But not for this PR yet ...

is it worth it to introduce layer cloning for a temporary solution? it's not a blocker for me but it's a quite intrusive thing, so it should be very well considered.

Member

m-kuhn commented Apr 4, 2017

@mhugo, yeah, a stateless rendering engine would be awesome. can you estimate what's missing for that?

mapsettings + feature source + some parts of map themes...?

Ideally all the rendering pipeline should be stateless and we might have something like a "layersettings", close to the current "mapsettings". But not for this PR yet ...

is it worth it to introduce layer cloning for a temporary solution? it's not a blocker for me but it's a quite intrusive thing, so it should be very well considered.

@mhugo

This comment has been minimized.

Show comment
Hide comment
@mhugo

mhugo Apr 5, 2017

Contributor

@m-kuhn

a stateless rendering engine would be awesome. can you estimate what's missing for that?

From the WMS point of view, I think what is still missing is what we can find here:
https://github.com/qgis/QGIS/pull/4313/files#diff-daca8d457c025999b6bcf3273bb4fc13R850
opacity, filter, selection, etc. @pblottiere ?

is it worth it to introduce layer cloning for a temporary solution? it's not a blocker for me but it's a quite intrusive thing

By "intrusive" you mean that clone() must be implemented by raster + vector + plugin layers or is it something else ?

Contributor

mhugo commented Apr 5, 2017

@m-kuhn

a stateless rendering engine would be awesome. can you estimate what's missing for that?

From the WMS point of view, I think what is still missing is what we can find here:
https://github.com/qgis/QGIS/pull/4313/files#diff-daca8d457c025999b6bcf3273bb4fc13R850
opacity, filter, selection, etc. @pblottiere ?

is it worth it to introduce layer cloning for a temporary solution? it's not a blocker for me but it's a quite intrusive thing

By "intrusive" you mean that clone() must be implemented by raster + vector + plugin layers or is it something else ?

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Apr 5, 2017

Contributor

I think a map layer clone method is useful in general, eg to replace the current fragile code used with the duplicate layer action.

But it's also critical stuff which has to be done right - so I would not like to see any clone method merged without a good base of unit tests accompanying it.

Contributor

nyalldawson commented Apr 5, 2017

I think a map layer clone method is useful in general, eg to replace the current fragile code used with the duplicate layer action.

But it's also critical stuff which has to be done right - so I would not like to see any clone method merged without a good base of unit tests accompanying it.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 5, 2017

Member

I think a map layer clone method is useful in general, eg to replace the current fragile code used with the duplicate layer action.

+1

But it's also critical stuff which has to be done right - so I would not like to see any clone method merged without a good base of unit tests accompanying it.

Agree. I'm going to do that.

Member

pblottiere commented Apr 5, 2017

I think a map layer clone method is useful in general, eg to replace the current fragile code used with the duplicate layer action.

+1

But it's also critical stuff which has to be done right - so I would not like to see any clone method merged without a good base of unit tests accompanying it.

Agree. I'm going to do that.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 5, 2017

Member

By "intrusive" you mean that clone() must be implemented by raster + vector + plugin layers or is it something else ?

There are a lot of small things to take care of (and are hard to get right) like...

  • layer dependencies (copy or not?)
  • relations (copy or not?)
  • layer id (generate a new one -> it might be referenced in existing expressions; copy the current one -> it's no longer unique)
  • lookup tables for non-integer feature ids (they will probably have a partial overlap which might be slightly confusing, but that should be acceptable)
  • ...
Member

m-kuhn commented Apr 5, 2017

By "intrusive" you mean that clone() must be implemented by raster + vector + plugin layers or is it something else ?

There are a lot of small things to take care of (and are hard to get right) like...

  • layer dependencies (copy or not?)
  • relations (copy or not?)
  • layer id (generate a new one -> it might be referenced in existing expressions; copy the current one -> it's no longer unique)
  • lookup tables for non-integer feature ids (they will probably have a partial overlap which might be slightly confusing, but that should be acceptable)
  • ...
@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Apr 5, 2017

Contributor

There are a lot of small things to take care of (and are hard to get right) like...

Clone to me implies EVERYTHING (except layer id ;)

Contributor

nyalldawson commented Apr 5, 2017

There are a lot of small things to take care of (and are hard to get right) like...

Clone to me implies EVERYTHING (except layer id ;)

@mhugo

This comment has been minimized.

Show comment
Hide comment
@mhugo

mhugo Apr 5, 2017

Contributor

Yes, for me a clone recursively clones every member of the class (where clone = copy most of the time), but no more. We could add in the doc that the caller is responsible for managing references to this new layer.

Contributor

mhugo commented Apr 5, 2017

Yes, for me a clone recursively clones every member of the class (where clone = copy most of the time), but no more. We could add in the doc that the caller is responsible for managing references to this new layer.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 5, 2017

Member

E.g. relations?

Given

house.city -> city.id

Clone both layers:

house.city -> city.id
house_copy.city ->city.id
house.city -> city_copy.id
houe_copy.city -> city.id

Member

m-kuhn commented Apr 5, 2017

E.g. relations?

Given

house.city -> city.id

Clone both layers:

house.city -> city.id
house_copy.city ->city.id
house.city -> city_copy.id
houe_copy.city -> city.id

@rldhont rldhont added this to the QGIS 3 milestone Apr 8, 2017

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 10, 2017

Member

Is the clone() question sorted out meanwhile? I'm still a tiny bit confused and hope someone can enlighten me.

It sounds like a central clone() functionality for the desktop case (duplicate layer functionality) is fine with changed layer id and nothing w.r.t relations being done.
I think this is mainly justified because we can assume that the user will manually do any required follow-up corrections.

Does this translate 1:1 to the WxS case? Some scenarios that I don't completely understand how they will work:

  • get_feature(layer_id...) expressions (for labels, symbology etc)? will they be able to access the source (uncloned) layer by some means? Are the two cloned layers in the same QgsProject (which is the successor of QgsMapLayerRegistry)?
  • what happens to relations (or are they not interesting in this scenario?)
Member

m-kuhn commented Apr 10, 2017

Is the clone() question sorted out meanwhile? I'm still a tiny bit confused and hope someone can enlighten me.

It sounds like a central clone() functionality for the desktop case (duplicate layer functionality) is fine with changed layer id and nothing w.r.t relations being done.
I think this is mainly justified because we can assume that the user will manually do any required follow-up corrections.

Does this translate 1:1 to the WxS case? Some scenarios that I don't completely understand how they will work:

  • get_feature(layer_id...) expressions (for labels, symbology etc)? will they be able to access the source (uncloned) layer by some means? Are the two cloned layers in the same QgsProject (which is the successor of QgsMapLayerRegistry)?
  • what happens to relations (or are they not interesting in this scenario?)
@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 11, 2017

Member

It sounds like a central clone() functionality for the desktop case (duplicate layer functionality) is fine with changed layer id and nothing w.r.t relations being done.

I took a look on how to avoid the copy of the layer id but it was not a good idea for all reasons you mentionned (expressions and so on). So I kept the original behavior for the clone methods with a boolean to indicate if we want (or not) to clone the unique ID. And as you said, I think that for a desktop use, it won't be necessary to keep the id.

  • what happens to relations (or are they not interesting in this scenario?)

About relations, I think we have 2 scenarios:

  • the layer is cloned with a new id and if there are relations related to the original layer, the user has to decide what to do
  • the layer is fully cloned (the unique id too) and if there are relations related to the original layer, then there is indeed an ambiguity. The user is aware of this ambiguity and he has to manage in whatever way he wants.

On the server side, we just create a vector layer for the rendering step, then once the renderer has terminated its job, the layer is deleted. So there's no interaction with relations.

@m-kuhn What do you think? Am I missing something?

By the way, the default behavior is to clone the unique ID. Is it wise?

Member

pblottiere commented Apr 11, 2017

It sounds like a central clone() functionality for the desktop case (duplicate layer functionality) is fine with changed layer id and nothing w.r.t relations being done.

I took a look on how to avoid the copy of the layer id but it was not a good idea for all reasons you mentionned (expressions and so on). So I kept the original behavior for the clone methods with a boolean to indicate if we want (or not) to clone the unique ID. And as you said, I think that for a desktop use, it won't be necessary to keep the id.

  • what happens to relations (or are they not interesting in this scenario?)

About relations, I think we have 2 scenarios:

  • the layer is cloned with a new id and if there are relations related to the original layer, the user has to decide what to do
  • the layer is fully cloned (the unique id too) and if there are relations related to the original layer, then there is indeed an ambiguity. The user is aware of this ambiguity and he has to manage in whatever way he wants.

On the server side, we just create a vector layer for the rendering step, then once the renderer has terminated its job, the layer is deleted. So there's no interaction with relations.

@m-kuhn What do you think? Am I missing something?

By the way, the default behavior is to clone the unique ID. Is it wise?

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 11, 2017

Member

@nyalldawson I added some tests for the clone() methods as you suggested.

Member

pblottiere commented Apr 11, 2017

@nyalldawson I added some tests for the clone() methods as you suggested.

@rldhont rldhont referenced this pull request Apr 12, 2017

Merged

[Server] WFS refactoring and QgsWfsProjectParser removal #4344

4 of 6 tasks complete
@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 12, 2017

Member

I think we are pretty much on the same page concerning the critical points.

On the server side, we just create a vector layer for the rendering step, then once the renderer has terminated its job, the layer is deleted.

Once upon a time, multithreaded rendering had the same problem (multiple parallel threads access the same information that might disappear or be modified by other parts of the system). The approach was to introduce QgsFeatureSource to copy (only) relevant information for the rendering.

To me it seems strange that a different route is taken here. Am I just still missing the difference between the two scenarios server instances and desktop multithreading? Or should the rendering jobs be adapted to also work on layer copies instead of feature sources?

Member

m-kuhn commented Apr 12, 2017

I think we are pretty much on the same page concerning the critical points.

On the server side, we just create a vector layer for the rendering step, then once the renderer has terminated its job, the layer is deleted.

Once upon a time, multithreaded rendering had the same problem (multiple parallel threads access the same information that might disappear or be modified by other parts of the system). The approach was to introduce QgsFeatureSource to copy (only) relevant information for the rendering.

To me it seems strange that a different route is taken here. Am I just still missing the difference between the two scenarios server instances and desktop multithreading? Or should the rendering jobs be adapted to also work on layer copies instead of feature sources?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 12, 2017

Member

Actually, isn't all that already done in QgsVectorLayerRenderer? It clones the renderer() and all other information required for rendering.

Member

m-kuhn commented Apr 12, 2017

Actually, isn't all that already done in QgsVectorLayerRenderer? It clones the renderer() and all other information required for rendering.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Apr 13, 2017

Contributor

@pblottiere does not forget that rendering can be based on join. So joined layer can be cloned too.

Contributor

rldhont commented Apr 13, 2017

@pblottiere does not forget that rendering can be based on join. So joined layer can be cloned too.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Apr 13, 2017

Contributor

@m-kuhn so clone layer has been introduced because the server, for access control needs to update the datasource not just the rendering context. QgsOWSServerFilterRestorer has been created for and will have to be removed for a better QGIS Server. Is it right @pblottiere ?

Contributor

rldhont commented Apr 13, 2017

@m-kuhn so clone layer has been introduced because the server, for access control needs to update the datasource not just the rendering context. QgsOWSServerFilterRestorer has been created for and will have to be removed for a better QGIS Server. Is it right @pblottiere ?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 13, 2017

Member

For that bit, it should be migrated to modify the featureRequest.filterExpression instead of the subsetString.

Member

m-kuhn commented Apr 13, 2017

For that bit, it should be migrated to modify the featureRequest.filterExpression instead of the subsetString.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 13, 2017

Member

@m-kuhn

I think we are pretty much on the same page concerning the critical points.

OK, it's a good start :)

The approach was to introduce QgsFeatureSource to copy (only) relevant information for the rendering.

In fact, as you explained, in the case of the multithreading problem, the idea is to manage a parallel access to the same data. But from the server point of view, the idea is just to avoid some side effects
between 2 successive requests. Moreover, from what I saw in QgsFeatureSource, it clones only information about join buffer, feature ids and so on. But in our case, interesting parameters are transparency (or opacity for raster layers), filters, selection, ...

Let's talk about a concrete example without cloning. A first request is coming from the client which changes the original layer filter. The corresponding project is opened and cached. We configure the layer coming from the project with the filter given in parameters and the rendering is achieved. Then a second request is coming and changes the original transparency. But as we are working on the same instance of the layer (as the project is cached), the previous filter of the first request is still applied to the layer. Here is the crux of the problem and its true for all parameters.

If you take a look to the original getmap method, you'll see some restorers. In fact, they keep in memory the original value for the opacity (for example). Then the layer is configured with the new opacity given in parameter. Once the renderer has terminated its job, the restorer reapplies the original opacity. But the same mechanism is necessary for each parameter (transparency or opacity for raster layer, filters, selection, ...) and for each layer. In my opinion, it's a little risky and brings an unnecessary complexity (even if the solution of QgsOWSServerFilterRestorer stored in a unique ptr to restore filter during the destruction is pretty elegant). As I wanted to get rid of all these restorers, I introduced the clone capacity. But if you think it's a bad idea, I can still keep the previous mechanism with restorers. @rldhont @mhugo @elpaso what do you think about it?

By the way, I think that the best option (as previously mentioned) would be to have a QgsLayerSettings as we have a QgsMapSettings. It would avoid to store rendering information within the layer itself. But as we wanted to concentrate on the server development, we choose a quickest route (clone clayers).

Member

pblottiere commented Apr 13, 2017

@m-kuhn

I think we are pretty much on the same page concerning the critical points.

OK, it's a good start :)

The approach was to introduce QgsFeatureSource to copy (only) relevant information for the rendering.

In fact, as you explained, in the case of the multithreading problem, the idea is to manage a parallel access to the same data. But from the server point of view, the idea is just to avoid some side effects
between 2 successive requests. Moreover, from what I saw in QgsFeatureSource, it clones only information about join buffer, feature ids and so on. But in our case, interesting parameters are transparency (or opacity for raster layers), filters, selection, ...

Let's talk about a concrete example without cloning. A first request is coming from the client which changes the original layer filter. The corresponding project is opened and cached. We configure the layer coming from the project with the filter given in parameters and the rendering is achieved. Then a second request is coming and changes the original transparency. But as we are working on the same instance of the layer (as the project is cached), the previous filter of the first request is still applied to the layer. Here is the crux of the problem and its true for all parameters.

If you take a look to the original getmap method, you'll see some restorers. In fact, they keep in memory the original value for the opacity (for example). Then the layer is configured with the new opacity given in parameter. Once the renderer has terminated its job, the restorer reapplies the original opacity. But the same mechanism is necessary for each parameter (transparency or opacity for raster layer, filters, selection, ...) and for each layer. In my opinion, it's a little risky and brings an unnecessary complexity (even if the solution of QgsOWSServerFilterRestorer stored in a unique ptr to restore filter during the destruction is pretty elegant). As I wanted to get rid of all these restorers, I introduced the clone capacity. But if you think it's a bad idea, I can still keep the previous mechanism with restorers. @rldhont @mhugo @elpaso what do you think about it?

By the way, I think that the best option (as previously mentioned) would be to have a QgsLayerSettings as we have a QgsMapSettings. It would avoid to store rendering information within the layer itself. But as we wanted to concentrate on the server development, we choose a quickest route (clone clayers).

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 13, 2017

Member

But in our case, interesting parameters are transparency (or opacity for raster layers), filters, selection, ...

Isn't that done in QgsVectorLayerRenderer?

Except for the filter - which really IS a mess with the subsetString and should be migrated to featureRequest.filterExpression.

Member

m-kuhn commented Apr 13, 2017

But in our case, interesting parameters are transparency (or opacity for raster layers), filters, selection, ...

Isn't that done in QgsVectorLayerRenderer?

Except for the filter - which really IS a mess with the subsetString and should be migrated to featureRequest.filterExpression.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Apr 13, 2017

Contributor

@m-kuhn

For that bit, it should be migrated to modify the featureRequest.filterExpression instead of the subsetString.

For that point, we need to enhance the expression compiler to cover the same function and have the same capabilities.
I have started the work in #3863 but some cases are not well compiled like pi() / 2 and the same work has to be done for all providers.

Contributor

rldhont commented Apr 13, 2017

@m-kuhn

For that bit, it should be migrated to modify the featureRequest.filterExpression instead of the subsetString.

For that point, we need to enhance the expression compiler to cover the same function and have the same capabilities.
I have started the work in #3863 but some cases are not well compiled like pi() / 2 and the same work has to be done for all providers.

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Apr 13, 2017

Contributor

@rldhont that division compilation should be fixed since a while ago...

Contributor

nyalldawson commented Apr 13, 2017

@rldhont that division compilation should be fixed since a while ago...

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 13, 2017

Member

For that point, we need to enhance the expression compiler to cover the same function and have the same capabilities.

There's some missing functionality which can be improved and some that will never be possible. I don't consider this a reason for the design choice here.

In any case we could copy the subsetString into QgsVectorLayerRenderer if that's really required.

Member

m-kuhn commented Apr 13, 2017

For that point, we need to enhance the expression compiler to cover the same function and have the same capabilities.

There's some missing functionality which can be improved and some that will never be possible. I don't consider this a reason for the design choice here.

In any case we could copy the subsetString into QgsVectorLayerRenderer if that's really required.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Apr 13, 2017

Contributor

@nyalldawson thanks for that

@rldhont that division compilation should be fixed since a while ago...

Contributor

rldhont commented Apr 13, 2017

@nyalldawson thanks for that

@rldhont that division compilation should be fixed since a while ago...

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 13, 2017

Member

Isn't that done in QgsVectorLayerRenderer?

From what I see, the transparency is for example not managed in QgsVectorLayerRenderer. The QgsMapRendererJob directly gets the transparency from the vector layer given through QgsMapSettings.

Member

pblottiere commented Apr 13, 2017

Isn't that done in QgsVectorLayerRenderer?

From what I see, the transparency is for example not managed in QgsVectorLayerRenderer. The QgsMapRendererJob directly gets the transparency from the vector layer given through QgsMapSettings.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 13, 2017

Member

Isn't that done in QgsVectorLayerRenderer?

Moreover, the QgsVectorLayerRenderer takes an instance of a QgsVectorLayer in the constructor.

So I'd have to configure the vector layer with transparency and so on before passing it to the QgsVectorLayerRenderer. But I'd still have to restore the original parameters of my vector layer once the rendering terminated to avoid side effects between requests.

Member

pblottiere commented Apr 13, 2017

Isn't that done in QgsVectorLayerRenderer?

Moreover, the QgsVectorLayerRenderer takes an instance of a QgsVectorLayer in the constructor.

So I'd have to configure the vector layer with transparency and so on before passing it to the QgsVectorLayerRenderer. But I'd still have to restore the original parameters of my vector layer once the rendering terminated to avoid side effects between requests.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 13, 2017

Member

@rldhont

does not forget that rendering can be based on join. So joined layer can be cloned too.

If the clone mechanism is kept, I'll took a look to add a test for this case.

Member

pblottiere commented Apr 13, 2017

@rldhont

does not forget that rendering can be based on join. So joined layer can be cloned too.

If the clone mechanism is kept, I'll took a look to add a test for this case.

@mhugo

This comment has been minimized.

Show comment
Hide comment
@mhugo

mhugo Apr 13, 2017

Contributor

But in our case, interesting parameters are transparency (or opacity for raster layers), filters, selection, ...

Isn't that done in QgsVectorLayerRenderer?

Yes, it is done to handle concurrent rendering. But it does not allow to render a layer with a specific (transparency | filter | selection | SLD ) without modifying the layer beforehand

The possibilities we saw were:

  1. modify in-place the layer before rendering and restore its state after rendering using "restorers". It works but it's not very clean and won't allow to share a project cache between server instances (not there yet, but discussed)
  2. modify all the layer / rendering classes so that its "stateless", meaning vector layers only store data and a new class store layer rendering settings. Major refactor that we could not afford yet.
  3. make layers clonable

I don't see much bad impacts of 3. But I do realize now that there are some code duplication between QgsVectorLayerRenderer and clone. Maybe QgsVectorLayerRender can reuse the clone ?

Except for the filter - which really IS a mess with the subsetString and should be migrated to featureRequest.filterExpression.

For instance by using renderJob.setFeatureFilterProvider ?

Contributor

mhugo commented Apr 13, 2017

But in our case, interesting parameters are transparency (or opacity for raster layers), filters, selection, ...

Isn't that done in QgsVectorLayerRenderer?

Yes, it is done to handle concurrent rendering. But it does not allow to render a layer with a specific (transparency | filter | selection | SLD ) without modifying the layer beforehand

The possibilities we saw were:

  1. modify in-place the layer before rendering and restore its state after rendering using "restorers". It works but it's not very clean and won't allow to share a project cache between server instances (not there yet, but discussed)
  2. modify all the layer / rendering classes so that its "stateless", meaning vector layers only store data and a new class store layer rendering settings. Major refactor that we could not afford yet.
  3. make layers clonable

I don't see much bad impacts of 3. But I do realize now that there are some code duplication between QgsVectorLayerRenderer and clone. Maybe QgsVectorLayerRender can reuse the clone ?

Except for the filter - which really IS a mess with the subsetString and should be migrated to featureRequest.filterExpression.

For instance by using renderJob.setFeatureFilterProvider ?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 20, 2017

Member

Thanks for all the comments!

From what I see, the transparency is for example not managed in QgsVectorLayerRenderer. The QgsMapRendererJob directly gets the transparency from the vector layer given through QgsMapSettings.

That should probably be copied into QgsVectorLayerRenderer like e.g. the blend mode. Or to get them from the QgsMapSettings which IIRC are also copied somewhere for the rendering process.

So I'd have to configure the vector layer with transparency and so on before passing it to the QgsVectorLayerRenderer. But I'd still have to restore the original parameters of my vector layer once the rendering terminated to avoid side effects between requests.

If that all happens synchronized (on desktop that should be guaranteed because that always happens on the main thread), there should be no bad side-effects, the setup job should take very little time. The problem with restoring the state only really is a problem if the layer remains modified throughout the whole rendering process, no?

I don't see much bad impacts of 3. But I do realize now that there are some code duplication between QgsVectorLayerRenderer and clone. Maybe QgsVectorLayerRender can reuse the clone ?

I think we need to do a short historic trip for this one ;)

Originally, QgsVectorLayerRenderer was introduced by @wonder-sk as a clean approach for stateless rendering.

Later on, as part of the server filtering a pointer to the layer was copied into QgsVectorLayerRenderer, since that time, it's no longer strictly stateless. I think it would be a pity to further set this into stone by using clone and make a step further away from stateless rendering (which you mention yourself as a nice-to-have future plan). Instead I would very much welcome a move to remove the mLayer pointer again from QgsVectorLayerRenderer.


What I would propose to continue is

  • that you verify shortly how much really is involved in making copies of the missing bits in QgsVectorLayerRenderer or QgsMapSettings. My feeling told me that it shouldn't be much harder to copy the missing properties inside QgsVectorLayerRenderer than to copy all properties in QgsVectorLayer, but I start to doubt this feeling.
  • If it's not feasible with a reasonable amount of work, use the clone approach but mark it with a comment as a short-term workaround that should be replaced with stateless rendering in the near future, when you find time for this refactoring.

Does that sound good to you?

Member

m-kuhn commented Apr 20, 2017

Thanks for all the comments!

From what I see, the transparency is for example not managed in QgsVectorLayerRenderer. The QgsMapRendererJob directly gets the transparency from the vector layer given through QgsMapSettings.

That should probably be copied into QgsVectorLayerRenderer like e.g. the blend mode. Or to get them from the QgsMapSettings which IIRC are also copied somewhere for the rendering process.

So I'd have to configure the vector layer with transparency and so on before passing it to the QgsVectorLayerRenderer. But I'd still have to restore the original parameters of my vector layer once the rendering terminated to avoid side effects between requests.

If that all happens synchronized (on desktop that should be guaranteed because that always happens on the main thread), there should be no bad side-effects, the setup job should take very little time. The problem with restoring the state only really is a problem if the layer remains modified throughout the whole rendering process, no?

I don't see much bad impacts of 3. But I do realize now that there are some code duplication between QgsVectorLayerRenderer and clone. Maybe QgsVectorLayerRender can reuse the clone ?

I think we need to do a short historic trip for this one ;)

Originally, QgsVectorLayerRenderer was introduced by @wonder-sk as a clean approach for stateless rendering.

Later on, as part of the server filtering a pointer to the layer was copied into QgsVectorLayerRenderer, since that time, it's no longer strictly stateless. I think it would be a pity to further set this into stone by using clone and make a step further away from stateless rendering (which you mention yourself as a nice-to-have future plan). Instead I would very much welcome a move to remove the mLayer pointer again from QgsVectorLayerRenderer.


What I would propose to continue is

  • that you verify shortly how much really is involved in making copies of the missing bits in QgsVectorLayerRenderer or QgsMapSettings. My feeling told me that it shouldn't be much harder to copy the missing properties inside QgsVectorLayerRenderer than to copy all properties in QgsVectorLayer, but I start to doubt this feeling.
  • If it's not feasible with a reasonable amount of work, use the clone approach but mark it with a comment as a short-term workaround that should be replaced with stateless rendering in the near future, when you find time for this refactoring.

Does that sound good to you?

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Apr 20, 2017

Contributor

Just to add my recent thoughts... I've been watching the conversation here and I'm afraid @m-kuhn's arguments and the issue raised have convinced me against using a layer clone method here. To me it feels like there's going to be many corner cases and unforeseen issues (with relations and such) and it honestly feels like we'll be falling back into the dangerous server project parser trap once again, but instead of the xml parsing we'll be adding workarounds to layer cloning just to get this to function.

As clarification - I'm not against a layer cloning method (I still think it'd be useful in other cases, such as the duplicate layer function). I'm just not in favor of cloning layers as a prerequisite for rendering.

Contributor

nyalldawson commented Apr 20, 2017

Just to add my recent thoughts... I've been watching the conversation here and I'm afraid @m-kuhn's arguments and the issue raised have convinced me against using a layer clone method here. To me it feels like there's going to be many corner cases and unforeseen issues (with relations and such) and it honestly feels like we'll be falling back into the dangerous server project parser trap once again, but instead of the xml parsing we'll be adding workarounds to layer cloning just to get this to function.

As clarification - I'm not against a layer cloning method (I still think it'd be useful in other cases, such as the duplicate layer function). I'm just not in favor of cloning layers as a prerequisite for rendering.

@wonder-sk

This comment has been minimized.

Show comment
Hide comment
@wonder-sk

wonder-sk Apr 21, 2017

Member

I am with @m-kuhn and @nyalldawson here - the approach with cloning of layers does not feel right. I would rather like to see some way to tell QgsVectorLayerRenderer that some values should be overridden (e.g. using existing layer style overrides in QgsMapSettings (?)).

Member

wonder-sk commented Apr 21, 2017

I am with @m-kuhn and @nyalldawson here - the approach with cloning of layers does not feel right. I would rather like to see some way to tell QgsVectorLayerRenderer that some values should be overridden (e.g. using existing layer style overrides in QgsMapSettings (?)).

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 21, 2017

Member

At least it's unanimous :).

As I already lost a bit of time on copy constructors, then clones and its unit tests, I propose to:

  • remove everything about the clone methods from this PR (to avoid unforeseen issues)
  • reinstate the prior mechanism of Restorers

Then, we'll be able to improve/remove the restorers mechanism afterward by updating QgsVectorLayerRenderer, QgsMapSettings, .... But for now, I think the most urgent thing to do is to continue removing parsers in WMS (getlegendgraphic request and son on).

@m-kuhn Do you agree with that? And thank you for your involvement in this PR!

Member

pblottiere commented Apr 21, 2017

At least it's unanimous :).

As I already lost a bit of time on copy constructors, then clones and its unit tests, I propose to:

  • remove everything about the clone methods from this PR (to avoid unforeseen issues)
  • reinstate the prior mechanism of Restorers

Then, we'll be able to improve/remove the restorers mechanism afterward by updating QgsVectorLayerRenderer, QgsMapSettings, .... But for now, I think the most urgent thing to do is to continue removing parsers in WMS (getlegendgraphic request and son on).

@m-kuhn Do you agree with that? And thank you for your involvement in this PR!

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Apr 21, 2017

Member

@pblottiere sounds good to me.

any chance you can put the clone() code into a separate PR so we can reuse it for layer duplication?

sorry for the troubles and thanks for the pragmatic solution!

Member

m-kuhn commented Apr 21, 2017

@pblottiere sounds good to me.

any chance you can put the clone() code into a separate PR so we can reuse it for layer duplication?

sorry for the troubles and thanks for the pragmatic solution!

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Apr 21, 2017

Member

any chance you can put the clone() code into a separate PR so we can reuse it for layer duplication?

OK, I'm going to do that.

Member

pblottiere commented Apr 21, 2017

any chance you can put the clone() code into a separate PR so we can reuse it for layer duplication?

OK, I'm going to do that.

@pblottiere pblottiere referenced this pull request May 16, 2017

Merged

Add clone() methods for layers #4567

0 of 8 tasks complete
@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont May 17, 2017

Contributor

@m-kuhn, @nyalldawson, @wonder-sk now that this PR has been cleaned from clone method and passes tests. Are you agree to merge ?

Contributor

rldhont commented May 17, 2017

@m-kuhn, @nyalldawson, @wonder-sk now that this PR has been cleaned from clone method and passes tests. Are you agree to merge ?

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere May 17, 2017

Member

This PR is not ready to be merged.

The clone method is still there (by the way see #4567). Moreover, I'm waiting for this PR #4432 to be merged (otherwise the restorer is not working in case of the SLD).

Member

pblottiere commented May 17, 2017

This PR is not ready to be merged.

The clone method is still there (by the way see #4567). Moreover, I'm waiting for this PR #4432 to be merged (otherwise the restorer is not working in case of the SLD).

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere May 30, 2017

Member

I updated the code to remove everything about clone() methods.

Everything is green, let me know what you think.

Member

pblottiere commented May 30, 2017

I updated the code to remove everything about clone() methods.

Everything is green, let me know what you think.

@rldhont

Good for me

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jun 1, 2017

Contributor

If no one has an objection, I will merge this PR today. @nyalldawson @m-kuhn @elpaso

Contributor

rldhont commented Jun 1, 2017

If no one has an objection, I will merge this PR today. @nyalldawson @m-kuhn @elpaso

@pcav

This comment has been minimized.

Show comment
Hide comment
@pcav

pcav Jun 1, 2017

Member

BTW, is there a way of doing early testing of the server, once merged?
I'd be available.

Member

pcav commented Jun 1, 2017

BTW, is there a way of doing early testing of the server, once merged?
I'd be available.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jun 1, 2017

Member

No objections from my side.
Thanks a lot for this!

Member

m-kuhn commented Jun 1, 2017

No objections from my side.
Thanks a lot for this!

@rldhont rldhont merged commit df9ee6f into qgis:master Jun 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jun 1, 2017

Contributor

Thanks Paul!
Now, we can go to:

  • GetPrint
  • GetFeatureInfo
  • GetLegendGraphics
  • DXFWriter
Contributor

rldhont commented Jun 1, 2017

Thanks Paul!
Now, we can go to:

  • GetPrint
  • GetFeatureInfo
  • GetLegendGraphics
  • DXFWriter
@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jun 2, 2017

Member

Awesome.
Great work, @pblottiere

Member

m-kuhn commented Jun 2, 2017

Awesome.
Great work, @pblottiere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment