rgcv / libcgal-julia

CGAL bindings for Julia
MIT License
7 stars 5 forks source link

Add 3D triangulation #3

Closed blegat closed 2 years ago

blegat commented 2 years ago

I tried to stay as close as possible to triangulation_2.cpp to make it easier to review and maintain. Current failure

In file included from /workspace/srcdir/src/triangulation_3.cpp:1:
/opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp: In instantiation of ‘static jlcxx::supertype<T>& jlcxx::UpCast<T>::apply(T&) [with T = CGAL::Delaunay_triangulation_3<CGAL::Simple_cartesian<CORE::Expr> >; jlcxx::supertype<T> = CGAL::Triangulation_3<CGAL::Simple_cartesian<CORE::Expr> >]’:
/opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp:934:5:   required from ‘void jlcxx::add_default_methods(jlcxx::Module&) [with T = CGAL::Delaunay_triangulation_3<CGAL::Simple_cartesian<CORE::Expr> >]’
/opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp:1183:27:   required from ‘jlcxx::TypeWrapper<T> jlcxx::Module::add_type_internal(const string&, JLSuperT*) [with T = CGAL::Delaunay_triangulation_3<CGAL::Simple_cartesian<CORE::Expr> >; SuperParametersT = jlcxx::ParameterList<>; JLSuperT = _jl_datatype_t; std::__cxx11::string = std::__cxx11::basic_string<char>]’
/opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp:1194:60:   required from ‘jlcxx::TypeWrapper<T> jlcxx::Module::add_type(const string&, JLSuperT*) [with T = CGAL::Delaunay_triangulation_3<CGAL::Simple_cartesian<CORE::Expr> >; SuperParametersT = jlcxx::ParameterList<>; JLSuperT = _jl_datatype_t; std::__cxx11::string = std::__cxx11::basic_string<char>]’
/workspace/srcdir/src/triangulation_3.cpp:79:52:   required from here
/opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp:134:12: error: invalid static_cast from type ‘CGAL::Delaunay_triangulation_3<CGAL::Simple_cartesian<CORE::Expr> >’ to type ‘jlcxx::supertype<CGAL::Delaunay_triangulation_3<CGAL::Simple_cartesian<CORE::Expr> > >&’ {aka ‘CGAL::Triangulation_3<CGAL::Simple_cartesian<CORE::Expr> >&’}
     return static_cast<supertype<T>&>(base);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
blegat commented 2 years ago

Here is a MWE for the content of src/triangulation_3.cpp to produce the error:

#include <jlcxx/module.hpp>

#include <julia.h>

#include "utils.hpp"
#include "triangulation.hpp"

namespace jlcxx {
  using namespace jlcgal;

  template<> struct SuperType<DTr_3>  { typedef Tr_3 type; };
}

namespace jlcgal {

void wrap_triangulation_3(jlcxx::Module& cgal) {
  const std::string tr_name = "Triangulation3";
  auto tr      = cgal.add_type<Tr_3>        (tr_name);

  const std::string dtr_name = "Delaunay" + tr_name;
  auto dtr = cgal.add_type<DTr_3>(dtr_name, tr.dt());
}

} // jlcgal

The error is:

