[feature][processing] Initial port of Create Grid algorithm to C++ #32070

root676 · 2019-09-30T18:54:12Z

Description

This PR proposes the port of the Create Grid algorithm from Python to C++. No other functionality has been added to the algorithm, it is a 1:1 port of the original Python implementation. An overall algorithm speedup of multiple orders of magnitude can be achieved by this port.

Reviews and guidance (especially with prepare scripts, etc.) are very welcome as this is my first PR to the QGIS project.

Checklist

Original Python algorithm has been disabled.
Original Python algorithm has been dropped.
Commit messages are descriptive and explain the rationale for changes
Commits which fix bugs include Fixes #11111 at the bottom of the commit message
I have read the QGIS Coding Standards and this PR complies with them
New unit tests have been added for core changes
I have run the scripts/prepare-commit.sh script before each commit
I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment

This commit includes the source files for the ported algorithm, integration into cmake and disabling (not yet dropping) the python algorithm.

nyalldawson

Looking great!

A couple of formatting changes needed, and I've made some extra suggestions on potential improvements we could add. Note that these aren't directly related to your c++ port, but would be nice to address while we're aiming for a super-fast algorithm!

nyalldawson · 2019-09-30T23:01:42Z

python/plugins/processing/algs/qgis/QgisAlgorithmProvider.py

@@ -180,7 +179,7 @@ def getAlgs(self):
                FindProjection(),
                GeometryByExpression(),
                GeometryConvert(),
-                Grid(),
+                #Grid(),


You can totally remove this line

src/analysis/processing/qgsalgorithmgrid.cpp

nyalldawson · 2019-09-30T23:03:43Z

src/analysis/processing/qgsalgorithmgrid.cpp

+  addParameter( new QgsProcessingParameterExtent(QStringLiteral("EXTENT"), QObject::tr( "Grid extent" )));
+
+  //add Distance Parameter for horizontal and vertical spacing
+  addParameter( new QgsProcessingParameterDistance( QStringLiteral( "HSPACING"), QObject::tr( "Horizontal Spacing" ), 1, QString( "CRS" ), false, 0, 1000000000.0));


Suggested change

addParameter( new QgsProcessingParameterDistance( QStringLiteral( "HSPACING"), QObject::tr( "Horizontal Spacing" ), 1, QString( "CRS" ), false, 0, 1000000000.0));

addParameter( new QgsProcessingParameterDistance( QStringLiteral( "HSPACING"), QObject::tr( "Horizontal spacing" ), 1, QStringLiteral( "CRS" ), false, 0, 1000000000.0));

nyalldawson · 2019-09-30T23:03:51Z

src/analysis/processing/qgsalgorithmgrid.cpp

+
+  //add Distance Parameter for horizontal and vertical spacing
+  addParameter( new QgsProcessingParameterDistance( QStringLiteral( "HSPACING"), QObject::tr( "Horizontal Spacing" ), 1, QString( "CRS" ), false, 0, 1000000000.0));
+  addParameter( new QgsProcessingParameterDistance( QStringLiteral( "VSPACING"), QObject::tr( "Vertical Spacing" ), 1, QString( "CRS" ), false, 0, 1000000000.0));


Suggested change

addParameter( new QgsProcessingParameterDistance( QStringLiteral( "VSPACING"), QObject::tr( "Vertical Spacing" ), 1, QString( "CRS" ), false, 0, 1000000000.0));

addParameter( new QgsProcessingParameterDistance( QStringLiteral( "VSPACING"), QObject::tr( "Vertical spacing" ), 1, QStringLiteral( "CRS" ), false, 0, 1000000000.0));

nyalldawson · 2019-09-30T23:04:27Z

src/analysis/processing/qgsalgorithmgrid.cpp

+  addParameter( new QgsProcessingParameterDistance( QStringLiteral( "VSPACING"), QObject::tr( "Vertical Spacing" ), 1, QString( "CRS" ), false, 0, 1000000000.0));
+
+  //add Distance Parameter for horizontal and vertical overlay
+  addParameter( new QgsProcessingParameterDistance( QStringLiteral( "HOVERLAY"), QObject::tr( "Horizontal Overlay" ), 0, QString( "CRS" ), false, 0, 1000000000.0));


Suggested change

addParameter( new QgsProcessingParameterDistance( QStringLiteral( "HOVERLAY"), QObject::tr( "Horizontal Overlay" ), 0, QString( "CRS" ), false, 0, 1000000000.0));

addParameter( new QgsProcessingParameterDistance( QStringLiteral( "HOVERLAY"), QObject::tr( "Horizontal overlay" ), 0, QStringLiteral( "CRS" ), false, 0, 1000000000.0));

nyalldawson · 2019-09-30T23:12:44Z

src/analysis/processing/qgsalgorithmgrid.cpp

