Skip to content
Permalink
Browse files

Fix rendering geometry generators for line layers

And add more tests
  • Loading branch information
m-kuhn committed Dec 11, 2015
1 parent c176e3f commit c9fa33410d3a40e4dde0a33b993d91263580e8bd
@@ -714,7 +714,10 @@ class CORE_EXPORT QgsVectorLayer : public QgsMapLayer
/** Return const renderer V2. */
const QgsFeatureRendererV2* rendererV2() const { return mRendererV2; }

/** Set renderer V2. */
/**
* Set renderer which will be invoked to represent this layer.
* Ownership is transferred.
*/
void setRendererV2( QgsFeatureRendererV2* r );

/** Returns point, line or polygon */
@@ -1227,26 +1227,29 @@ void QgsMarkerSymbolV2::renderPoint( const QPointF& point, const QgsFeature* f,

if ( layerIdx != -1 )
{
if ( layerIdx >= 0 && layerIdx < mLayers.count() )
QgsSymbolLayerV2* symbolLayer = mLayers.value( layerIdx );
if ( symbolLayer )
{
QgsMarkerSymbolLayerV2* markerLayer = dynamic_cast<QgsMarkerSymbolLayerV2*>( mLayers.at( layerIdx ) );

if ( markerLayer )
if ( symbolLayer->type() == QgsSymbolV2::Marker )
{
QgsMarkerSymbolLayerV2* markerLayer = static_cast<QgsMarkerSymbolLayerV2*>( symbolLayer );
renderPointUsingLayer( markerLayer, point, symbolContext );
}
else
renderUsingLayer( mLayers.at( layerIdx ), symbolContext );
renderUsingLayer( symbolLayer, symbolContext );
}
return;
}

Q_FOREACH ( QgsSymbolLayerV2* layer, mLayers )
Q_FOREACH ( QgsSymbolLayerV2* symbolLayer, mLayers )
{
QgsMarkerSymbolLayerV2* markerLayer = dynamic_cast<QgsMarkerSymbolLayerV2*>( layer );

if ( markerLayer )
if ( symbolLayer->type() == QgsSymbolV2::Marker )
{
QgsMarkerSymbolLayerV2* markerLayer = static_cast<QgsMarkerSymbolLayerV2*>( symbolLayer );
renderPointUsingLayer( markerLayer, point, symbolContext );
}
else
renderUsingLayer( layer, symbolContext );
renderUsingLayer( symbolLayer, symbolContext );
}
}

