PMP: Accelerate isotropic remeshing #5507

afabri · 2021-03-02T14:01:28Z

Summary of Changes

vtuning in order to find small improvements to make.

Todo

Do tests with smaller than average edge length
Do test with larger than average edge length

Release Management

Affected package(s): Polygon_mesh_processing
License and copyright ownership: unchanged

afabri · 2021-03-02T14:03:57Z

I also replaced some std::vector with boost::container::small_vector (not committed) but it slowed things down.
When reading iphigenia.off I see std::getline() and CGAL::Euler::add_face() relatively high. That is not in isotropic remeshing, but let's mention it here.

afabri · 2021-03-02T16:17:50Z

@janetournois when I simplify (that is a target edge length larger than the average edge length) iphigenia.off, we spend 16% of the time in an exact collinearity test. Is that by accident or by construction?

I think there is a flaw. For the first hd the points t and q are the same vertex t.

BGL/include/CGAL/boost/graph/Euler_operations.h

afabri · 2021-03-02T17:16:00Z

@janetournois can we avoid this std::vector<std:vector<Vector_3>> ? For example just a vector per patch with one normal. And when the next normal of the same patch is computed, we do the test right away, and replace the one stored normal.

…etournois

…etournois please double-check this commit)

Intersections_3/include/CGAL/Intersections_3/internal/Bbox_3_Sphere_3_do_intersect.h

afabri · 2021-03-03T15:41:08Z

Filtered_kernel/include/CGAL/internal/Static_filters/Do_intersect_3.h

