Feature #8725-revival: Fast rendering of geometries with provider simplification support

[ahuarte47]

Implements fast rendering of LineStrings and Polygons simplificating the geometries before render them in QGIS. Also disable 'Antialiasing' when it is possible.

This PR is inspired in merged PR #980, but it contains several changes based on advice received ( @Matthias-Kuhn thanks! ) and new features:

• About code, the simplification is configured in a new QgsSimplifyMethod class which indicates how to simplify the geometries in a feature iterator.
• Now, the user can define where the simplification executes (There is a new option in settings panel), locally after fetch the geometry from provider, or simplifying it on provider side. e.g. In postgres provider, first option simplifies the geometry already fetched locally, but the second option simplifies the geometry in database using the function ST_Simplify.
• The settings panel also shows the simplification threshold in pixel units as @timlinux suggested me.

About postgres simplification, the ST_simplify function needs a tolerance parameter, I use map2pixel/5.0 as input value, it is experimental and I must define it better (All ideas are welcome). This simplification can be applicable to other database providers (MySQL, SQL Server, Oracle...)

[ahuarte47]

Hi @Matthias-Kuhn, about...

@ahuarte47 What do you think about the idea of dropping the capability indication for this feature completely and do this on an iterator basis? This would allow for finer-grained control, the provider decides at runtime if it can simplify a layer based on a particular request and datasource and communicates it only after the decision for a certain request.

This code could fix your comment ?

QgsFeatureIterator QgsOgrProvider::getFeatures( const QgsFeatureRequest& request )
{
  QgsOgrFeatureIterator* ogrFeatureIterator = new QgsOgrFeatureIterator( this, request );

  if ( request.simplifyMethod().methodType() != QgsSimplifyMethod::NoSimplification )
  {
    const QgsSimplifyMethod& simplifyMethod = request.simplifyMethod();

    if ( simplifyMethod.forceLocalOptimization() )
    {
      ogrFeatureIterator->prepareLocalSimplification(); //-> from 'QgsAbstractFeatureIterator'
    }
    else
    {
      ogrFeatureIterator->prepareProviderSimplification();
    }
  }
  return QgsFeatureIterator( ogrFeatureIterator );
}

But, I think that capability indication is useful because of the settings panel disable the 'simplification on provider side' checkbox when this capability is not supported.

About my proposed code above, now I call to 'prepareLocalSimplification' method in QgsVectorLayerFeatureIterator class, this allows me to not have to call it in all providers, but with this change ...

Also, I can not prepare the local/provider simplification in the constructor of QgsAbstractFeatureIterator class because of QgsVectorLayerFeatureIterator creates two QgsAbstractFeatureIterator instances (QgsVectorLayerFeatureIterator itself and mProviderIterator member) and the simplification of each geometry runs twice in local mode.

Alvaro

[ahuarte47]

@Matthias-Kuhn said me:

Concerning your comment about having two iterators on top of each other and therefore code being executed twice: please have a look at the filterExpression code, the same kind of problem had to be tackled there. I think it should be possible to apply almost the same logic to the simplification code.

Edit
As I am not sure if the filterExpression code applies that well:
In the vector layer feature iterator, only simplify features with a) changed geometries or b) which originate from the local mAddedFeatures cache. Leave the simplification of everything else to the provider iterator.

[nyalldawson]

One question - is it necessary to have the checkbox for simplifying at the provider? Couldn't we just always enable this? Is there any down sides to it?

[ahuarte47]

I think in situations where there are many applications using a server, then it is more interesting that simplification running locally to avoid overloading the server

One question - is it necessary to have the checkbox for simplifying at the provider? Couldn't we just always enable this? Is there any down sides to it?

[m-kuhn]

How about leaving server-side simplification as default, with an opt-out checkbox "Force local simplification" hidden in a collapsible groupbox labelled "Advanced settings" along with other simplification fine-tuning options (collapsed by default)?

Some pseudo-code to outline the concept I have in mind. Feel free to tear them apart and destroy them :)

Main pro argument IMHO: It allows for the providers to make the decision, if a simplification can be done locally or remote based on a particular QgsFeatureRequest (as an iterator is always based on a request)

