Skip to content

Commit

Permalink
Rework API and improve memory handling of graph objects
Browse files Browse the repository at this point in the history 
Instead of QgsGraphBuilder::graph() taking the ownership of the graph
and leaving the QgsGraphBuilder in an unpredictable state, add an
explicit "takeGraph" method which takes the existing graph and
make the existing "graph" method just return a copy of the graph.

Also fix corresponding memory leaks in network analysis processing
algorithms.

Fixes #44079
  • Loading branch information
@nyalldawson
nyalldawson committed Jul 21, 2021
1 parent 8ebeab1 commit 13e3d2f
Show file tree
Hide file tree
Showing 9 changed files with 68 additions and 35 deletions.
16 changes: 14 additions & 2 deletions python/analysis/auto_generated/network/qgsgraphbuilder.sip.in
View file
Expand Up @@ -38,9 +38,21 @@ MANDATORY BUILDER PROPERTY DECLARATION
virtual void addEdge( int pt1id, const QgsPointXY &pt1, int pt2id, const QgsPointXY &pt2, const QVector< QVariant > &prop );


QgsGraph *graph() /Factory/;
QgsGraph graph() const;
%Docstring
Returns generated :py:class:`QgsGraph`
Returns the generated :py:class:`QgsGraph`.

The builder is left in its current state.

.. seealso:: :py:func:`takeGraph`
%End

QgsGraph *takeGraph() /Factory/;
%Docstring
Takes the generated graph from the builder, resetting the builder back to its initial
state ready for additional graph construction.

.. versionadded:: 3.22
%End

};
Expand Down
21 changes: 13 additions & 8 deletions src/analysis/network/qgsgraphbuilder.cpp
View file
Expand Up @@ -26,13 +26,10 @@
QgsGraphBuilder::QgsGraphBuilder( const QgsCoordinateReferenceSystem &crs, bool otfEnabled, double topologyTolerance, const QString &ellipsoidID )
: QgsGraphBuilderInterface( crs, otfEnabled, topologyTolerance, ellipsoidID )
{
mGraph = new QgsGraph();
mGraph = std::make_unique< QgsGraph >();
}

QgsGraphBuilder::~QgsGraphBuilder()
{
delete mGraph;
}
QgsGraphBuilder::~QgsGraphBuilder() = default;

