PMP: Polyline Snapping #2883

sgiraudot · 2018-02-28T09:24:01Z

Summary of Changes

This introduces an internal non-documented function to snap a set of polylines under a user-specified tolerance. I also put a test and a plug-in for the demo.

Release Management

Affected package(s): PMP

lrineau · 2018-02-28T13:47:05Z

For which release do we want this PR? 4.12 or 4.13?

sgiraudot · 2018-02-28T14:52:54Z

It's honestly no rush at all, so unless @sloriot (or you) can do a review and that the testsuite looks good soon enough for 4.12, it can easily be postponed to 4.13.

lrineau · 2018-03-12T11:17:37Z

This change is

lrineau · 2018-03-16T10:04:06Z

Warnings:

In file included from /Users/cgaltester/cgal_test/CGAL-4.12-Ic-200/cmake/platforms/x86-64_Darwin-13.0_Apple-clang-5.0_Release/test/Polyhedron_Demo/Plugins/PMP/Snap_polylines_plugin.cpp:7:
/Users/cgaltester/cgal_test/CGAL-4.12-Ic-200/include/CGAL/Polygon_mesh_processing/polyline_snapping.h:275:64: warning: unused typedef 'PointIterator' [-Wunused-local-typedef]
  typedef typename boost::range_const_iterator<Polyline>::type PointIterator;
                                                               ^
/Users/cgaltester/cgal_test/CGAL-4.12-Ic-200/include/CGAL/Polygon_mesh_processing/polyline_snapping.h:281:84: warning: unused typedef 'OutputType' [-Wunused-local-typedef]
  typedef cpp11::tuple<std::size_t, std::size_t, std::size_t, std::size_t, FT, FT> OutputType;
                                                                                   ^
/Users/cgaltester/cgal_test/CGAL-4.12-Ic-200/include/CGAL/Polygon_mesh_processing/polyline_snapping.h:273:69: warning: unused typedef 'PolylineIterator' [-Wunused-local-typedef]
  typedef typename boost::range_const_iterator<PolylineRange>::type PolylineIterator;
                                                                    ^
3 warnings generated.

Maybe have a look at the other columns, because there might be other warnings:
https://cgal.geometryfactory.com/CGAL/testsuite/results-4.12-Ic-200.shtml#Polyhedron_Demo

sgiraudot · 2018-03-20T08:48:37Z