@@ -265,6 +264,7 @@ class Do_intersect_3
        if (double_tmp_result < -eps)
          return true;
        else {
+          // return true; // AF:  just to test conservative approach


@lrineau Maybe you have an idea for the following problem: We use this sphere/bbox test in the AABB tree for finding the closest triangle. While vtuning code I noticed that there are filter failures that is we do the test with exact arithmetic. I could well live with a true (that is overlap) in case of uncertainly. The question is, if we have to add such a functor to the kernel, or if we can add somehow a parameter.

That would be a sort of "possibly intersect" instead of "do intersect".

CGAL::internal::Static_filters_predicates::Do_intersect_3 could have a third template parameter (a non-type template argument, I suggest a dedicated enum, to avoid the curve of a Boolean parameter), with a default-value.

Then we can add a new functor name in the Kernel concept, and it would be implemented using either Do_intersect_3 or CGAL::internal::Static_filters_predicates::Do_intersect_3 with the special value as third template argument.

I might actually be more efficient to implement that possibly_intersect(sphere, bbox) by do_overlap(sphere.bbox(), bbox).

Note that I also mentioned the tag this morning with #4654 in mind where we also brought the tag onto the table to allow/forbid degenerate input.

To use the bbox of the sphere makes it 25% slower.

sloriot · 2021-03-18T14:31:34Z

Polygon_mesh_processing/examples/Polygon_mesh_processing/isotropic_remeshing_example.cpp

@@ -1,6 +1,7 @@
+#define EARLY


To be removed?

No longer needed. It was for an #ifdef EARLY which I had in the code.

sloriot · 2021-03-18T14:32:17Z

Polygon_mesh_processing/examples/Polygon_mesh_processing/isotropic_remeshing_example.cpp

+  std::cout << "Read: " << t.time() << " sec" << std::endl;
+  t.reset();
+
+  double target_edge_length = (argc > 2) ? std::stod(std::string(argv[2])) : 1.0;


Any reason to change the previous default?

By accident.

sloriot · 2021-03-18T14:35:07Z

...h_processing/include/CGAL/Polygon_mesh_processing/internal/Isotropic_remeshing/remesh_impl.h

+      for(halfedge_descriptor h : halfedges(mesh_))
+      {
+        vertex_descriptor t = target(h, mesh_);
+        put(degree, t, get(degree,t)+1);


@janetournois shall the degree depend only on the selected edges? This matters only for vertices on the boundary of a selection.

The degree of a vertex that stands on the boundary of the selection (i.e. on a PATCH_BORDER) should count only internal edges.
Here all edges are counted, right?

Note that in your code, IRC you were using degree() so Andreas' code does the same thing the original (to be checked)

But in the original code we call the degree() function for the vertex which does not distinguish either.

You are right.
valence(v) and target_valence(v) (which is 4 on borders and 6 elsewhere) both take into account only the "real" borders, not the patch borders.

It's not wrong, but maybe we can improve that, when a vertex stands at the limit between a patch to be remeshed and a patched to be left as is.
We can keep it this way for now.

sloriot · 2021-03-18T14:36:21Z

...h_processing/include/CGAL/Polygon_mesh_processing/internal/Isotropic_remeshing/remesh_impl.h

@@ -859,6 +875,12 @@ namespace internal {
        vvb -= 1;
        vvc += 1;
        vvd += 1;
+#ifdef VALE


Do you want to keep that ? you observe no gain when activating it?

In fact THAT is the code we must take.

sloriot · 2021-03-18T14:38:05Z

...h_processing/include/CGAL/Polygon_mesh_processing/internal/Isotropic_remeshing/remesh_impl.h

@@ -1879,8 +1913,10 @@ namespace internal {
    bool check_normals(const HalfedgeRange& hedges) const
    {
      std::size_t nb_patches = patch_id_to_index_map.size();
+      //std::vector<boost::optional<Vector_3> > normal_per_patch(nb_patches,boost::none);


Yes. I later replaced it with two vectors. By the way I am not sure if vector<bool> is a good idea, and if it should not be vector<int> instead.

why would it be bad?

Because it is more time consuming to change a bit inside an int.

but you checked with optional and vector<bool> was faster? I agree with int since nb_patches is not expected to be very large

I did not really see a difference in vtune but sticked to the second solution. I don''t agree on what you say on nb_patches. If it is the number of patches of the data set and not only the patches incident to the vertex it may be arbitrarily big.

it is the total number of surface patches indeed, but still it's likely not to be arbitrarily big. A hundred would already be a strong data set...

Because it is more time consuming to change a bit inside an int.

That is very cheap, comparable to a memory load or a memory store from L1 cache. Both are about 3-4 CPU cycles. As soon as you have L1 caches misses, the memory load/stores are at more costly that that operation (setting a bit in a word).

That should be benched, but I doubt you will see any penalty for using std::vector<bool> instead of std::vector<int>

janetournois · 2021-03-19T12:24:00Z

I made quite a few experiments with this branch, and all the results are good/similar to master.

About timings, the computation time (displayed by the demo) is on average 10 times faster than master on those experiments!

afabri · 2021-03-19T12:29:05Z

I have not planned to do more work on this. Note also that some improvements are not in the PMP package, so even other packages will profit from this PR.

sloriot · 2021-03-19T12:34:00Z

I think the next step with this function should be #805. The improvement is very nice! Congrats.

afabri · 2021-03-19T12:37:04Z

I think the next step with this function should be #805. The improvement is very nice! Congrats.

But in an independent PR I would suggest, so that we have this one quickly in the master branch.

sloriot · 2021-03-19T12:37:49Z

I think the next step with this function should be #805. The improvement is very nice! Congrats.

But in an independent PR I would suggest, so that we have this one quickly in the master branch.

Of course !

afabri · 2021-03-19T12:50:26Z

Polygon_mesh_processing/examples/Polygon_mesh_processing/isotropic_remeshing_example.cpp

+  out.precision(17);
+  //out << mesh << std::endl;
+  out.close();
+


@MaelRL just spotted this. @janetournois shall we write a mesh or shall we just go back to the original file and also kick out the timer.

Since it is the "main" example for isotropic remeshing, visible in the user manual, I would remove this and the timer too.

If you look at the other examples it seems that those who somehow modify the mesh or produce a mesh (union), at the end write the mesh to a file. We might unify. I also do not like the prefix or suffix example in the file names.

lrineau · 2021-03-29T12:27:43Z

@maxGimeno Both Sébastien and Mael approved this PR. It can be tested now.

…g-GF PMP: Accelerate isotropic remeshing

maxGimeno · 2021-04-06T12:45:59Z

successfully tested in https://cgal.geometryfactory.com/CGAL/testsuite/results-5.3-Ic-98.shtml

afabri added 3 commits March 2, 2021 12:25

Use flaat_set and put it outside the loop

19e5e4a

reserve(8) for incident faces to a vertex

10f454d

map -> unordered_map

5e2a580

afabri added the Pkg::PMP label Mar 2, 2021

afabri requested review from sloriot and janetournois March 2, 2021 14:01

janetournois approved these changes Mar 2, 2021

View reviewed changes

MaelRL added the Speed label Mar 2, 2021

MaelRL added this to the 5.3-beta milestone Mar 2, 2021

Important gain for Euler::add_face()

95ea922

lrineau reviewed Mar 2, 2021

View reviewed changes

BGL/include/CGAL/boost/graph/Euler_operations.h Outdated Show resolved Hide resolved

afabri added 5 commits March 2, 2021 21:30

small_vector has reserve()

a945c4f

Replace geometric by combinatorial test

22789f6

Early exit in do_intersect of Sphere/Bbox_3

2ac977b

Do not collect in a vector per patch, but do the tests directly (@jan…

82e5b3a

…etournois please double-check this commit)

Remove trailing whitespace

dad0287

lrineau reviewed Mar 3, 2021

View reviewed changes

Intersections_3/include/CGAL/Intersections_3/internal/Bbox_3_Sphere_3_do_intersect.h Outdated Show resolved Hide resolved

afabri added 3 commits March 3, 2021 15:00

No need for calling abs for a squared distance

e7f18df

Comment something experimental that sneaked in

431c5b8

Use certainly()

7785a9c

afabri commented Mar 3, 2021

View reviewed changes

afabri added 3 commits March 3, 2021 21:14

Add static filter for Equal_3::operator()(Vector_3, Null_vector)

4fab843

Reduce calls to target @sloriot please double check the correctness

2e3bfa8

Replace vector with optional by two vectors. No idea what is better yet

45265a7

MaelRL added the Not yet approved The feature or pull-request has not yet been approved. label Mar 4, 2021

afabri added 2 commits March 7, 2021 18:25

WIP: early exit in the static filter

b1c2dd8

WIP for not hard coding conservative. Todo: Use of Has_filtered

643810f

afabri added the Ready to be tested label Mar 10, 2021

afabri added 2 commits March 10, 2021 15:28

Simplify. todo: tighter bounds per case or factorization with a lambda

e6c2e5c

Avoid computing degree several times

da17681

sloriot reviewed Mar 18, 2021

View reviewed changes

remove #ifdef that should be defined

109a893

afabri commented Mar 19, 2021

View reviewed changes

Remove the timer and the output of the result

29a40dc

MaelRL requested a review from sloriot March 25, 2021 13:53

sloriot approved these changes Mar 25, 2021

View reviewed changes

MaelRL removed the Not yet approved The feature or pull-request has not yet been approved. label Mar 25, 2021

lrineau assigned maxGimeno Mar 29, 2021

maxGimeno added the Under Testing label Mar 29, 2021

sloriot added a commit to sloriot/cgal that referenced this pull request Apr 1, 2021

Merge pull request CGAL#5507 from afabri/PMP-vtune_isotropic_remeshin…

0db4b8d

…g-GF PMP: Accelerate isotropic remeshing

maxGimeno added the Tested label Apr 6, 2021

lrineau added rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' and removed Ready to be tested Under Testing labels Apr 6, 2021

lrineau merged commit 58ddf16 into CGAL:master Apr 6, 2021

lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Apr 7, 2021

lrineau deleted the PMP-vtune_isotropic_remeshing-GF branch April 7, 2021 07:58

lrineau added Merged_in_5.3-beta1 Merged_in_5.3 labels Jul 2, 2021

MaelRL mentioned this pull request Dec 20, 2024

EPECK defines Has_static_filters as "false" regardless of CGAL_NO_STATIC_FILTERS_FOR_LAZY_KERNEL #8671

Open

PMP: Accelerate isotropic remeshing #5507

PMP: Accelerate isotropic remeshing #5507

Conversation

afabri commented Mar 2, 2021 • edited Loading

Summary of Changes

Todo

Release Management

afabri commented Mar 2, 2021 • edited Loading

afabri commented Mar 2, 2021 • edited Loading

afabri commented Mar 2, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetournois commented Mar 19, 2021

afabri commented Mar 19, 2021

sloriot commented Mar 19, 2021

afabri commented Mar 19, 2021

sloriot commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrineau commented Mar 29, 2021

maxGimeno commented Apr 6, 2021

afabri commented Mar 2, 2021 •

edited

Loading

afabri commented Mar 2, 2021 •

edited

Loading

afabri commented Mar 2, 2021 •

edited

Loading

afabri commented Mar 2, 2021 •

edited

Loading