Move implementation of implicit_function() into .cc file

[fdrmrc]

This moves the implementation of GridGenerator::implicit_function() inside grid_generator.cc and adds the corresponding stub.

To fix some issues with headers, this depends on #13929 , only be572d6 should be looked at.

[drwells]

This fixes part of #13926, but, in general, we need to avoid including any non-forwarded CGAL headers in our own headers to fully fix the problem.

To that end - could you move CGAL::AdditionalData into a separate header and define its constructor in a source file? That will completely eliminate the need for any CGAL stuff in GridGenerator.

[masterleinad]

Also please rebase on top of master now.

[masterleinad]

/rebuild

[drwells]

👍

[drwells]

I double checked this and I don't understand why these are the default values. In particular, in my installed copy of CGAL/boost/graph/Named_function_parameters.h that function is defined as

bool inline is_default_parameter(const internal_np::Param_not_found&)
{
  return true;
}

template <class T>
bool is_default_parameter(const T&)
{
  return false;
}

i.e., these are always true which will be converted to 1 - why do we do this?

[drwells]

We also don't do this in 2D.

[luca-heltai]

I thought this is what the CGAL::Mesh_criteria_3 uses as default values. We don't do this in 2d because in 2d we use Surface_mesh_criteria, which takes double values as input, and not named parameters. @fdrmrc, can you double check that this is actually the case? I seem to remember that the default should be something like cgal_ignored, which is not the same as a default_parameter when converted to double.

[fdrmrc]

My original issue was that this ignored value (also written in CGAL documentation) is not available in user codes. However, inside CGAL::Mesh_Criteria_3 they have:

https://github.com/CGAL/cgal/blob/a8e87526348d2d1faa2ea893747314853ee6868d/Mesh_3/include/CGAL/Mesh_criteria_3.h#L97-L111

which suggests to replace my CGAL::parameters::is_default_parameter(true) with their default (doubles) values, as we're allowing only doubles so far. All cgal tests are still okay.

I've just rebased and force pushed this change, but I'm okay with other options as well. The best choice would clearly be to allow named parameters, but that is not something I can fix (and test) in next hours.

[luca-heltai]

Looking at the lines you pointed out, I think we should replace the default values with std::limits::max for the first guy, and zero for all the others.

[fdrmrc]

Sure, that one was added in one the next PRs. I'll add it here now.

[tamiko]

I can confirm that moving the CGAL headers into the one compilation unit reduces the bogus misleading indentation warning to one compilation unit.

Incidentally this improves parsing and compilation of a lot of compilation units. In numbers: Compiling the entire library (debug configuration only, full dependencies) with this pull request took 5:59 min. Without this pull request I get 7:16 min.

[drwells]

Incidentally this improves parsing and compilation of a lot of compilation units. In numbers: Compiling the entire library (debug configuration only, full dependencies) with this pull request took 5:59 min. Without this pull request I get 7:16 min.

I believe it. We should try hard to avoid putting external library headers in our own headers for precisely this reason.