class QgsAbstractFeatureIterator
{
  virtual void simplify( QgsFeature& f ); // Default implementation contains your simplification code
  bool nextFeature ( QgsFeature& f )
  {
    // adapt current code to not return immediately
    simplify( f );
    // return appropriate value
}

class QgsOgrFeatureIterator
{
  virtual void simplify( QgsFeature& f ) { return; } // Overwrite default simplification algorithm as no-op

  virtual bool fetchFeature( QgsFeature& f ) 
  {
    // Insert provider-specific simplification code here
  }
}

class QgsPostgresFeatureIterator
{
  virtual void simplify( QgsFeature& f ) { return; } // Overwrite default simplification algorithm as no-op

  QgsPostgresFeatureIterator() // Constructor
  {
    // Adjust where clause if required
  }
}

class QgsVectorLayerFeatureIterator
{
  virtual void simplify( QgsFeature& f ) { return; } // Overwrite default simplification algorithm as no-op

  // Adjust methods to only simplify changedGeometries and addedFeatures
}

[ahuarte47]

Hi @Matthias-Kuhn, thanks for your proposal, but one thing I do not like ( ;-) ), the exitence of public methods to do nothing, they are confuse to programmers who uses. IMHO I think better simplify the geometries within of iterator without a "simplify" method visible. The iterator will simplify the geometries how it knowns (arguments...) according how the simplification was defined or deactivated.

I could write a more explicit method to configure the simplification, then it will be execute or not "within" nextFeature method as it decides.

QgsAbstractFeatureIterator::setSimplifyMethod(const QgsSimplifyMethod& simplifyMethod);

What do you think ?

[ahuarte47]

It is curious, in Geoserver-devel mailing list there is a similar debate about simplification and ST_simplify:
http://sourceforge.net/mailarchive/forum.php?forum_name=geoserver-devel

They are implementing the "simplification" of geometries fetched using a similar strategy as we are thinking for QGIS
:-)

[ahuarte47]

about...

How about leaving server-side simplification as default, with an opt-out checkbox "Force local simplification" hidden in a collapsible groupbox labelled "Advanced settings" along with other simplification fine-tuning options (collapsed by default)?

I agree, I will add a groupbox labelled "Advanced settings" containing the two current options: "Force local simplification" CheckBox and the "simplification threshold" slider.

Alvaro

[m-kuhn]

You are right, they don't need to be public, they could be made private, so they could be overwritten, but not called.

The pro of a method is, that it can contain logic. E.g.

void simplify()
{
  // Provider X does not support topological simplification
  if ( mRequest.simplifyMethod().type() == QgsSimplifyMethod.PreserveTopology )
    QgsAbstractFeatureIterator::simplify();
  else // But provider X supports anything else
    return;
}

[NathanW2]

Yes no other methods should be public on a QgsAbstractFeatureIterator apart from nextFeature, rewind, and close

[ahuarte47]

Ok, I will apply your advices
Thanks!

[ahuarte47]

done...

How about leaving server-side simplification as default, with an opt-out checkbox "Force local simplification" hidden in a collapsible groupbox labelled "Advanced settings" along with other simplification fine-tuning options (collapsed by default)?

[3nids]

@ahuarte47 hi, I don't know what is the status of your work. But I observe that the last master is still much much slower than before the merge of the fast rendering.

I tested this again with postgis layers (one line, one polygon, one point), with or without labeling.
The difference is huge, QGIS 2.0 is almost instantaneous while last master takes more than 1 sec to refresh the canvas. The problem is that the last master is slower with or without the simplification activated.

Am I the only one observing this?

Thanks!

[ahuarte47]

@ahuarte47 hi, I don't know what is the status of your work. But I observe that the last master is still much much slower than before the merge of the fast rendering.
I tested this again with postgis layers (one line, one polygon, one point), with or without labeling.
The difference is huge, QGIS 2.0 is almost instantaneous while last master takes more than 1 sec to refresh the canvas. The problem is that the last master is slower with or without the simplification activated.
Am I the only one observing this?