void QgsGraphBuilder::addVertex( int, const QgsPointXY &pt )
{
Expand All @@ -44,9 +41,17 @@ void QgsGraphBuilder::addEdge( int pt1id, const QgsPointXY &, int pt2id, const Q
mGraph->addEdge( pt1id, pt2id, prop );
}

QgsGraph *QgsGraphBuilder::graph()
QgsGraph QgsGraphBuilder::graph() const
{
QgsGraph *res = mGraph;
mGraph = nullptr;
return *mGraph;
}

QgsGraph *QgsGraphBuilder::takeGraph()
{
QgsGraph *res = mGraph.release();

// create a new graph in case this builder is used for additional work
mGraph = std::make_unique< QgsGraph >();

return res;
}
18 changes: 15 additions & 3 deletions src/analysis/network/qgsgraphbuilder.h
View file
Expand Up @@ -51,13 +51,25 @@ class ANALYSIS_EXPORT QgsGraphBuilder : public QgsGraphBuilderInterface SIP_NODE
void addEdge( int pt1id, const QgsPointXY &pt1, int pt2id, const QgsPointXY &pt2, const QVector< QVariant > &prop ) override;

/**
* Returns generated QgsGraph
* Returns the generated QgsGraph.
*
* The builder is left in its current state.
*
* \see takeGraph()
*/
QgsGraph *graph() SIP_FACTORY;
QgsGraph graph() const;

/**
* Takes the generated graph from the builder, resetting the builder back to its initial
* state ready for additional graph construction.
*
* \since QGIS 3.22
*/
QgsGraph *takeGraph() SIP_FACTORY;

private:

QgsGraph *mGraph = nullptr;
std::unique_ptr< QgsGraph > mGraph;

QgsGraphBuilder( const QgsGraphBuilder & ) = delete;
QgsGraphBuilder &operator=( const QgsGraphBuilder & ) = delete;
Expand Down
8 changes: 5 additions & 3 deletions src/analysis/processing/qgsalgorithmserviceareafromlayer.cpp
View file
Expand Up @@ -109,7 +109,7 @@ QVariantMap QgsServiceAreaFromLayerAlgorithm::processAlgorithm( const QVariantMa
mDirector->makeGraph( mBuilder.get(), points, snappedPoints, feedback );

feedback->pushInfo( QObject::tr( "Calculating service areas…" ) );
QgsGraph *graph = mBuilder->graph();
std::unique_ptr< QgsGraph > graph( mBuilder->takeGraph() );

QgsFields fields = startPoints->fields();
fields.append( QgsField( QStringLiteral( "type" ), QVariant::String ) );
Expand All @@ -136,7 +136,7 @@ QVariantMap QgsServiceAreaFromLayerAlgorithm::processAlgorithm( const QVariantMa
QgsFeature feat;
QgsAttributes attributes;

int step = snappedPoints.size() > 0 ? 100.0 / snappedPoints.size() : 1;
const double step = snappedPoints.size() > 0 ? 100.0 / snappedPoints.size() : 1;
for ( int i = 0; i < snappedPoints.size(); i++ )
{
if ( feedback->isCanceled() )
Expand All @@ -147,7 +147,7 @@ QVariantMap QgsServiceAreaFromLayerAlgorithm::processAlgorithm( const QVariantMa
idxStart = graph->findVertex( snappedPoints.at( i ) );
origPoint = points.at( i ).toString();

QgsGraphAnalyzer::dijkstra( graph, idxStart, 0, &tree, &costs );
QgsGraphAnalyzer::dijkstra( graph.get(), idxStart, 0, &tree, &costs );

QgsMultiPointXY areaPoints;
QgsMultiPolylineXY lines;
Expand Down Expand Up @@ -235,6 +235,8 @@ QVariantMap QgsServiceAreaFromLayerAlgorithm::processAlgorithm( const QVariantMa
}
} // costs

upperBoundary.reserve( nodes.size() );
lowerBoundary.reserve( nodes.size() );
for ( int n : std::as_const( nodes ) )
{
upperBoundary.push_back( graph->vertex( graph->edge( tree.at( n ) ).toVertex() ).point() );
Expand Down
6 changes: 4 additions & 2 deletions src/analysis/processing/qgsalgorithmserviceareafrompoint.cpp
View file
Expand Up @@ -103,12 +103,12 @@ QVariantMap QgsServiceAreaFromPointAlgorithm::processAlgorithm( const QVariantMa
mDirector->makeGraph( mBuilder.get(), QVector< QgsPointXY >() << startPoint, snappedPoints, feedback );

feedback->pushInfo( QObject::tr( "Calculating service area…" ) );
QgsGraph *graph = mBuilder->graph();
std::unique_ptr< QgsGraph> graph( mBuilder->takeGraph() );
int idxStart = graph->findVertex( snappedPoints[0] );

QVector< int > tree;
QVector< double > costs;
QgsGraphAnalyzer::dijkstra( graph, idxStart, 0, &tree, &costs );
QgsGraphAnalyzer::dijkstra( graph.get(), idxStart, 0, &tree, &costs );

QgsMultiPointXY points;
QgsMultiPolylineXY lines;
Expand Down Expand Up @@ -214,6 +214,8 @@ QVariantMap QgsServiceAreaFromPointAlgorithm::processAlgorithm( const QVariantMa
}
} // costs

upperBoundary.reserve( nodes.size() );
lowerBoundary.reserve( nodes.size() );
for ( int i : nodes )
{
upperBoundary.push_back( graph->vertex( graph->edge( tree.at( i ) ).toVertex() ).point() );
Expand Down
6 changes: 3 additions & 3 deletions src/analysis/processing/qgsalgorithmshortestpathlayertopoint.cpp
View file
Expand Up @@ -87,7 +87,7 @@ QVariantMap QgsShortestPathLayerToPointAlgorithm::processAlgorithm( const QVaria
mDirector->makeGraph( mBuilder.get(), points, snappedPoints, feedback );

feedback->pushInfo( QObject::tr( "Calculating shortest paths…" ) );
QgsGraph *graph = mBuilder->graph();
std::unique_ptr< QgsGraph > graph( mBuilder->takeGraph() );
int idxEnd = graph->findVertex( snappedPoints[0] );
int idxStart;
int currentIdx;
Expand All @@ -102,7 +102,7 @@ QVariantMap QgsShortestPathLayerToPointAlgorithm::processAlgorithm( const QVaria
feat.setFields( fields );
QgsAttributes attributes;

int step = points.size() > 0 ? 100.0 / points.size() : 1;
const double step = points.size() > 0 ? 100.0 / points.size() : 1;
for ( int i = 1; i < points.size(); i++ )
{
if ( feedback->isCanceled() )
Expand All @@ -111,7 +111,7 @@ QVariantMap QgsShortestPathLayerToPointAlgorithm::processAlgorithm( const QVaria
}

idxStart = graph->findVertex( snappedPoints[i] );
QgsGraphAnalyzer::dijkstra( graph, idxStart, 0, &tree, &costs );
QgsGraphAnalyzer::dijkstra( graph.get(), idxStart, 0, &tree, &costs );

if ( tree.at( idxEnd ) == -1 )
{
Expand Down
6 changes: 3 additions & 3 deletions src/analysis/processing/qgsalgorithmshortestpathpointtolayer.cpp
View file
Expand Up @@ -87,13 +87,13 @@ QVariantMap QgsShortestPathPointToLayerAlgorithm::processAlgorithm( const QVaria
mDirector->makeGraph( mBuilder.get(), points, snappedPoints, feedback );

feedback->pushInfo( QObject::tr( "Calculating shortest paths…" ) );
QgsGraph *graph = mBuilder->graph();
std::unique_ptr< QgsGraph > graph( mBuilder->takeGraph() );
int idxStart = graph->findVertex( snappedPoints[0] );
int idxEnd;

QVector< int > tree;
QVector< double > costs;
QgsGraphAnalyzer::dijkstra( graph, idxStart, 0, &tree, &costs );
QgsGraphAnalyzer::dijkstra( graph.get(), idxStart, 0, &tree, &costs );

QVector<QgsPointXY> route;
double cost;
Expand All @@ -102,7 +102,7 @@ QVariantMap QgsShortestPathPointToLayerAlgorithm::processAlgorithm( const QVaria
feat.setFields( fields );
QgsAttributes attributes;

int step = points.size() > 0 ? 100.0 / points.size() : 1;
const double step = points.size() > 0 ? 100.0 / points.size() : 1;
for ( int i = 1; i < points.size(); i++ )
{
if ( feedback->isCanceled() )
Expand Down
4 changes: 2 additions & 2 deletions src/analysis/processing/qgsalgorithmshortestpathpointtopoint.cpp
View file
Expand Up @@ -80,13 +80,13 @@ QVariantMap QgsShortestPathPointToPointAlgorithm::processAlgorithm( const QVaria
mDirector->makeGraph( mBuilder.get(), points, snappedPoints, feedback );

feedback->pushInfo( QObject::tr( "Calculating shortest path…" ) );
QgsGraph *graph = mBuilder->graph();
std::unique_ptr< QgsGraph > graph( mBuilder->takeGraph() );
int idxStart = graph->findVertex( snappedPoints[0] );
int idxEnd = graph->findVertex( snappedPoints[1] );

QVector< int > tree;
QVector< double > costs;
QgsGraphAnalyzer::dijkstra( graph, idxStart, 0, &tree, &costs );
QgsGraphAnalyzer::dijkstra( graph.get(), idxStart, 0, &tree, &costs );

if ( tree.at( idxEnd ) == -1 )
{
Expand Down
18 changes: 9 additions & 9 deletions tests/src/analysis/testqgsnetworkanalysis.cpp
View file
Expand Up @@ -159,7 +159,7 @@ void TestQgsNetworkAnalysis::testBuild()
QVector<QgsPointXY > snapped;
director->makeGraph( builder.get(), QVector<QgsPointXY>() << QgsPointXY( 0, 0 ) << QgsPointXY( 10, 10 ), snapped );
QCOMPARE( snapped, QVector<QgsPointXY>() << QgsPointXY( 0, 0 ) << QgsPointXY( 10, 10 ) );
std::unique_ptr< QgsGraph > graph( builder->graph() );
std::unique_ptr< QgsGraph > graph( builder->takeGraph() );
QCOMPARE( graph->vertexCount(), 3 );
QCOMPARE( graph->edgeCount(), 4 );
QCOMPARE( graph->vertex( 0 ).point(), QgsPointXY( 0, 0 ) );
Expand Down Expand Up @@ -191,7 +191,7 @@ void TestQgsNetworkAnalysis::testBuild()
builder = std::make_unique< QgsGraphBuilder > ( network->sourceCrs(), true, 0 );
director->makeGraph( builder.get(), QVector<QgsPointXY>() << QgsPointXY( 0.2, 0.1 ) << QgsPointXY( 10.1, 9 ), snapped );
QCOMPARE( snapped, QVector<QgsPointXY>() << QgsPointXY( 0.2, 0.0 ) << QgsPointXY( 10.0, 9 ) );
graph.reset( builder->graph() );
graph.reset( builder->takeGraph() );
QCOMPARE( graph->vertexCount(), 5 );
QCOMPARE( graph->edgeCount(), 8 );

Expand All @@ -218,7 +218,7 @@ void TestQgsNetworkAnalysis::testBuildTolerance()

QVector<QgsPointXY > snapped;
director->makeGraph( builder.get(), QVector<QgsPointXY>(), snapped );
std::unique_ptr< QgsGraph > graph( builder->graph() );
std::unique_ptr< QgsGraph > graph( builder->takeGraph() );
QCOMPARE( graph->vertexCount(), 5 );
QCOMPARE( graph->edgeCount(), 6 );
QCOMPARE( graph->vertex( 0 ).point(), QgsPointXY( 0, 0 ) );
Expand Down Expand Up @@ -252,7 +252,7 @@ void TestQgsNetworkAnalysis::testBuildTolerance()

builder = std::make_unique< QgsGraphBuilder > ( network->sourceCrs(), true, tolerance );
director->makeGraph( builder.get(), QVector<QgsPointXY>(), snapped );
graph.reset( builder->graph() );
graph.reset( builder->takeGraph() );
QCOMPARE( graph->vertexCount(), 4 );
QCOMPARE( graph->edgeCount(), 6 );
QCOMPARE( graph->vertex( 0 ).point(), QgsPointXY( 0, 0 ) );
Expand Down Expand Up @@ -332,7 +332,7 @@ void TestQgsNetworkAnalysis::dijkkjkjkskkjsktra()

QVector<QgsPointXY > snapped;
director->makeGraph( builder.get(), QVector<QgsPointXY>(), snapped );
std::unique_ptr< QgsGraph > graph( builder->graph() );
std::unique_ptr< QgsGraph > graph( builder->takeGraph() );

int startVertexIdx = graph->findVertex( QgsPointXY( 20, -10 ) );
QVERIFY( startVertexIdx != -1 );
Expand Down Expand Up @@ -384,7 +384,7 @@ void TestQgsNetworkAnalysis::dijkkjkjkskkjsktra()
director->addStrategy( strategy.release() );
builder = std::make_unique< QgsGraphBuilder > ( network->sourceCrs(), true, 0 );
director->makeGraph( builder.get(), QVector<QgsPointXY>(), snapped );
graph.reset( builder->graph() );
graph.reset( builder->takeGraph() );
startVertexIdx = graph->findVertex( QgsPointXY( 0, 0 ) );
QVERIFY( startVertexIdx != -1 );
resultTree.clear();
Expand Down Expand Up @@ -427,7 +427,7 @@ void TestQgsNetworkAnalysis::dijkkjkjkskkjsktra()
director->addStrategy( strategy.release() );
builder = std::make_unique< QgsGraphBuilder > ( network->sourceCrs(), true, 0 );
director->makeGraph( builder.get(), QVector<QgsPointXY>(), snapped );
graph.reset( builder->graph() );
graph.reset( builder->takeGraph() );
startVertexIdx = graph->findVertex( QgsPointXY( 10, 10 ) );
QVERIFY( startVertexIdx != -1 );
resultTree.clear();
Expand Down Expand Up @@ -494,7 +494,7 @@ void TestQgsNetworkAnalysis::testRouteFail()

QVector<QgsPointXY > snapped;
director->makeGraph( builder.get(), QVector<QgsPointXY>() << start << end, snapped );
std::unique_ptr< QgsGraph > graph( builder->graph() );
std::unique_ptr< QgsGraph > graph( builder->takeGraph() );

QgsPointXY snappedStart = snapped.at( 0 );
QGSCOMPARENEAR( snappedStart.x(), 302131.3, 0.1 );
Expand Down Expand Up @@ -551,7 +551,7 @@ void TestQgsNetworkAnalysis::testRouteFail2()

QVector<QgsPointXY > snapped;
director->makeGraph( builder.get(), QVector<QgsPointXY>() << start << end, snapped );
std::unique_ptr< QgsGraph > graph( builder->graph() );
std::unique_ptr< QgsGraph > graph( builder->takeGraph() );

QgsPointXY snappedStart = snapped.at( 0 );
QGSCOMPARENEAR( snappedStart.x(), 11.250450, 0.000001 );
Expand Down

0 comments on commit 13e3d2f

Please sign in to comment.