Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cgal corefinement #111

Closed
wants to merge 9 commits into from
Closed

Cgal corefinement #111

wants to merge 9 commits into from

Conversation

fdrmrc
Copy link
Collaborator

@fdrmrc fdrmrc commented May 6, 2022

Just a draft to describe things a little.

  • The "Surface with edge"d part in the following image the is result of the corefinement test. It is just a CGAL::Surface_mesh.
    intersection_hedra_cube

  • Here, that precise surface is filled with tets (use medit visualize it). This has still to be added. Notice that this could be used to generate tetrahedral meshes, not to compute Quadratures over the result of BooleanOperations.
    Screenshot 2022-05-06 at 22 23 04

@fdrmrc fdrmrc force-pushed the cgal-corefinement branch 5 times, most recently from bc3b17d to 98970b3 Compare May 7, 2022 15:44
@fdrmrc
Copy link
Collaborator Author

fdrmrc commented May 7, 2022

  • Fixed output files
  • Added boolean operations between Surface_meshes
  • Added Quadrature formulas over volumetric region bounded by a Surface_mesh and simple test
  • Use coarse volumetric mesh to compute intersections(i.e. just compute the ConvexHull)

So far this works in 3D only. I'll try to make this work for <2,2> objects too.

Copy link
Owner

@luca-heltai luca-heltai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the PR on top of cgal-triangulation, instead of on top of master?

