Refactor QgsCircularString::curveToLine #4746

strk · 2017-06-17T16:04:25Z

See https://issues.qgis.org/issues/16717
and https://issues.qgis.org/issues/16722

strk · 2017-08-11T16:09:02Z

This became both a bugfix and an enhancement.
Bugfix because it fixed https://issues.qgis.org/issues/16717 and https://issues.qgis.org/issues/16722
Enhancement because it adds truly symmetric curveToLine output

Do you see any reason to use the old behavior of output being non-symmetric ? As that would need a parameter...

strk · 2017-08-11T16:27:19Z

Another enhancement that could be added is avoiding duplicated vertices in a multi-arc curve, whereas right now there will be an additional duplicated point on every arc.

strk · 2017-08-11T16:54:09Z

Oh, one more note: the code before this change had a control structure ensuring that the control point in the input curve still appeared in the output curve. I obviously removed that check because it's clearly incompatible with a symmetric output. If there's any good reason to retain that point it should once again become a parameter to keep it. I cannot think of any good reason, but there's a test guarding for that, which is currently failing: https://travis-ci.org/qgis/QGIS/jobs/263558531#L1785

strk · 2017-08-22T10:31:09Z

@mhugent or @andreasneumann do you know why it was desired for control point to always be in output ? For a regularly-spaced output the control point may be just very close to previous or next vertex, introducing vicinities that are probably not wanted..

andreasneumann · 2017-08-22T11:25:25Z

@edigonzales can you help here? Or maybe @mhugent ?

strk · 2017-08-22T11:31:26Z

Commit da18565 restores control point addition (while avoiding a point duplication). Chances of nearby points are still there.

strk · 2017-08-22T12:44:56Z

Travis succeeds with Python 3.5 and fails with Python 3.3 but I don't see what's the error in here: https://travis-ci.org/qgis/QGIS/jobs/267145607

3nids · 2017-08-22T13:24:16Z

nothing Python related
https://dash.orfeo-toolbox.org/viewBuildError.php?buildid=287408

3nids · 2017-08-22T13:25:37Z

the build with Python 3.5 is for code layout (spelling, syntax, sip bindings, etc)

strk · 2017-08-22T16:01:50Z

@3nids can you tell what's missing in this case, for the build to fail ?

strk · 2017-08-22T16:35:50Z

got it, now that I followed the CDash link (sorry) - it would be great to see that link in cubital fonts in the logs of the build :)

strk · 2017-08-25T14:21:44Z

This PR should now be passing Travis, but I had to expect the control point to be in the output for all new tests I added. I don't really like to see that control point in, because it makes the output ugly in the best case (non same-length segments) and dangerous at worst (very small segments).

@edigonzales, @mhugent, @andreasneumann I'm still interested in your opinion about this: why is it desired to have control point in output ?

strk · 2017-08-25T14:59:39Z

I'm working on adding a parameter to make it optional to force or not inclusion of the control point, but I still think the default should be NOT to force it, do you agree ?

strk · 2017-08-25T15:10:47Z

now the test fails for an unrelated issue ...

strk · 2017-08-25T15:15:46Z

Rebased to latest-passing commit, to see if that fixes this unrelated error:

234: ======================================================================
234: FAIL: testInterpolateAngle (__main__.TestQgsGeometry)
234: test QgsGeometry.interpolateAngle()
234: ----------------------------------------------------------------------
234: Traceback (most recent call last):
234:   File "/usr/src/qgis/qgis-master/tests/src/python/test_qgsgeometry.py", line 3545, in testInterpolateAngle
234:     self.assertAlmostEqual(geom.interpolateAngle(5), 1.69120, places=3)
234: AssertionError: 1.6919297347154518 != 1.6912 within 3 places
234:
234: ======================================================================
234: FAIL: testLineLocatePoint (__main__.TestQgsGeometry)
234: test QgsGeometry.lineLocatePoint()
234: ----------------------------------------------------------------------
234: Traceback (most recent call last):
234:   File "/usr/src/qgis/qgis-master/tests/src/python/test_qgsgeometry.py", line 3501, in testLineLocatePoint
234:     self.assertAlmostEqual(geom.lineLocatePoint(point), 7.372, places=3)
234: AssertionError: 7.3773841496377 != 7.372 within 3 places
234:
234: ======================================================================

