Skip to content
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

replace QgsMapRenderer #3809

Merged
merged 2 commits into from Dec 12, 2016
Merged

replace QgsMapRenderer #3809

merged 2 commits into from Dec 12, 2016

Conversation

@pblottiere
Copy link
Member

pblottiere commented Nov 28, 2016

The aim of this PR is to remove the QgsMapRenderer by using QgsMapRendererCustomPainterJob for qgis/QGIS-Enhancement-Proposals#74.

Moreover, some unit tests have been added.

My starting point was this PR made by @mhugent : #3129

Note that I wanted to use

renderJob.start();
renderJob.waitForFinished()

But there's some timeout in unit tests... So I used renderJob.renderSynchronously(); instead.

Let me know if you have any comments!

@pblottiere pblottiere force-pushed the pblottiere:render branch from 1f392b1 to 4ffed96 Nov 28, 2016
Copy link
Collaborator

nyalldawson left a comment

Fantastic work @mhugent and @pblottiere -- looks good to me and great to see the extra tests added too

%End

//! Some utility functions that may be included from everywhere in the code
namespace QgsMSUtils

This comment has been minimized.

@nyalldawson

nyalldawson Nov 28, 2016 Collaborator

With QGIS 3.0 we are trying to follow Qt's name capatilization conventions, so this should be QgsMsUtils. But we're also trying to remove all abbreviated names so QgsMapServerUtils would be best.