Hi @3nids, this PR is a simple refactoring of PR #980 already merged. When you got normal results you tried the PR #1040 where the simplification runs directly in rendering loop. This new PR implements the simplification in FeatureIterator classes and I must investigate why it is so slow in your SO (Ubuntu). In Windows I get normal behavior.

Thanks!

[3nids]

I obtained normal results with QGIS 2.0 and speed issues with today's master.
I did not try with the PR 1040 yet (I am a bit lost in the status of the implementation).

I can give it a try, if you'd like to.

[ahuarte47]

Then, I would greatly appreciate you to try this PR #1053, this should be the final (PR #1040 was a test), It only needs a small rearrangement in the code applying one advice of @Matthias-Kuhn, not how the geometries are simplified (This really has never changed).

Thank you very much!

[3nids]

@ahuarte47 It is at least as fast as before! can't wait to see this merged!

[m-kuhn]

@3nids Did you also test with provider-side postgis simplification?

[3nids]

@Matthias-Kuhn yes. I can't tell a difference with/without simplification and provider/qgis side in terms of rendering speed. Would need further testing.

[m-kuhn]

@3nids Are both "too fast to tell" or is it "they are both in the same range of acceptable latency"?

[3nids]

@Matthias-Kuhn I would say exactly as fast as it was in QGIS 2.0. So, something in between ;)

To be more explicit: acceptable latency in my big project. It seems faster when using server side, but marginally.
Too fast to tell, in the small project.

[m-kuhn]

Cool. So all tests look good so far. Main issue for me is the architectural change which you seem to be ok with Alvaro.

Looking forward to merging this soon, so we have some time to iron out any leftover issues before the release of 2.2.

[ahuarte47]

Hi @Matthias-Kuhn, I hope that last commit (ahuarte47@605b66b) implements the simplification methods as you advised me.

I still have two tasks:

• define a precise tolerance for ST_simplify calls in postgres provider Fix bug #8725R: fix collapsed geometries by ST_simplify function of postgis #1099
• simplify changedGeometries and addedFeatures in QgsVectorLayerFeatureIterator (commit: ahuarte47@8b990ad)

Alvaro

[m-kuhn]

Hi Alvaro,

Is this branch ready to be reviewed?

[ahuarte47]

Hi @Matthias-Kuhn, I think so, but I have pending define a more precise value for the tolerance of ST_simplify calls in postgres provider.

I am testing how geoserver define it but I could not complete this task, sorry. The current value used is very conservative (map2pixel tolerance calculated / 5.0 -> "src/providers/postgres/qgspostgresfeatureiterator.cpp+line 312") and may just be that the new value is greater.

Alvaro

[3nids]

Would it be possible to merge this and do the tuning later on?

This pull request fixes (at least on Ubuntu) a major regression in terms of speed rendering.

[m-kuhn]

I will have a look at it ASAP. The fine-tuning of the postgres parameter is no requirement for this pull request.

[m-kuhn]

Hi Alvaro,

I had a look at the code. Most things look good, sweet we now have also for postgres server side simplification. I hope other provider maintainer will bother to follow this good example.
There is still one thing I do not see implemented, and that is the simplification on a per-iterator, rather than a per-provider basis. I would totally remove the Simplification Capabilities from the provider class itself. The vector layer should only bother for the configuration settings, when setting the forceLocalSimplification flag on the request and not take the providers' capabilities into consideration.

Basically that means adding the following new virtual method:

QgsAbstractFeatureIterator
{
  private:
    virtual bool providerCanSimplify() { return false; }
}

This should be overwritten by OGR, Postgres and VectorLayer iterators to return true.

In the constructor of the QgsAbstractFeatureIterator, you can insert the following code

if ( mRequest.simplificationMethod().forceLocal() || !providerCanSimplify() )
  mLocalSimplification = true;

And in