[14:11:41] /opt/bin/x86_64-linux-gnu-libgfortran5-cxx11/x86_64-linux-gnu-g++ --sysroot=/opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/ -DBOOST_ALL_NO_LIB -DCGAL_USE_CORE=1 -DCGAL_USE_GMPXX=1 -DJULIA_ENABLE_THREADING -Dcgal_julia_inexact_EXPORTS -I/workspace/srcdir/include -isystem /workspace/destdir/include/julia -O3 -DNDEBUG -fPIC -frounding-math -std=gnu++17 -MD -MT CMakeFiles/cgal_julia_inexact.dir/src/voronoi_diagram_2.cpp.o -MF CMakeFiles/cgal_julia_inexact.dir/src/voronoi_diagram_2.cpp.o.d -o CMakeFiles/cgal_julia_inexact.dir/src/voronoi_diagram_2.cpp.o -c /workspace/srcdir/src/voronoi_diagram_2.cpp
[14:12:00] In file included from /workspace/srcdir/src/triangulation_3.cpp:1:
[14:12:00] /opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp: In instantiation of ‘static jlcxx::supertype<T>& jlcxx::UpCast<T>::apply(T&) [with T = CGAL::Delaunay_triangulation_3<CGAL::Epick>; jlcxx::supertype<T> = CGAL::Triangulation_3<CGAL::Epick>]’:
[14:12:00] /opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp:934:5:   required from ‘void jlcxx::add_default_methods(jlcxx::Module&) [with T = CGAL::Delaunay_triangulation_3<CGAL::Epick>]’
[14:12:00] /opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp:1183:27:   required from ‘jlcxx::TypeWrapper<T> jlcxx::Module::add_type_internal(const string&, JLSuperT*) [with T = CGAL::Delaunay_triangulation_3<CGAL::Epick>; SuperParametersT = jlcxx::ParameterList<>; JLSuperT = _jl_datatype_t; std::__cxx11::string = std::__cxx11::basic_string<char>]’
[14:12:00] /opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp:1194:60:   required from ‘jlcxx::TypeWrapper<T> jlcxx::Module::add_type(const string&, JLSuperT*) [with T = CGAL::Delaunay_triangulation_3<CGAL::Epick>; SuperParametersT = jlcxx::ParameterList<>; JLSuperT = _jl_datatype_t; std::__cxx11::string = std::__cxx11::basic_string<char>]’
[14:12:00] /workspace/srcdir/src/triangulation_3.cpp:21:52:   required from here
[14:12:00] /opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/local/include/jlcxx/module.hpp:134:12: error: invalid static_cast from type ‘CGAL::Delaunay_triangulation_3<CGAL::Epick>’ to type ‘jlcxx::supertype<CGAL::Delaunay_triangulation_3<CGAL::Epick> >&’ {aka ‘CGAL::Triangulation_3<CGAL::Epick>&’}
[14:12:00]      return static_cast<supertype<T>&>(base);
[14:12:00]             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[14:12:03] make[2]: *** [CMakeFiles/cgal_julia_inexact.dir/build.make:650: CMakeFiles/cgal_julia_inexact.dir/src/triangulation_3.cpp.o] Error 1

@barche any idea what's the issue ?

barche commented 2 years ago

The problem is in the DTr_3 supertype definition, it needs an extra template argument, changing it like this compiles for me:

template<> struct SuperType<DTr_3>  { typedef CGAL::Triangulation_3<Kernel, typename DTr_3::Triangulation_data_structure> type; };

See "Inherits from" here.

blegat commented 2 years ago

Thanks! It is compiling for me too now with the latest commit. @rgcv could this be merged ?

rgcv commented 2 years ago

Hey @blegat,

First off, thank you very much for your contribution. I've had the PR saved in my backlog and have been slowly accompanying its progress albeit from afar.

Since I've been far removed from the Julia ecosystem a while now, I'd like to review things a bit more cautiously. Namely, I haven't been up to speed on how the binary artefact build process over at yggdrasil has been. After merging this PR, testing within the context of CGAL.jl, and updating the lib's version, there's still the deployment of the package to take care of.

One other thing I definitely want to look into is updating the version of CGAL being depended on. Quite a few patches have already been released. That'd require deploying new versions of the lib via yggdrasil as well so we can depend on it. That could also result in some breaking changes, either with or without this PR's changes.

Finally, I'll also want to stop supporting Julia <1.6, but that shouldn't be too big an issue.

I haven't really allotted the time for these things and the package ended up neglected to no one's fault but my own. Sorry for the late reply and lack of activity, I'll see if I can get things rolling come this weekend or so at most.

Thanks again. And thank you @barche for the unblocker as well!

blegat commented 2 years ago

Thanks for the reply. Yes I think Julia <1.6 can be dropped, all packages have done so. Let me know if I can do anything to help!

rgcv commented 2 years ago

Sorry, I managed to not get around it this weekend. Seems like life keeps getting in the way. I'll be merging the PR regardless, it looks like it's in good condition. Hopefully I'll be able to look into it further soon.