@lrineau I see you push it to the testsuite (there are warnings I'm going to take care of). Do you want this to be in 4.12? As I said, I don't mind waiting for 4.13 for this.

lrineau · 2018-03-20T11:28:36Z

We said we would try to push it in CGAL-4.12. So I included it.

sgiraudot · 2018-03-21T06:41:57Z

Testsuite looks good.

sloriot · 2018-03-21T07:43:51Z

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/polyline_snapping.h

+  if (const Point_3* p = boost::get<Point_3>(&*intersection0))
+    p0 = *p;
+  else
+    return false;


I don't know what the function is supposed to do but I'm surprized you return false in case the segments are coplanar and collinear

sloriot

I don't see any specific treatment for:

segments smaller that threshold
consecutive segments (thus sharing a common endpoint)

Are those cases not an issue?

sloriot · 2018-03-21T07:44:52Z

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/polyline_snapping.h

+
+  Point_3 to_exact(const Inexact_point_3& p) const
+  {
+    return Point_3(p.x(), p.y(), p.z());


use Cartesian_converter

sloriot · 2018-03-21T07:45:01Z

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/polyline_snapping.h

+  }
+  Inexact_point_3 to_inexact(const Point_3& p, const Exact_to_double& etd) const
+  {
+    return Inexact_point_3(etd(p.x()),


use Cartesian_converter

sloriot · 2018-03-21T07:45:22Z

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/polyline_snapping.h

+
+public:
+
+  Exact_snapping()


sloriot · 2018-03-21T07:47:03Z

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/polyline_snapping.h

+    if (exact_snapping<Exact_kernel> (exact_s0, exact_s1, result))
+      return std::make_pair (to_inexact (result, exact_to_double), true);
+
+    return std::make_pair (Inexact_point_3(), false);


not sure it's worth is but it seems that exact_snapping should return an optional<Point_3>. I agree that you then have the conversion issue so maybe it is already the best choice.

sloriot · 2018-03-21T07:52:03Z

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/polyline_snapping.h

+                  polylines[b->info().first][b->info().second + 1]);
+    Segment_3 s2 (polylines[c->info().first][c->info().second],
+                  polylines[c->info().first][c->info().second + 1]);
+    if (CGAL::squared_distance (s1, s2) > squared_tolerance)


aren't you somehow doing twice the job with exact_snapping.intersection()? I mean couldn't you use new_point directly?

sloriot · 2018-03-21T07:53:11Z

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/polyline_snapping.h

+
+
+// Note this is not officially documented  
+/*!


I think this will show up in the documentation if ! is here. Anyway if not documented, the function should move in the experimental namespace.

lrineau · 2018-03-21T08:36:53Z

I have temporarily added the label "not yet approved", to avoid that I accidentally merge this PR, before the discussion between Sébastien and Simon is over.

lrineau · 2018-03-22T11:19:20Z

After a discussion with @sgiraudot, we have decided to postpone the test of this PR for CGAL-4.13.

To be done:

handle the case where the segments are colinear,
even when segment are not colinear, handle the cases where the point of a segment s1 that is closest to a segment s2 is not the intersection of projections.

Maybe Simon you want to add things. I think we should in the end modify the first message of the discussion, with a summary of the to-do list.

lrineau · 2018-06-07T11:02:21Z

@sgiraudot Have a look whether you want to refresh this PR and have it tested for CGAL-4.13.

sgiraudot · 2018-06-11T08:33:17Z

I don't think I'll have time to do the work that has to be done (summed up in your message of March 22nd) before the release, so we should probably postpone it.

maxGimeno · 2019-01-10T14:02:08Z

Ok, let's postpone it then

maxGimeno · 2019-06-17T07:53:51Z

Conflicts

sgiraudot · 2021-05-06T11:49:30Z

I rebased the branch and did some more work on it:

the algorithm now takes a Graph as input and directly modifies it
degenerate cases (collinear segments or coplanar segments) are well detected, and a first version of proximities detection is done
so far, some cases are not handled, specifically cases were a segment is completely included in the tolerance cylinder of another coplanar segment

Actually, it is really difficult to figure out a way to deal with all possible cases. The problem seems ill-posed, as the same polyline set will give very different results depending on how densely it's sampled: it seems we should consider polylines as a linear distribution and not a set of segments, otherwise we are dependent on vertex densities/positions. But on the other hand, I don't see an easy way to do it without the points.

Handling collinear segments gives also a sort of instability: two segments almost collinear would snap at 1 point, but as soon as they become collinear, they should snap as a common segment?

Depending on how the handle proximities, we can also get quite different outputs, see this coplanar example:

There does not seem to be a right answer. If you start considering cases where N polylines might snap at the same location with a combination of degenerate cases, it quickly becomes a nightmare.

In all cases, we also have the problem that applying the algorithm several times would sequentially collapse the polylines onto each other, which is not the case with non-degenerate cases (that would remain stable).

I'm wondering if we should not just make it a precondition that no segments are collinear/coplanar (or use random perturbations to make sure of that), but that might be too restrictive.

maxGimeno · 2021-05-06T11:56:03Z

Problems in license :

 There are modifications in licenses (check if all modified/added headers files contain a valid SPDX tag compatible with the one in package_info/PKG/license and that no license text is present):
 M Polygon_mesh_processing/package_info/Polygon_mesh_processing/license.txt

sgiraudot added the Pkg::PMP label Feb 28, 2018

sgiraudot requested a review from sloriot February 28, 2018 09:24

lrineau self-assigned this Mar 12, 2018

lrineau added Ready to be tested Under Testing CGAL 3D demo labels Mar 12, 2018

sgiraudot force-pushed the PMP-Polyline_snapping-GF branch from d1261b2 to ccec5d6 Compare March 20, 2018 08:59

sloriot reviewed Mar 21, 2018

View reviewed changes

lrineau added the Not yet approved The feature or pull-request has not yet been approved. label Mar 21, 2018

lrineau removed the Under Testing label Mar 22, 2018

lrineau added this to the 4.13-beta milestone Mar 22, 2018

lrineau added rm: not for next release Indicate to the release team that a PR should not be merged before the next release branch is forked and removed Under Testing labels Mar 22, 2018

sloriot added TODO and removed Ready to be tested labels May 15, 2018

maxGimeno modified the milestones: 4.14-beta, 4.15-beta Jan 10, 2019

lrineau added rm: not for next release Indicate to the release team that a PR should not be merged before the next release branch is forked and removed rm: not for next release Indicate to the release team that a PR should not be merged before the next release branch is forked labels Jan 28, 2019

lrineau removed the rm: not for next release Indicate to the release team that a PR should not be merged before the next release branch is forked label Apr 1, 2019

maxGimeno added the Conflicts label Jun 17, 2019

MaelRL modified the milestones: 5.0-beta, 5.1-beta Jul 3, 2019

MaelRL modified the milestones: 5.1-beta, Trash / Attic Feb 13, 2020

sgiraudot added 14 commits April 15, 2021 09:02

Add polyline snapping algorithm

739c4de

Add polyline snapping test

524efc1

Add polyline snapping plugin

86ae4ea

Internal documentation of polyline snapping

29ba539

Add test data for polyline snapping

6bddd12

Remove unused typedefs

3da5c36

Remove typename outside class

9d0cdb7

Refresh polyline snapping code

a899a31

Add variant that directly modified input polylines

8346d82

WIP: Graph based variant

82a6195

Graph based snapping + example

b07dc33

More on polyline snapping

a54b342

WIP: handle degenerate cases

a20fff9

Partial handling of coplanar cases

2d893c4

sgiraudot force-pushed the PMP-Polyline_snapping-GF branch from d08e032 to 2d893c4 Compare May 6, 2021 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PMP: Polyline Snapping #2883

PMP: Polyline Snapping #2883

sgiraudot commented Feb 28, 2018

lrineau commented Feb 28, 2018

sgiraudot commented Feb 28, 2018

lrineau commented Mar 12, 2018

lrineau commented Mar 16, 2018

sgiraudot commented Mar 20, 2018

lrineau commented Mar 20, 2018

sgiraudot commented Mar 21, 2018

sloriot Mar 21, 2018

sloriot left a comment

sloriot Mar 21, 2018

sloriot Mar 21, 2018

sloriot Mar 21, 2018

sloriot Mar 21, 2018

sloriot Mar 21, 2018

sloriot Mar 21, 2018

lrineau commented Mar 21, 2018

lrineau commented Mar 22, 2018 •

edited

Loading

lrineau commented Jun 7, 2018

sgiraudot commented Jun 11, 2018

maxGimeno commented Jan 10, 2019

maxGimeno commented Jun 17, 2019

sgiraudot commented May 6, 2021 •

edited

Loading

maxGimeno commented May 6, 2021


		public:

		Exact_snapping()

PMP: Polyline Snapping #2883

Are you sure you want to change the base?

PMP: Polyline Snapping #2883

Conversation

sgiraudot commented Feb 28, 2018

Summary of Changes

Release Management

lrineau commented Feb 28, 2018

sgiraudot commented Feb 28, 2018

lrineau commented Mar 12, 2018

lrineau commented Mar 16, 2018

sgiraudot commented Mar 20, 2018

lrineau commented Mar 20, 2018

sgiraudot commented Mar 21, 2018

sloriot Mar 21, 2018

Choose a reason for hiding this comment

sloriot left a comment

Choose a reason for hiding this comment

sloriot Mar 21, 2018

Choose a reason for hiding this comment

sloriot Mar 21, 2018

Choose a reason for hiding this comment

sloriot Mar 21, 2018

Choose a reason for hiding this comment

sloriot Mar 21, 2018

Choose a reason for hiding this comment

sloriot Mar 21, 2018

Choose a reason for hiding this comment

sloriot Mar 21, 2018

Choose a reason for hiding this comment

lrineau commented Mar 21, 2018

lrineau commented Mar 22, 2018 • edited Loading

lrineau commented Jun 7, 2018

sgiraudot commented Jun 11, 2018

maxGimeno commented Jan 10, 2019

maxGimeno commented Jun 17, 2019

sgiraudot commented May 6, 2021 • edited Loading

maxGimeno commented May 6, 2021

lrineau commented Mar 22, 2018 •

edited

Loading

sgiraudot commented May 6, 2021 •

edited

Loading