deprecate Surface_mesher package

sloriot commented 4 weeks ago

Surface_mesher has not been updated for a long time and all its functionalities (and more) have been implemented in Mesh_3. We now officially recommend to use Mesh_3 instead by deprecating the package.

Fixes #3237

Fixes #8211

janetournois commented 4 weeks ago

There is one issue left in CGAL/poisson_refine_triangulation.h , that still uses Surface_mesh_traits_generator_3

lrineau commented 4 weeks ago

I am quite surprised by this PR. I thought the urgency for CGAL-6.0-beta1 was to announce a deprecation:

at the top of the user- and ref- manuals,
in the CHANGES.md,
and maybe only for the entry-point function make_surface_mesh.

This PR is deprecating all the headers, and then everything that uses Surface_mesher internal now has to move in a rush, before CGAL-6.0-beta1. Is there a chance that:

we can converge to a stable state in, say, less than a week,
and that we have the confidence we will not introduce regression bugs in the process?

lrineau commented 4 weeks ago

In a conversion IRL, @afabri reminded me that this work is probably related to https://github.com/CGAL/cgal/pull/7891.

lrineau commented 3 weeks ago

With commit 6547f648968441b60f7bc4733bda687b69ad3fa5 I have repaired the reconstruction plugin, and tested it in CGAL Lab. It seems correct.

Screenshot_20240605_165157

On the console:

Computes Poisson implicit function using Conjugate Gradient...
Construction time of the sizing field: 1.2761150000000008 seconds
Total implicit function (triangulation+refinement+solver): 5.9814699999999998 seconds
Surface meshing...
/home/lrineau/Git/cgal-master/Poisson_surface_reconstruction_3/include/CGAL/Poisson_reconstruction_function.h:263:70: runtime error: reference binding to null pointer of type 'struct value_type'
Surface meshing: 10.593293999999998 seconds, 756 output vertices
Total reconstruction (implicit function + meshing): 16.730679000000002 seconds
Reconstruction error:
  max = 0.127707182737539
  avg = 0.0087130995426717341
Reconstruction achieved in 10.024125099182129s

Note that the UBAN sanitizer does not like that code, for a null cell pointer:

https://github.com/CGAL/cgal/blob/6547f648968441b60f7bc4733bda687b69ad3fa5/Poisson_surface_reconstruction_3/include/CGAL/Poisson_reconstruction_function.h#L261-L264

lrineau commented 3 weeks ago

Note that the UBAN sanitizer does not like that code, for a null cell pointer:

https://github.com/CGAL/cgal/blob/6547f648968441b60f7bc4733bda687b69ad3fa5/Poisson_surface_reconstruction_3/include/CGAL/Poisson_reconstruction_function.h#L261-L264

Fixed by 9cd4535b11877266e8085be3299ea81df4ed5123. Now:

https://github.com/CGAL/cgal/blob/9cd4535b11877266e8085be3299ea81df4ed5123/Poisson_surface_reconstruction_3/include/CGAL/Poisson_reconstruction_function.h#L261-L267

Maybe s_iterator_to in the compact container should have an overload for pointers, to avoid that extract test for null.

Edit: see the new issue #8260

lrineau commented 3 weeks ago

/build:wip-doc

github-actions[bot] commented 3 weeks ago

There was an error while building the doc:

/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor classcgal_multimap found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor sectionProjectionFunctionObjects found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor Chapter_STL_Extensions_for_CGAL found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_intro found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_doubly found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_compact found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_multi found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_hash found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_polyobject found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_uncertainty found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_complexity found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_defaults found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor secchecks found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_alteriung found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_control found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_customizing found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_failure_example found

https://github.com/CGAL/cgal/actions/runs/9388765120

lrineau commented 3 weeks ago

There was an error while building the doc:

/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor classcgal_multimap found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor sectionProjectionFunctionObjects found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor Chapter_STL_Extensions_for_CGAL found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_intro found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_doubly found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_compact found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_multi found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_hash found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_polyobject found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_uncertainty found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_complexity found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_defaults found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor secchecks found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_alteriung found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_control found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_customizing found
/home/runner/work/cgal/cgal/build_doc/doc_tags/STL_Extension.tag:5046: warning: Duplicate anchor stl_failure_example found

https://github.com/CGAL/cgal/actions/runs/9388765120

@sloriot Can you help? Or maybe @albert-github?

albert-github commented 3 weeks ago

It looks like that the STL_Extension.tag is mentioned twice in the used Doxyfile (or the content is contained in another tag file as well). It looks like that this is a doxygen 1.9.6 problem and is fixed in the doxygen 1.10.0 and later, looks like fixes https://github.com/doxygen/doxygen/pull/10323 it looks though that also 1.11.0 version contains a small adjustment fix https://github.com/doxygen/doxygen/issues/10850 i.e. https://github.com/doxygen/doxygen/pull/10854

In the 1.11.0 version it looks like the message cannot appear anymore except for the C++-20 modules

lrineau commented 3 weeks ago

/force-build:wip-doc