+  {
+    for(int row = 0; row < rows; row++)
+    {
+      double x = mGridExtent.xMinimum() + (col * mHSpacing - col * mHOverlay);


This could potentially be moved up into the outer loop, as we don't need to calculate it for every row.

nyalldawson · 2019-09-30T23:14:25Z

src/analysis/processing/qgsalgorithmgrid.cpp

+      double x = mGridExtent.xMinimum() + (col * mHSpacing - col * mHOverlay);
+      double y = mGridExtent.yMaximum() - (row * mVSpacing - row * mVOverlay);
+
+      f.setGeometry(QgsGeometry().fromPointXY(QgsPoint( x, y )));


Might get a little speed boost by simplifying this to:

f.setGeometry( QgsGeometry( new QgsPoint( x, y ) ) );

nyalldawson · 2019-09-30T23:17:01Z

src/analysis/processing/qgsalgorithmgrid.cpp

+      id++;
+      cnt++;
+
+      if(std::fmod(static_cast<double>(cnt),cnt_log) == 0)


Another potential speed boost (optional!):

Instead of calculating the mod for every feature, we could have an int variable of "int lastProgress". Then here this would become something like:

int thisProgress = cnt / cellcnt; if ( thisProgress != lastProgress ) { lastProgress= thisProgress; feedback->setProgress( lastProgress ); }

nyalldawson · 2019-09-30T23:18:57Z

src/analysis/processing/qgsalgorithmgrid.cpp

+{
+  QgsFeature f = QgsFeature();
+
+  QVector<double> hSpace;


Much more efficient c++ would be to use a fixed array size here, e.g.

double hSpace[2]; if( mHOverlay > 0) { hSpace[0] = mHSpacing - mHOverlay; hSpace[1] = mHOverlay; }

etc

nyalldawson · 2019-09-30T23:20:35Z

src/analysis/processing/qgsalgorithmgrid.cpp

+    QVector<QgsPoint> lineNodes = QVector<QgsPoint>() << pt1 << pt2;
+
+    QgsPolyline line = QgsPolyline(lineNodes);
+    f.setGeometry(QgsGeometry::fromPolyline(line));


We can use an optimised line string constructor here (for raw speeeed!):

f.setGeometry( QgsGeometry( new QgsLineString( pt1, pt2 ) ) );

nyalldawson · 2019-09-30T23:29:25Z

Re the prepare-commit script -- you need to run this before you make the commit, and the files already need to be checked in to git. So there's two ways to do this:

manually add the files to git, run prepare-commit, and then commit
or
(better) add the prepare-commit script as a git pre commit hook.

In order to update the formatting post-commit, try running scripts/astyle-all.sh which will apply the formatting script to all files in the repo. After this, you'll need to run scripts/remove_temporary_files.sh to cleanup the mess astyle-all leaves behind.

Ping me if you run into difficulties here!

Overall, a fantastic contribution -- great stuff!!

alexbruy · 2019-10-02T05:24:59Z

src/analysis/processing/qgsalgorithmgrid.cpp

+
+bool QgsGridAlgorithm::prepareAlgorithm( const QVariantMap &parameters, QgsProcessingContext &context, QgsProcessingFeedback * )
+{
+  //retrieve grid type


Do we really need all these comments for obvious things?

alexbruy · 2019-10-02T05:27:04Z

@root676 there are some unrelated commits in your branch.

Co-Authored-By: Nyall Dawson <nyall.dawson@gmail.com>

root676 · 2019-10-02T16:02:11Z

@alexbruy thank you for the review! Yes - I have had a little trouble with the git branch, I have it now fixed locally and will update the PR as soon as I have the suggested changes implemented.

root676 · 2019-10-03T14:50:58Z

Hi! I have fine tuned the code according to the suggestions. Should I also delete the original Python algorithm file from the repository?

nyalldawson · 2019-10-09T21:37:18Z

@root676 #32182 will fix the overly sensitive attribute value check which is causing the travis test failure here

nyalldawson · 2019-10-09T21:37:31Z

Should I also delete the original Python algorithm file from the repository?

Yes please!

nyalldawson · 2019-10-09T21:37:48Z

python/plugins/processing/algs/help/qgis.yaml

-  The CRS of the output layer must be defined. The grid extent and the spacing values must be expressed in the coordinates and units of this CRS.
-
-  The top-left point (minX, maxY) is used as the reference point. That means that, at that point, an element is guaranteed to be placed. Unless the width and height of the selected extent is a multiple of the selected spacing, that is not true for the other points that define that extent.
+  This algorithm creates a vector layer with a grid covering a given extent. Elements in the grid can be points, lines or polygons.The size and/or placement of each element in the grid is defined using a horizontal and vertical spacing. The CRS of the output layer must be defined. The grid extent and the spacing values must be expressed in the coordinates and units of this CRS. The top-left point (minX, maxY) is used as the reference point. That means that, at that point, an element is guaranteed to be placed. Unless the width and height of the selected extent is a multiple of the selected spacing, that is not true for the other points that define that extent.


This can be totally removed -- it won't be used anymore

nyalldawson · 2019-10-09T23:33:26Z

Ok, travis is happy!

nyalldawson · 2019-10-09T23:34:09Z

Thanks for your hard work here @root676 !

[feature][processing] Initial Port of Create Grid algorithm to C++

baa5a92

This commit includes the source files for the ported algorithm, integration into cmake and disabling (not yet dropping) the python algorithm.

nyalldawson reviewed Sep 30, 2019

View reviewed changes

nyalldawson added Optimization I feel the need... the need for speed! Processing Relating to QGIS Processing framework or individual Processing algorithms labels Sep 30, 2019

alexbruy reviewed Oct 2, 2019

View reviewed changes

root676 and others added 8 commits October 2, 2019 16:56

fix algorithm name typo

0909277

Co-Authored-By: Nyall Dawson <nyall.dawson@gmail.com>

disable python algorithm

e79c2a4

Refactor parameter names and use QStringLiteral

04beb60

Remove paragaphs from helpstring

db029cc

update help string in source file

1502159

update conditional operators and rework switch statements

e8440e8

Set row, col, id and iterator variables to long long

ae8bfc6

Do not calculate the column coordinate every loop

876a09e

root676 added 3 commits October 3, 2019 16:09

Fine tune code for efficiency

45a7d26

strip source file from unnecessary comments

4730236

run prepare-commit.sh on Create Grid algorithm

4e3fd40

root676 force-pushed the processing_cpp_ports branch from 9d7ccc0 to 4e3fd40 Compare October 3, 2019 14:44

root676 added 5 commits October 5, 2019 09:44

fix unused progress variable

a13810f

change test signatures from qgis to native

41d4f20

fix long long typecast in point method

3788b64

fix std::ceil and id column

5b30ef5

fix std::ceil in createPointGrid()

8660586

nyalldawson reviewed Oct 9, 2019

View reviewed changes

nyalldawson closed this Oct 9, 2019

nyalldawson reopened this Oct 9, 2019

nyalldawson added the Squash! Remember to squash this PR, instead of merging or rebasing label Oct 9, 2019

nyalldawson merged commit 9e9ade3 into qgis:master Oct 9, 2019

root676 deleted the processing_cpp_ports branch October 25, 2019 21:01

This was referenced Jan 25, 2020

Remove qgis:creategrid python alg help #34027

Merged

[3.10] Fix "Invalid algorithm ID for menu: qgis:creategrid" #34026

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature][processing] Initial port of Create Grid algorithm to C++ #32070

[feature][processing] Initial port of Create Grid algorithm to C++ #32070

root676 commented Sep 30, 2019 •

edited

nyalldawson left a comment

nyalldawson Sep 30, 2019

nyalldawson Sep 30, 2019

nyalldawson Sep 30, 2019

nyalldawson Sep 30, 2019

nyalldawson Sep 30, 2019

nyalldawson Sep 30, 2019

nyalldawson Sep 30, 2019

nyalldawson Sep 30, 2019

nyalldawson Sep 30, 2019

nyalldawson commented Sep 30, 2019

alexbruy Oct 2, 2019

alexbruy commented Oct 2, 2019

root676 commented Oct 2, 2019

root676 commented Oct 3, 2019

nyalldawson commented Oct 9, 2019

nyalldawson commented Oct 9, 2019

nyalldawson Oct 9, 2019

nyalldawson commented Oct 9, 2019

nyalldawson commented Oct 9, 2019

	addParameter( new QgsProcessingParameterDistance( QStringLiteral( "HSPACING"), QObject::tr( "Horizontal Spacing" ), 1, QString( "CRS" ), false, 0, 1000000000.0));
	addParameter( new QgsProcessingParameterDistance( QStringLiteral( "HSPACING"), QObject::tr( "Horizontal spacing" ), 1, QStringLiteral( "CRS" ), false, 0, 1000000000.0));

[feature][processing] Initial port of Create Grid algorithm to C++ #32070

[feature][processing] Initial port of Create Grid algorithm to C++ #32070

Conversation

root676 commented Sep 30, 2019 • edited

Description

Checklist

nyalldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyalldawson commented Sep 30, 2019

Choose a reason for hiding this comment

alexbruy commented Oct 2, 2019

root676 commented Oct 2, 2019

root676 commented Oct 3, 2019

nyalldawson commented Oct 9, 2019

nyalldawson commented Oct 9, 2019

Choose a reason for hiding this comment

nyalldawson commented Oct 9, 2019

nyalldawson commented Oct 9, 2019

root676 commented Sep 30, 2019 •

edited