strk · 2017-08-25T15:56:41Z

Ok I surrender: the failure is not unlrelated. It's for "interpolateAngle" which ensures to always work on linearized curve. Then computes the angle parallel to the "curve" at the specified distance. It is expected that the number can be slightly different upon refactoring the curveToLine function. The other are similar: functions that operate on segmentation of curves.

I'll update the expected results. Then one day these functions will work directly on curves so that the results will be more precise

strk · 2017-08-25T16:10:07Z

Expected results updated, but I'd love to be able to have the control point not added by default before doing this, as that additional point may very well affect reuslts.

So say, the same curve (only defined with a different control-point) would end up having a different centroid, or interpolated angle, or located point. It isn't nice is it ?

Control point is any point on the arc, so could be everywhere, hardly with a meaning on itself other than defining the curve...

nyalldawson · 2017-08-25T17:09:49Z

Control point is any point on the arc, so could be everywhere, hardly with a meaning on itself other than defining the curve...

I'm +0 on this, but am not a heavy curved geometry user myself so don't know exactly what users want in this situation. In the absence of any reply from @mhugent or @andreasneumann I'd say your safe to go with your instinct and change the behavior.

Fixes qgis#16717 Fixes qgis#16722 Include tests

…ference curves These values change because they are computed on the *linearization* of those curves, and refactoring linearization codes results in slighly different values. NOTE: adding or not adding the control point would also affect these results

This reverts commit dae9d02.

strk · 2017-08-25T18:13:14Z

Ok rebased to fix conflicts and reverted to NOT include the control point, let's see how centroid/interpolateAngle/locatPoint change again with this (maybe those tests should be made more tolerant, btw)

There are more tests in dedicated file (testqgscurve.cpp)

strk · 2017-08-25T21:06:56Z

I'm considering this closed, no force of central point. Curves will be segmentized with segments of equal length.

nyalldawson · 2017-08-31T22:04:15Z

src/core/geometry/qgsgeometryutils.cpp

    bool addP2 = true;
    if ( qgsDoubleNear( circlePoint1.x(), circlePoint3.x() ) && qgsDoubleNear( circlePoint1.y(), circlePoint3.y() ) )
    {
      addP2 = false;
    }
-
-    for ( double angle = a1 + increment; angle < a3; angle += increment )
+    addP2 = false;


@strk this has left a lot of unreachable code in the loop below - was this intended?

strk · 2017-09-01T15:51:56Z

On Thu, Aug 31, 2017 at 10:04:21PM +0000, Nyall Dawson wrote: - for ( double angle = a1 + increment; angle < a3; angle += increment ) + addP2 = false; @strk this has left a lot of unreachable code in the loop below - was this intended?

I was intended in case anyone (@andreasneumann?) comes out with a reason why that control-point addition was required to be in output in the first place (there was a testcase guarding for that occurrence) To me, the code needs to go, but I feel like stepping on somebody's thought by removing that part (although nobody complained so far)

nyalldawson · 2017-09-02T04:23:00Z

@strk

was intended in case anyone (@andreasneumann?) comes out with
a reason why that control-point addition was required to be in output
in the first place (there was a testcase guarding for that occurrence)

To me, the code needs to go, but I feel like stepping on somebody's
thought by removing that part (although nobody complained so far)

Fair enough. In that case can we #ifdef 0 it out so that it's obvious to readers that the unused code is intended here? The static analysis tools will also have issues if we leave it in.

mhugent · 2017-09-02T08:22:51Z

@strk: control point on segmented curve was a requirement in the first implementation. I've asked back at the office that ordered this functionality. It seems it's ok for them if it is removed.

Drops the unused support for including control points in output. See qgis#4746 (comment)

strk · 2017-09-04T07:03:00Z

Thanks @mhugent - @nyalldawson please see #5120 5120

strk mentioned this pull request Jun 19, 2017

Improve documentation for curveToLine function #4728

Closed