@@ -1428,28 +1431,31 @@ void QgsLineSymbolV2::renderPolyline( const QPolygonF& points, const QgsFeature*

if ( layerIdx != -1 )
{
if ( layerIdx >= 0 && layerIdx < mLayers.count() )
QgsSymbolLayerV2* symbolLayer = mLayers.value( layerIdx );
if ( symbolLayer )
{
QgsLineSymbolLayerV2* lineLayer = dynamic_cast<QgsLineSymbolLayerV2*>( mLayers.at( layerIdx ) );

if ( lineLayer )
if ( symbolLayer->type() == QgsSymbolV2::Line )
{
QgsLineSymbolLayerV2* lineLayer = static_cast<QgsLineSymbolLayerV2*>( symbolLayer );
renderPolylineUsingLayer( lineLayer, points, symbolContext );
}
else
renderUsingLayer( mLayers.at( layerIdx ), symbolContext );
renderUsingLayer( symbolLayer, symbolContext );
}
return;
}

Q_FOREACH ( QgsSymbolLayerV2* symbolLayer, mLayers )
{
if ( symbolLayer->type() != QgsSymbolV2::Line )
continue;
QgsLineSymbolLayerV2* lineLayer = static_cast<QgsLineSymbolLayerV2*>( symbolLayer );

if ( lineLayer )
if ( symbolLayer->type() == QgsSymbolV2::Line )
{
QgsLineSymbolLayerV2* lineLayer = static_cast<QgsLineSymbolLayerV2*>( symbolLayer );
renderPolylineUsingLayer( lineLayer, points, symbolContext );
}
else
{
renderUsingLayer( symbolLayer, symbolContext );
}
}

context.setPainter( renderPainter );
@@ -1502,23 +1508,23 @@ void QgsFillSymbolV2::renderPolygon( const QPolygonF& points, QList<QPolygonF>*

if ( layerIdx != -1 )
{
QgsSymbolLayerV2* layer = mLayers.value( layerIdx );
if ( layer )
QgsSymbolLayerV2* symbolLayer = mLayers.value( layerIdx );
if ( symbolLayer )
{
if ( layer->type() == Fill || layer->type() == Line )
renderPolygonUsingLayer( layer, points, rings, symbolContext );
if ( symbolLayer->type() == Fill || symbolLayer->type() == Line )
renderPolygonUsingLayer( symbolLayer, points, rings, symbolContext );
else
renderUsingLayer( layer, symbolContext );
renderUsingLayer( symbolLayer, symbolContext );
}
return;
}

Q_FOREACH ( QgsSymbolLayerV2* layer, mLayers )
Q_FOREACH ( QgsSymbolLayerV2* symbolLayer, mLayers )
{
if ( layer->type() == Fill || layer->type() == Line )
renderPolygonUsingLayer( layer, points, rings, symbolContext );
if ( symbolLayer->type() == Fill || symbolLayer->type() == Line )
renderPolygonUsingLayer( symbolLayer, points, rings, symbolContext );
else
renderUsingLayer( layer, symbolContext );
renderUsingLayer( symbolLayer, symbolContext );
}
}

@@ -31,6 +31,7 @@
from qgis.core import (QgsVectorLayer,
QgsSingleSymbolRendererV2,
QgsFillSymbolV2,
QgsLineSymbolV2,
QgsMarkerSymbolV2,
QgsMapLayerRegistry,
QgsRectangle,
@@ -53,49 +54,98 @@
class TestQgsGeometryGeneratorSymbolLayerV2(TestCase):

def setUp(self):
myShpFile = os.path.join(TEST_DATA_DIR, 'polys_overlapping.shp')
layer = QgsVectorLayer(myShpFile, 'Polygons', 'ogr')
QgsMapLayerRegistry.instance().addMapLayer(layer)
polys_shp = os.path.join(TEST_DATA_DIR, 'polys.shp')
points_shp = os.path.join(TEST_DATA_DIR, 'points.shp')
lines_shp = os.path.join(TEST_DATA_DIR, 'lines.shp')
self.polys_layer = QgsVectorLayer(polys_shp, 'Polygons', 'ogr')
self.points_layer = QgsVectorLayer(points_shp, 'Points', 'ogr')
self.lines_layer = QgsVectorLayer(lines_shp, 'Lines', 'ogr')
QgsMapLayerRegistry.instance().addMapLayer(self.polys_layer)
QgsMapLayerRegistry.instance().addMapLayer(self.lines_layer)
QgsMapLayerRegistry.instance().addMapLayer(self.points_layer)

# Create style
sym1 = QgsFillSymbolV2.createSimple({'color': '#fdbf6f'})
sym2 = QgsLineSymbolV2.createSimple({'color': '#fdbf6f'})
sym3 = QgsMarkerSymbolV2.createSimple({'color': '#fdbf6f'})

self.polys_layer.setRendererV2(QgsSingleSymbolRendererV2(sym1))
self.lines_layer.setRendererV2(QgsSingleSymbolRendererV2(sym2))
self.points_layer.setRendererV2(QgsSingleSymbolRendererV2(sym3))

self.renderer = QgsSingleSymbolRendererV2(sym1)
layer.setRendererV2(self.renderer)
self.mapsettings = CANVAS.mapSettings()
self.mapsettings.setOutputSize(QSize(400, 400))
self.mapsettings.setOutputDpi(96)
self.mapsettings.setExtent(QgsRectangle(-133, 22, -70, 52))

rendered_layers = [layer.id()]
self.mapsettings.setLayers(rendered_layers)

def tearDown(self):
QgsMapLayerRegistry.instance().removeAllMapLayers()

def test_marker(self):
sym = self.renderer.symbol()
sym = self.polys_layer.rendererV2().symbol()
sym_layer = QgsGeometryGeneratorSymbolLayerV2.create({'geometryModifier': 'centroid($geometry)'})
sym_layer.setSymbolType(QgsSymbolV2.Marker)
sym.changeSymbolLayer(0, sym_layer)

rendered_layers = [self.polys_layer.id()]
self.mapsettings.setLayers(rendered_layers)

renderchecker = QgsMultiRenderChecker()
renderchecker.setMapSettings(self.mapsettings)
renderchecker.setControlName('expected_geometrygenerator_marker')
self.assertTrue(renderchecker.runTest('geometrygenerator_marker'))

def test_mixed(self):
sym = self.renderer.symbol()
sym = self.polys_layer.rendererV2().symbol()

buffer_layer = QgsGeometryGeneratorSymbolLayerV2.create({'geometryModifier': 'buffer($geometry, "value"/15)'})
buffer_layer.setSymbolType(QgsSymbolV2.Fill)
buffer_layer.subSymbol()
self.assertIsNotNone(buffer_layer.subSymbol())
sym.appendSymbolLayer(buffer_layer)
marker_layer = QgsGeometryGeneratorSymbolLayerV2.create({'geometryModifier': 'centroid($geometry)'})
marker_layer.setSymbolType(QgsSymbolV2.Marker)
sym.appendSymbolLayer(marker_layer)

rendered_layers = [self.polys_layer.id()]
self.mapsettings.setLayers(rendered_layers)

renderchecker = QgsMultiRenderChecker()
renderchecker.setMapSettings(self.mapsettings)
renderchecker.setControlName('expected_geometrygenerator_mixed')
self.assertTrue(renderchecker.runTest('geometrygenerator_mixed'))

def test_buffer_lines(self):
sym = self.lines_layer.rendererV2().symbol()

buffer_layer = QgsGeometryGeneratorSymbolLayerV2.create({'geometryModifier': 'buffer($geometry, "value"/15)'})
buffer_layer.setSymbolType(QgsSymbolV2.Fill)
self.assertIsNotNone(buffer_layer.subSymbol())
sym.appendSymbolLayer(buffer_layer)

rendered_layers = [self.lines_layer.id()]
self.mapsettings.setLayers(rendered_layers)

renderchecker = QgsMultiRenderChecker()
renderchecker.setMapSettings(self.mapsettings)
renderchecker.setControlName('expected_geometrygenerator_buffer_lines')
self.assertTrue(renderchecker.runTest('geometrygenerator_buffer_lines'))

def test_buffer_points(self):
sym = self.points_layer.rendererV2().symbol()

buffer_layer = QgsGeometryGeneratorSymbolLayerV2.create({'geometryModifier': 'buffer($geometry, "staff"/15)'})
buffer_layer.setSymbolType(QgsSymbolV2.Fill)
self.assertIsNotNone(buffer_layer.subSymbol())
sym.appendSymbolLayer(buffer_layer)

rendered_layers = [self.points_layer.id()]
self.mapsettings.setLayers(rendered_layers)

renderchecker = QgsMultiRenderChecker()
renderchecker.setMapSettings(self.mapsettings)
renderchecker.setControlName('expected_geometrygenerator_buffer_points')
self.assertTrue(renderchecker.runTest('geometrygenerator_buffer_points'))


if __name__ == '__main__':
unittest.main()
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.

8 comments on commit c9fa334

@nirvn

This comment has been minimized.

Copy link
Contributor

@nirvn nirvn replied Dec 20, 2015

@m-kuhn , playing with the generator, loving it more and more.

After several days of usage, one thing became evident: we should get rid of the Geometry Type drop down list (i.e. poygon, line point), and instead have the geometry type set according to the type returned by the expression. If the expression returned is invalid (or does not return a geometry), stick to the last valid geometry type returned (by default, it'll be the same as the layer geometry type since you begin with a simple $geometry expression).

Thoughts? @nyalldawson ?

@m-kuhn

This comment has been minimized.

Copy link
Member Author

@m-kuhn m-kuhn replied Dec 20, 2015

The idea sounds nice in theory, but in practice this means, that we do not only need a one feature sample of the output (as currently done for the preview below the editor) but need to be certain about what is returned.

It is well possible that not every feature returns a geometry or different geometry types are returned and in this case it needs to be possible to specify the geometry manually.

An approach would be to have a button "polygon configured but line generated, [link]change to line symbol now[/link]".

@nirvn

This comment has been minimized.

Copy link
Contributor

@nirvn nirvn replied Dec 20, 2015

Could you loop through a sample, like 10 features, and pick most returned type?

Maybe the geometry type should have an additional "Automatically determined" item which would serve as default setting. That would be more user friendly.

@m-kuhn

This comment has been minimized.

Copy link
Member Author

@m-kuhn m-kuhn replied Dec 20, 2015

And which type of subsymbol would be shown with "automatically determined"?
Would the symbol be changed silently everytime the type of the calculated geometry changes?

@nirvn

This comment has been minimized.

Copy link
Contributor

@nirvn nirvn replied Dec 20, 2015

Yes, the subsymbol type would match / change according to the type of calculated area, when someone finishes writing the expression by leaving the text area as to avoid switching to intermediary types while constructing the expression.

@nirvn

This comment has been minimized.

Copy link
Contributor

@nirvn nirvn replied Dec 20, 2015

The one problem I can forsee with this: someone accidentally losing style with an accidental type change.

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Dec 20, 2015

What's the issue with the current approach? This is always going to be an advanced feature, and I think the current GUI is fine.

@nirvn

This comment has been minimized.

Copy link
Contributor

@nirvn nirvn replied Dec 20, 2015

I get slightly irritated to have to manually switch type when the calculated geometry is a known type.

The current UI isn't wrong per say. Maybe it's just me testing the feature a lot, vs norma sporadic use of it.

Please sign in to comment.
You can’t perform that action at this time.