github-actions[bot] commented 3 weeks ago

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/8248/wip/Manual/index.html

lrineau commented 3 weeks ago

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/8248/wip/Manual/index.html

Nice! Now the doc builds without any errors.

It looks like that the STL_Extension.tag is mentioned twice in the used Doxyfile (or the content is contained in another tag file as well).

Thanks @albert-github. That was indeed the issue. I have sorted the two doc/*/dependencies files of this PR, so ease future duplication errors. See https://github.com/CGAL/cgal/pull/8248/commits/36c7ecb77375d03014f5571858c02d045a527381 and https://github.com/CGAL/cgal/pull/8248/commits/6c6814dd2fd876a330a7a1b855767dcd70a2b248.

lrineau commented 3 weeks ago

Issue with `Poisson_implicit_surface_3`

For the moment, in master, we have an improved point oracle for Surface_mesher, than is only used one example file: Poisson_surface_reconstruction_3/examples/Poisson_surface_reconstruction_3/poisson_reconstruction.cpp.

History

_Thanks to git log -p --reverse -S Poisson_implicit_surface_3 cgal/master._

The class template CGAL::Poisson_implicit_surface_3 (and its companion Poisson_implicit_surface_oracle_3) was introduced by commit b0ce738c82988226af8cdf7ce2f029a3a30e9574 (Sep 27, 2011, first merged in CGAL by commit 485d1bc5e0202ac54a512216473a2674be344962, Nov 20, 2012, for CGAL-4.2). In the same branch Surface_reconstruction_points_3-make_it_faster-afabri, there was also the following commits:
- 197870acffe4359940c107f31676fe5885007cff adding the use of that better oracle class to the poisson reconstruction demo (demo/Surface_reconstruction_points_3/PS_demo_poisson_plugin_cgal_code.cpp)
- eb1dc0775047fbed03d9f731c1e3a1b33755b3f5 adding the use of the improved oracle to the examples examples/Surface_reconstruction_points_3/poisson_reconstruction.cpp.
then 028b6cc505a4fe5e4fc0f226f21af275566d6793 (in CGAL-4.5):

remove the Point set demo as all functionalities are now in the Polyhedron demo

but the polyhedron demo never had the speed improvements
2016, @sgiraudot added the improved oracle in examples/Poisson_surface_reconstruction_3/tutorial_example.cpp, commit f052a95d2ab3a0bd6b15e28aed8d6c2721b6c369, but then removed that usage in b9730aa925cdaaeada768c129afd2afb63177ead in 2020.

lrineau commented 3 weeks ago

Issue with Poisson_implicit_surface_3

For the moment, in master, we have an improved point oracle for Surface_mesher, than is only used one example file: Poisson_surface_reconstruction_3/examples/Poisson_surface_reconstruction_3/poisson_reconstruction.cpp.

And that example is not in the documentation.

lrineau commented 3 weeks ago

Issue with Poisson_implicit_surface_3

The difference is in the associated oracle class. The Poisson function is queried via a hidden dedicated API:

https://github.com/CGAL/cgal/blob/2c13f1fe6bcb7496b5f5c68095790ae4f894f3dc/Poisson_surface_reconstruction_3/include/CGAL/Surface_mesher/Poisson_implicit_surface_oracle_3.h#L286-L292

And then, later in the bisection, those added lines:

https://github.com/CGAL/cgal/blob/2c13f1fe6bcb7496b5f5c68095790ae4f894f3dc/Poisson_surface_reconstruction_3/include/CGAL/Surface_mesher/Poisson_implicit_surface_oracle_3.h#L303-L310

Once the two endpoints of the segment are in the same cell, then there is no need to continue the bisection.

lrineau commented 3 weeks ago

Issue with Poisson_implicit_surface_3

The full diff is at https://gist.github.com/lrineau/f5408f2e0c57e35ebf13f48b4778db90

lrineau commented 3 weeks ago

@sloriot @afabri See 6daec19ce91987970959af9bd48042a8bf49c659. There are a lot more dependencies for Poisson reconstruction, now.

lrineau commented 3 weeks ago

/force-build:wip

github-actions[bot] commented 3 weeks ago

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/8248/wip/Manual/index.html

lrineau commented 3 weeks ago

In CGAL Lab, the dialog that opens with the action "Surface Reconstruction", still mention "Surface Mesher". But I do not know what it could be replaced with. "Mesh_3" and "3D Mesh Generation" both the same issue: they do not convey that the code is only returning a surface mesh, as a result.

Screenshot_20240607_162133 (1)

afabri commented 3 weeks ago

This is no problem. We use Mesh_3 as Surface Mesher. In fact even with Mesh_3 we somehow would lilke to have a function called make_surface_mesh.

lrineau commented 3 weeks ago

This is no problem. We use Mesh_3 as Surface Mesher. In fact even with Mesh_3 we somehow would lilke to have a function called make_surface_mesh.

I have created a new issue https://github.com/CGAL/cgal/issues/8269 to track that idea.

sloriot commented 3 weeks ago

Successfully tested in CGAL-6.0-Ic-264

CGAL / cgal