cgal icon indicating copy to clipboard operation
cgal copied to clipboard

Fix Sqrt(3) subdivision mask calculation for Epeck kernels and Windows 64-bit

Open dpapavas opened this issue 1 year ago • 0 comments

Compilation fails when building Sqrt(3) subdivision code using a Epeck kernel on Windows 64-bit, where size_t is long long unsigned int. I'm cross-compiling to WIN64 using MinGW, but, as far as I can see, the problem should exist for MSVC builds as well. You can use the following trivial test program to verify this:

#include <CGAL/Exact_predicates_exact_constructions_kernel.h>
#include <CGAL/subdivision_method_3.h>
#include <CGAL/Polyhedron_3.h>

typedef CGAL::Exact_predicates_exact_constructions_kernel Kernel;
typedef CGAL::Polyhedron_3<Kernel> PolygonMesh;

using namespace CGAL;
namespace params = CGAL::parameters;
int main(int argc, char **argv) {
  PolygonMesh pmesh;
  Subdivision_method_3::Sqrt3_subdivision(pmesh, params::number_of_iterations(1));
  Subdivision_method_3::DooSabin_subdivision(pmesh, params::number_of_iterations(1));
  return 0;
}

This builds fine for me under Linux, but using MingGW64, I get the following compilation error:

/usr/include/CGAL/Subdivision_method_3/subdivision_masks_3.h:497:52: error: ambiguous overload for ‘operator/’ (operand types are ‘CGAL::DooSabin_mask_3<CGAL::Polyhedron_3<CGAL::Epeck>, CGAL::internal::Point_accessor<CGAL::internal::In_place_list_iterator<CGAL::HalfedgeDS_in_place_list_vertex<CGAL::I_Polyhedron_vertex<CGAL::HalfedgeDS_vertex_base<CGAL::HalfedgeDS_list_types<CGAL::Epeck, CGAL::I_Polyhedron_derived_items_3<CGAL::Polyhedron_items_3>, std::allocator<int> >, CGAL::Boolean_tag<true>, CGAL::Point_3<CGAL::Epeck> > > >, std::allocator<CGAL::HalfedgeDS_in_place_list_vertex<CGAL::I_Polyhedron_vertex<CGAL::HalfedgeDS_vertex_base<CGAL::HalfedgeDS_list_types<CGAL::Epeck, CGAL::I_Polyhedron_derived_items_3<CGAL::Polyhedron_items_3>, std::allocator<int> >, CGAL::Boolean_tag<true>, CGAL::Point_3<CGAL::Epeck> > > > > >, CGAL::Point_3<CGAL::Epeck>, CGAL::Point_3<CGAL::Epeck>&, false> >::FT’ {aka ‘CGAL::Lazy_exact_nt<__gmp_expr<__mpq_struct [1], __mpq_struct [1]> >’} and ‘size_t’ {aka ‘long unsigned int’})
  497 |         else a = (FT) (3+2*std::cos(2*k*CGAL_PI/n))/n;
      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

This seems to be similar to #6100, and so is the fix and rationale. The change to a smaller integer type should be safe as far as I can see, since the integer holds the vertex degree, which I think can be assumed not to reach several billions.

dpapavas avatar Sep 07 '22 09:09 dpapavas