strk self-assigned this Jul 21, 2017

strk force-pushed the bugfix/16717 branch from 8c77c4f to e04f063 Compare July 21, 2017 20:36

strk force-pushed the bugfix/16717 branch from e04f063 to 7677740 Compare August 11, 2017 16:01

strk changed the title ~~Add test for QgsCircularString::curveToLine~~ Refactor QgsCircularString::curveToLine Aug 11, 2017

strk force-pushed the bugfix/16717 branch from 7677740 to dd8d9d2 Compare August 11, 2017 16:06

strk added Bugfix Feature labels Aug 11, 2017

strk force-pushed the bugfix/16717 branch from fe5a036 to 319d639 Compare August 22, 2017 16:05

strk force-pushed the bugfix/16717 branch from acbe86d to dc2c8f7 Compare August 25, 2017 15:15

Refactor curveToLine to emit equidistant segments and fix some issues

ae462ce

Fixes qgis#16717 Fixes qgis#16722 Include tests

strk added 5 commits August 25, 2017 20:11

Fix build and multi-arc duplicated points

0f9320e

Remove test off-variable

dae9d02

Update expected results now that we're forcing control point in output

bf24dfa

Do not force inclusion of control point

6e6f74c

This reverts commit dae9d02.

strk force-pushed the bugfix/16717 branch from 9dcc08e to 6e6f74c Compare August 25, 2017 18:12

strk added 3 commits August 25, 2017 21:40

Do not expect forced control point in output

b6dbe15

Expect centroid obtained by not forcing inclusion of control point

64be83a

Remove line segmentation test checking for control point containement

d02efa0

There are more tests in dedicated file (testqgscurve.cpp)

strk merged commit 650cf6a into qgis:master Aug 25, 2017

strk deleted the bugfix/16717 branch August 25, 2017 21:07

nyalldawson reviewed Aug 31, 2017

View reviewed changes

strk added a commit to strk/QGIS that referenced this pull request Sep 4, 2017

Remove unreachable code from curveToLine

c555079

Drops the unused support for including control points in output. See qgis#4746 (comment)

strk mentioned this pull request Sep 4, 2017

Remove unreachable code from curveToLine #5120

Merged

This was referenced May 25, 2019

curveToLine introduces duplicated points #24616

Closed

Wrong result for curveToLine with max angle 70 on a semicircle #24621

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor QgsCircularString::curveToLine #4746

Refactor QgsCircularString::curveToLine #4746

strk commented Jun 17, 2017 •

edited

Loading

strk commented Aug 11, 2017

strk commented Aug 11, 2017

strk commented Aug 11, 2017

strk commented Aug 22, 2017

andreasneumann commented Aug 22, 2017

strk commented Aug 22, 2017

strk commented Aug 22, 2017

3nids commented Aug 22, 2017

3nids commented Aug 22, 2017

strk commented Aug 22, 2017

strk commented Aug 22, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

nyalldawson commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

nyalldawson Aug 31, 2017

strk commented Sep 1, 2017 via email

nyalldawson commented Sep 2, 2017

mhugent commented Sep 2, 2017

strk commented Sep 4, 2017

Refactor QgsCircularString::curveToLine #4746

Refactor QgsCircularString::curveToLine #4746

Conversation

strk commented Jun 17, 2017 • edited Loading

strk commented Aug 11, 2017

strk commented Aug 11, 2017

strk commented Aug 11, 2017

strk commented Aug 22, 2017

andreasneumann commented Aug 22, 2017

strk commented Aug 22, 2017

strk commented Aug 22, 2017

3nids commented Aug 22, 2017

3nids commented Aug 22, 2017

strk commented Aug 22, 2017

strk commented Aug 22, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

nyalldawson commented Aug 25, 2017

strk commented Aug 25, 2017

strk commented Aug 25, 2017

nyalldawson Aug 31, 2017

Choose a reason for hiding this comment

strk commented Sep 1, 2017 via email

nyalldawson commented Sep 2, 2017

mhugent commented Sep 2, 2017

strk commented Sep 4, 2017

strk commented Jun 17, 2017 •

edited

Loading