struct QgsLayerCoordinateTransform
{
QString srcAuthId;

This comment has been minimized.

@nyalldawson

nyalldawson Nov 28, 2016 Collaborator

As above, should be sourceAuthId and destinationAuthId. (As explanation we are doing this to make the api more predictable - in the past we mix up things like dst/dest/destination which means you're never sure which to use without having to check the docs.

//! @note Added in QGIS 3.0
//! Set the feature filter provider used by the QgsRenderContext of
//! each LayerRenderJob.
void setFeatureFilterProvider( const QgsFeatureFilterProvider *f ) { mFeatureFilterProvider = f; };

This comment has been minimized.

@nyalldawson

nyalldawson Nov 28, 2016 Collaborator

Would be good to note in the docs about the lifetime of the provider here - something like ownership is not transferred and the provider must not be deleted before the render job.

@@ -61,7 +62,7 @@

QString* QgsServer::sConfigFilePath = nullptr;
QgsCapabilitiesCache* QgsServer::sCapabilitiesCache = nullptr;
QgsMapRenderer* QgsServer::sMapRenderer = nullptr;
QgsMapSettings* QgsServer::sMapSettings = nullptr;

This comment has been minimized.

@nyalldawson

nyalldawson Nov 28, 2016 Collaborator

What's the motivation behind a static QgsMapSettings object? Why not just create one per map render?

@@ -724,20 +724,11 @@ bool QgsSLDConfigParser::wmsInspireActivated() const
return false;
}

QgsComposition* QgsSLDConfigParser::createPrintComposition( const QString& composerTemplate, QgsMapRenderer* mapRenderer, const QMap< QString, QString >& parameterMap, QStringList& highlightLayers ) const
QgsComposition* QgsSLDConfigParser::initComposition( const QString& composerTemplate, QgsMapSettings* mapSettings, QList< QgsComposerMap*>& mapList, QList< QgsComposerLegend* >& legendList, QList< QgsComposerLabel* >& labelList, QList<const QgsComposerHtml *>& htmlFrameList ) const

This comment has been minimized.

@nyalldawson

nyalldawson Nov 28, 2016 Collaborator

I'd make the mapSettings argument a reference here - it's required and composer will crash if a nullptr is passed.

context.setScaleFactor( mConfigParser->outputUnits() == QgsUnitTypes::RenderUnit::RenderMillimeters ? mMapSettings->outputDpi() / 25.4 : 1.0 );
context.setRendererScale( mMapSettings->scale() );
context.setMapToPixel( mMapSettings->mapToPixel() );
context.setExtent( mMapSettings->extent() );

This comment has been minimized.

@nyalldawson

nyalldawson Nov 28, 2016 Collaborator

I think this should use QgsRenderContext::fromMapSettings initially, and then override the things it needs for server. It's less maintenance/more future proof that way.

{
QgsVectorLayer* vl = qobject_cast<QgsVectorLayer*>( QgsMapLayerRegistry::instance()->mapLayer( layerID ) );
if ( !vl || !vl->renderer() )
continue;

if ( vl->hasScaleBasedVisibility() && ( mMapRenderer->scale() < vl->minimumScale() || mMapRenderer->scale() > vl->maximumScale() ) )
if ( vl->hasScaleBasedVisibility() && ( mMapSettings->scale() < vl->minimumScale() || mMapSettings->scale() > vl->maximumScale() ) )

This comment has been minimized.

@nyalldawson

nyalldawson Nov 28, 2016 Collaborator

Use QgsMapLayer::isInScaleRange here instead.

@@ -1699,13 +1704,13 @@ int QgsWmsServer::getFeatureInfo( QDomDocument& result, const QString& version )

//Render context is needed to determine feature visibility for vector layers
QgsRenderContext renderContext;
if ( mMapRenderer )
if ( mMapSettings )

This comment has been minimized.

@nyalldawson

nyalldawson Nov 28, 2016 Collaborator

This should also use QgsRenderContext::fromMapSettings

@nyalldawson nyalldawson mentioned this pull request Nov 28, 2016
@@ -20,6 +20,13 @@
//! Some utility functions that may be included from everywhere in the code
namespace QgsMSUtils
{
struct QgsLayerCoordinateTransform
{
QString srcAuthId;

This comment has been minimized.

@m-kuhn

m-kuhn Nov 28, 2016 Member

Would be great if you could add some doxygen comments to highlight the meaning of these variables.

@@ -992,7 +992,7 @@ void QgsWmsServer::runHitTest( const QgsMapSettings& mapSettings, QPainter* pain
if ( !vl || !vl->renderer() )
continue;

if ( vl->hasScaleBasedVisibility() && ( mapSettings.scale() < vl->minimumScale() || mapSettings.scale() > vl->maximumScale() ) )
if ( vl->hasScaleBasedVisibility() && vl->isInScaleRange( mapSettings.scale() ) )

This comment has been minimized.

@nyalldawson

nyalldawson Nov 30, 2016 Collaborator

the first test hasScaleBasedVisibility is included in isInScaleRange and can also be safely removed

@nyalldawson
Copy link
Collaborator

nyalldawson commented Nov 30, 2016

Looks great to me! +1 from my side, but a review by @wonder-sk would be good too.

Copy link
Member

wonder-sk left a comment

Great stuff - apart from few comments/questions I have the PR looks good to me.

QgsMapSettings mapSettings;
legendSettings.setMapScale( mapSettings.scale() );
double scaleFactor = mConfigParser->outputUnits() == QgsUnitTypes::RenderUnit::RenderMillimeters ? mapSettings.outputDpi() / 25.4 : 1.0;
legendSettings.setMmPerMapUnit( 1 / ( mapSettings.mapUnitsPerPixel() * scaleFactor ) );

This comment has been minimized.

@wonder-sk

wonder-sk Dec 1, 2016 Member

Does this bit of code work? It looks like a temporary map settings instance is constructed to get configuration from it (which will be just some default values)

context.setRendererScale( mMapRenderer->scale() );
context.setMapToPixel( *mMapRenderer->coordinateTransform() );
context.setExtent( mMapRenderer->extent() );
context.setRasterScaleFactor(( thePaintDevice->logicalDpiX() + thePaintDevice->logicalDpiY() ) / 2.0 / mapSettings.outputDpi() );

This comment has been minimized.

@wonder-sk

wonder-sk Dec 1, 2016 Member

It should not be necessary to set "raster scale factor" anymore and it should always stay equal to 1. It is a legacy used with QgsMapRenderer.

Next step would be to actually remove it from QgsRenderContext finally (in some other PR).

context.setMapToPixel( *mMapRenderer->coordinateTransform() );
context.setExtent( mMapRenderer->extent() );
context.setRasterScaleFactor(( thePaintDevice->logicalDpiX() + thePaintDevice->logicalDpiY() ) / 2.0 / mapSettings.outputDpi() );
context.setScaleFactor( mConfigParser->outputUnits() == QgsUnitTypes::RenderUnit::RenderMillimeters ? mapSettings.outputDpi() / 25.4 : 1.0 );

This comment has been minimized.

@wonder-sk

wonder-sk Dec 1, 2016 Member

The scale factor should have been already set to a correct value in QgsRenderContext::fromMapSettings()

@@ -8,6 +8,7 @@
%Import QtXml/QtXmlmod.sip

%Import core/core.sip
%Include core/qgsdatumtransformstore.sip

This comment has been minimized.

@wonder-sk

wonder-sk Dec 1, 2016 Member

Why do we include an extra sip file from "core"?

@@ -117,7 +117,7 @@ class SERVER_EXPORT QgsServer
// Status
static QString* sConfigFilePath;
static QgsCapabilitiesCache* sCapabilitiesCache;
static QgsMapRenderer* sMapRenderer;
static QgsMapSettings* sMapSettings;

This comment has been minimized.

@wonder-sk

wonder-sk Dec 1, 2016 Member

Is this still used anywhere? Not sure if we need a static map settings instance...

@pblottiere pblottiere force-pushed the pblottiere:render branch from 545c8ee to bba5c95 Dec 2, 2016
@pblottiere
Copy link
Member Author

pblottiere commented Dec 2, 2016

@nyalldawson @m-kuhn @wonder-sk

Thank you for your comments! I've updated the code accordingly.

#endif
renderJob.renderSynchronously();
//renderJob.start();
//renderJob.waitForFinished();
}

This comment has been minimized.

@pblottiere

pblottiere Dec 2, 2016 Author Member

A timeout is raised during unit tests when the start/waitForFinished functions are used. Do you know why?

This comment has been minimized.

@elpaso

elpaso Dec 4, 2016 Contributor

@pblottiere just guessing: maybe you miss an event loop? (A local event loop in the test might help if that's the case, connect to https://qgis.org/api/classQgsMapRendererJob.html#a8dbfbe2cc381f0eb6e721733838d3baf)


# r, h = self._result(self.server.handleRequest(qs))
# self._img_diff_error(r, h, "WMS_GetLegendGraphic_BBox2")

This comment has been minimized.

@pblottiere

pblottiere Dec 2, 2016 Author Member

Since the rebase, I didn't manage to use the getLegendGraphic request with a BBOX parameter. Each time, I have an empty response. So i disabled my tests for now.

What am i missing?

@pblottiere pblottiere force-pushed the pblottiere:render branch 2 times, most recently from 8ea9b83 to e8cb233 Dec 5, 2016
@pblottiere pblottiere force-pushed the pblottiere:render branch 3 times, most recently from a46b3e2 to 3dcb3cd Dec 9, 2016
@pblottiere pblottiere force-pushed the pblottiere:render branch from 3dcb3cd to 4d76308 Dec 12, 2016
@pblottiere pblottiere force-pushed the pblottiere:render branch from 4d76308 to 8cda1bb Dec 12, 2016
@rldhont rldhont merged commit 7028eee into qgis:master Dec 12, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.