Skip to content
Permalink
Browse files
Rework API and improve memory handling of graph objects
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 authored and github-actions committed Jul 20, 2021
1 parent 1c3c5cd commit 35f3ffdb8c4efa32a8914f28e5ce6037a8534b9e
@@ -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

};
@@ -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 )
{
@@ -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;
}
@@ -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;
@@ -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 ) );
@@ -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() )
@@ -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;
@@ -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() );
@@ -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;
@@ -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() );
@@ -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;
@@ -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() )
@@ -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 )
{
@@ -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;
@@ -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() )
@@ -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 )
{
@@ -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 ) );
@@ -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 );

@@ -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 ) );
@@ -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 ) );
@@ -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 );
@@ -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();
@@ -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();
@@ -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 );
@@ -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 );

0 comments on commit 35f3ffd

Please sign in to comment.