QgsAbstractFeatureIterator::nextFeature( QgsFeature& )
{
....
if ( mLocalSimplification )
  simplify( f ); // sidenote: no need to check for f.geometry() if you already to this inside the simplify() method

There is a slight change in the requirements to before. My old comments would have meant a call to a virtual method per feature, while the updated comment (this message) is on a per-iterator basis. But as you still didn't change from a per-provider basis, I figure it's ok to adjust it.

I assume it should not be much work, looking at the current state of the code.

[anitagraser]

@NathanW2 That's fine with me as long as it's discussed before the release. I suggest to discuss the GUI on http://osgeo-org.1560.x6.nabble.com/Discuss-renderer-behavior-geometry-simplification-settings-GUI-tc5098020.html

[NathanW2]

Yeah it will be discussed. We still have about a month and a half to make
those tweaks.

To me the main code needs to be merged after Matthais is happy with it so
we can get some performance testing done. People will handle a bit of a
bad UI but performance issue not so much.

• Nathan

On Wed, Jan 15, 2014 at 7:52 AM, anitagraser [email protected]:

@NathanW2 https://github.com/NathanW2 That's fine with me as long as
it's discussed before the release. I suggest to discuss the GUI on
http://osgeo-org.1560.x6.nabble.com/Discuss-renderer-behavior-geometry-simplification-settings-GUI-tc5098020.html

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1053#issuecomment-32311416
.

[ahuarte47]

Better ?

[ahuarte47]

Hi @Matthias-Kuhn

Last commit contains all advices received (I guess):

• add comment about 'prepareSimplification()' in constructor
• fix comment in 'providerCanSimplify()'
• simplify UI (Feature #8725-revival: Fast rendering of geometries with provider simplification support #1053 (comment)) with @timlinux and @anitagraser advices. I hope resolved the OSX problem.

Also, I did rebase.

I will implement the scale filter as Tim suggested me in other PR.

Thanks for all!
Best Regards
Alvaro

[timlinux]

@ahuarte47 New UI looks better thanks. Last line can be simplified (ha!) to read:

• Simplify on provider side if possible

@NathanW2 Fully agreed about UI tweaks being able to wait till after core stuff works - I just added my comment to the PR since they were not addressed when the last PR was closed and I didn't want them to get forgotten.

[ahuarte47]

Done: ahuarte47@c79cafc

New UI looks better thanks. Last line can be simplified (ha!) to read:
Simplify on provider side if possible

Thanks @timlinux

[m-kuhn]

Merged to master

A huge thank you to Alvaro, amazing work! Especially for the patience with all the wishes I had.

Let's see how it works in the wild, and I assume you will be available for any follow-ups to this PR which may appear in the upcoming testing phase.

8fb87f1

[3nids]

So nice! Thanks a lot for the job!

[nyalldawson]

A big +1 from me to what Matthias and Denis have said. Thanks for putting in the time to implement these great changes @ahuarte47 !

[ahuarte47]

it was a team effort, thanks for all!
All advices were lessons for me!

Alvaro

[ahuarte47]

Now, I still have tasks:

• fix errors if needed
• define a precise tolerance for ST_simplify calls Fix bug #8725R: fix collapsed geometries by ST_simplify function of postgis #1099
• filter scale from @timlinux advice Feature #8725R: support for maximum scale at which the layer should be simplified #1094
• implement simplification on provider side for MySQL, Oracle, SQL Server, ....
• improve UI if needed

Before I would like fix bug #9060, I have partial fixes and code to finish it
Regards

[timlinux]

Wow Alvaro, congrats and kudos for your patience. Thanks for also taking on
board my suggestions!

Regards

Tim

On Wed, Jan 15, 2014 at 12:53 PM, Alvaro Huarte [email protected]:

Now, I still have tasks:

• fix errors if needed
• define a precise tolerance for ST_simplify calls
• filter scale from @timlinux https://github.com/timlinux advice
• implement simplification on provider side for MySQL, Oracle, SQL
  Server, ....
• imporver UI if needed

Before I would like fix bug #9060, I has partial fixes and code to finish
it
Regards

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1053#issuecomment-32351410
.

Tim Sutton - QGIS Project Steering Committee Member

Visit http://linfiniti.com to find out about:

• QGIS programming services
• GeoDjango web development

• FOSS Consulting Services
  Skype: timlinux Irc: timlinux on #qgis at freenode.net

[ahuarte47]

Done:
#1094

Could you add a maximum scale at which generalisation should be carried out to the layer rendering options? ?>> So >e.g. if I choose 1:50000, no simplification should be carried out in scales [1:1, 1:50000].