Comment on lines 39 to 44
enum class BooleanOperation
{
only_corefinement = 1 << 0,
union_op = 1 << 2,
intersection_op = 1 << 3,
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enum class BooleanOperation
{
only_corefinement = 1 << 0,
union_op = 1 << 2,
intersection_op = 1 << 3,
};
enum class BooleanOperation
{
none = 1 << 0,
union = 1 << 1,
intersection = 1 << 2,
};

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shalle we support also difference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks.

# include <CGAL/Surface_mesh.h>
# include <CGAL/Triangulation_3.h>
# include <CGAL/convex_hull_3.h>


DEAL_II_NAMESPACE_OPEN

namespace CGALWrappers
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a bit of documentation, with some examples from the CGAL web site, or computed with the tests.

Comment on lines +86 to +88
* Extensive examples can be found in the CGAL documentation at
* https://doc.cgal.org/latest/Polygon_mesh_processing/index.html#coref_coref_subsec
*
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put here some examples compute with this function, similar to the one you have in the comments.

Comment on lines 89 to 91
* @tparam CGALPointType
* @tparam dim
* @tparam spacedim
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the template paramaters comments.

Comment on lines 92 to 94
* @param[in] cell1
* @param[in] cell2
* @param[out] outsm
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param[in] cell1
* @param[in] cell2
* @param[out] outsm
* @param[in] cell1 First input cell
* @param[in] cell2 Second input cell
* @param[out] outsm Output surface mesh

/**
* Build a Quadrature<spacedim> formula to integrate over the volumetric
* region described by a CGAL::Surface_mesh, by filling the interior with
* simplices and collecting points and weights to initegrate over it.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the surface mesh is not closed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot think to a case where having a non-closed surface makes sense, after all we're intersecting deal.II cells in that function.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but does the function check that surface mesh is closed or not? You should document what happens in that case, and if the function throws an exception, you should state this here.

Comment on lines 114 to 115
* @tparam spacedim
* @tparam CGALPointType
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove parameters documentation

Comment on lines 116 to 118
* @param sm
* @param degree
* @return Quadrature<spacedim>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments input and output

* @return Quadrature<spacedim>
*/
template <int spacedim, typename CGALPointType, typename CGALTriangulation>
Quadrature<spacedim>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Quadrature<spacedim>
Quadrature<CGALTriangulation::Space_dimension()>

I'm not sure if this works, but this would allow us to remove the template parameter.

Copy link
Collaborator Author

@fdrmrc fdrmrc May 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not constexpr. I used CGALPointType::Ambient_dimension::value and this is constexpr. It works. Thanks!

*/
template <int spacedim, typename CGALPointType, typename CGALTriangulation>
Quadrature<spacedim>
quadrature_inside_region(const CGAL::Surface_mesh<CGALPointType> &sm,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we like the name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I don't, but I was not able to find a meaningful name. Maybe collect_quadratures_inside_surface ?

@luca-heltai
Copy link
Owner

Do you think we could make this more general by having it return a polygon soup? I would really love to have this as we did with parmoonolith, i.e., arbitrary cell VS arbitrary cell. Of course if it is not possible, then we simply need to specialize this for each possibility.

@fdrmrc
Copy link
Collaborator Author

fdrmrc commented May 8, 2022

Thanks for the comments! I addressed them and added a bit of documentation, except for the non-closed surface case.
For what concerns the Polygon soup, I'm looking into it right now.

Comment on lines +56 to +59
NONE = 1 << 0,
UNION = 1 << 2,
INTERSECTION = 1 << 3,
DIFFERENCE = 1 << 4,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deal.II notation: lowercase. If none doesn't work, use corefine as you had at the beginning. Aso, each one should be commented (using the ///< doxygen tag):

union = 1<<2, ///< Compute the union of the input arguments
intersection = 1<<2, ///< Compute the intersection...

* Performs corefinement and boolean operations on two deal.II cells and put
* the result in a CGAL::Surface_mesh object. If the cells are disjoint and
* you try to perform a boolean operation which makes no sense, like their
* intersection, an exception is thrown.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to throw an exception, or simply return an empty mesh? In the end, an empty set is a valid result for an intersection from a mathematical standpoint.

* @param[in] degree The degree of the quadrature formula
* @param[out] tr A CGAL triangulation storing the convex_hull of the volume
* bounded by the Surface_mesh
* @return Quadrature<spacedim> Collection of Quadratures. The sum of the weights equals to the volume of the polygonal region bounded by sm
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return Quadrature<spacedim> Collection of Quadratures. The sum of the weights equals to the volume of the polygonal region bounded by sm
* @return Quadrature<spacedim> Collection of Quadratures. The sum of the weights
* equals to the volume of the polygonal region bounded by sm

CGAL::Surface_mesh<CGALPointType> & outsm,
const BooleanOperation &boolean_operation = BooleanOperation::NONE);

/**
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You no longer have spacedim at your disposal. You should either document what spacedim is, or use directly CGALPointType::Ambient_dimension::value in place of spacedim in the documentation.

Copy link
Owner

@luca-heltai luca-heltai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

* cube and a tetrahedron, converted to a deal.II triangulation
*
* @image html intersection_cube_with_tetrahedron.png
*
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

Can you also add the same picture computin the union and the difference?

* the result in a CGAL::Surface_mesh object. If the cells are disjoint and
* you try to perform a boolean operation which makes no sense, like their
* intersection, an exception is thrown.
*
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add a line in which you describe who you subtract from whom. I'm imagining you do cell1 \setminus cell2, but you should document this.

Also in this case, you could have an empty result (i.e., cell1 is entirely included in cell2).

* Build a Quadrature<spacedim> formula to integrate over the volumetric
* region described by a CGAL::Surface_mesh, by filling the interior with
* simplices and collecting points and weights to integrate over it.
*
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment that explains what happens if the operation results in a empty set: you return an empty quadrature.

@luca-heltai
Copy link
Owner

There is an issue with the image:

[100%] Built target documentation
/home/runner/work/dealii/dealii/include/deal.II/cgal/surface_mesh.h:51: warning: image file intersection_cube_with_tetrahedron.png is not found in IMAGE_PATH: assuming external image.
Error: Process completed with exit code 1.

boolean_operation(
const typename Triangulation<dim, spacedim>::cell_iterator &cell1,
const typename Triangulation<dim, spacedim>::cell_iterator &cell2,
CGAL::Surface_mesh<CGALPointType> & outsm,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add mapping to the input argument list.

case BooleanOperation::DIFFERENCE:
res = PMP::corefine_and_compute_difference(sm1, sm2, outsm);
break;
case BooleanOperation::NONE:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not going to produce anything visible on the outside. outsm is not going to be touched by this function, and sm1 and sm2 will be destroied at the end of the call. We should document what will happen in this case, i.e., we return sm1, i.e., the corefinement of the first cell, therefore here you need to do outsm = std::move(sm1);


template <typename CGALPointType, typename CGALTriangulation>
Quadrature<CGALPointType::Ambient_dimension::value>
collect_quadratures_inside_surface(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a name like build_quadrature_on_convex_hull?

Also, remove GCALTriangulation from the input argument. Since you are only using it inside the function, just build it inside.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also provide a different version of this function for non-convex polyhedral meshes. Maybe adding a boolean argument like integrate_on_convex_hull which defaults to false that uses the algorithm you showed me yesterday, i.e., create a domain, then a mesh, then last lines.

const unsigned int degree,
CGALTriangulation & tr)
{
constexpr unsigned int spacedim = 3;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constexpr unsigned int spacedim = 3;
constexpr unsigned int spacedim = CGALPointType::Ambient_dimension::value;

QGaussSimplex<spacedim> quad(degree);
std::vector<dealii::Point<spacedim>> pts;
std::vector<double> wts;
for (const auto &f : tr.finite_cell_handles())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is different if spacedim is 3 or 2

@fdrmrc
Copy link
Collaborator Author

fdrmrc commented Jun 6, 2022

See upstream

@fdrmrc fdrmrc closed this Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants