Grid overlay #3934

Merged
merged 9 commits into from Jan 19, 2017

Projects

None yet

3 participants

@lbartoletti
Contributor

Rewrite GridLine and GridPolygon; Add an overlay option; add full tests; rm old tests.

This pull request close #3546 and #3547.

This was referenced Jan 2, 2017
+ polyline.append(QgsPoint(x1, y))
+ polyline.append(QgsPoint(x2, y))
+
+ feat.setGeometry(QgsGeometry.fromPolyline(polyline))
@nyalldawson
nyalldawson Jan 3, 2017 Contributor

fromPolyline is slow. Can you restore the implementation which uses QgsPointV2/QgsLineString?

@lbartoletti
lbartoletti Jan 3, 2017 Contributor

I have restored the implementation that uses QgsPointV2 / QgsLineString for GridLine, I'll pull it tomorrow, this will depend on your answer to your second comment. Do you have the same suggestion for GridPolygon?

@@ -1,214 +0,0 @@
-<?xml version="1.0" encoding="utf-8" ?>
@nyalldawson
nyalldawson Jan 3, 2017 Contributor

It makes me nervous seeing the existing tests removed. Is there any reason these could not be retained? If not, have you checked to see what the differences in the resultant layers are?

@lbartoletti
lbartoletti Jan 3, 2017 Contributor

You're right, I have not quite explained my proposal.
There are some differences between the proposed alg and the original alg. Obviously this is a new option for overlay, but also:

  • The point of origin is located at the bottom left and not at the top left as on the original algo; It may be an error.
  • The original algo does not fully cover the extent for diamond/rectangle and does not necessarily end the grid for the lines, as user it is a point that I can regret.

That is why I proposed new tests.

However, I have prepared another version more current friendly where the existing tests are respected except for GridLine where I propose to totally grid the extent like GridPolygon.

lbartoletti added some commits Jan 5, 2017
@lbartoletti lbartoletti I have rewrite algs from current. Just add an overlay option and not
change current algorithm. Actual tests are respected and only add one
test for each algs with overlay option.
33b0dd3
@lbartoletti lbartoletti add correct grid_hexagon_overlay test file 6307d0b
@lbartoletti
Contributor

Travis says there is an error but, not for my test...
Start 4: ProcessingQgisAlgorithmsTest 4/280 Test #4: ProcessingQgisAlgorithmsTest ............ Passed 7.23 sec

But at the end
The job exceeded the maximum time limit for jobs, and has been terminated.

@m-kuhn m-kuhn closed this Jan 8, 2017
@m-kuhn m-kuhn reopened this Jan 8, 2017
@m-kuhn
Member
m-kuhn commented Jan 8, 2017

@lbartoletti that was an unrelated issue with our testing infrastructure, your tests are all fine

@m-kuhn m-kuhn added the Processing label Jan 8, 2017
@nyalldawson nyalldawson merged commit 01cd784 into qgis:master Jan 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nyalldawson
Contributor

Thanks @lbartoletti !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment