CGAL
Refactor old code for new CGAL
Issue Details
I am interested to try and ultimately modify and use methods described in this paper. There is associated cpp code that I am trying to compile and run, and I am familiar with cpp so I should be able to. However, the paper and code was written in 2012, and since CGAL has changed in ways that I find difficult to understand. I have done my best to understand the abstraction that belies these issues, but ultimately I think I need someone to tell me if I am on the right track or wasting time.
The fundamental issue seems to be the use of weighted_point, whereas many functions included take weighted_point as input, I have issues during compile that no functions with these inputs are defined. I find this strange, but am currently reverting to overloading some function defined in CGAL headers, which is progressively removing compilation errors, but feels like the wrong thing to do. I believe I am missing something fundamental, and am probably better off changing top-level code, but am not sure how to do this to comply with modern CGAL headers.
Source Code
This is an example of the top level code, and the header functions called, as well as the compilation error.
Vertex_handle Scene::insert_vertex(const Point& point,
const FT weight,
const unsigned index)
{
Weighted_point wp(point, weight);
Vertex_handle vertex = m_rt.insert(wp);
if (vertex->get_index() != -1)
return Vertex_handle();
vertex->set_index(index);
return vertex;
}
Here, a weighted_point is created and passed to insert() within the Regular_triangulation_2.h header:
template < class Gt, class Tds >
typename Regular_triangulation_2<Gt,Tds>::Vertex_handle
Regular_triangulation_2<Gt,Tds>::
insert(const Weighted_point &p, Face_handle start)
{
Locate_type lt;
int li;
Face_handle loc = locate(p, lt, li, start);
return insert(p, lt, loc, li);
}
This results in a compilation error because there is no locate() function that can take a weighted_point. Now, I can overcome this by changing locate() call to locate(p.point(), lt, li, start), but again this is changing headers in CGAL, rather than the project. Similarly, I get errors from
calls to orientation() because there is no such function that accepts weighted_point. I can solve this more generally by amending the Triangulation_2.h header to overload it as such:
template <class Gt, class Tds >
inline
Orientation
Triangulation_2<Gt, Tds>::
orientation(const Weighted_point& p, const Weighted_point& q,const Weighted_point& r ) const
{
return geom_traits().orientation_2_object()(construct_point(p.point()),
construct_point(q.point()),
construct_point(r.point()));
}
But once again, this feels very strange. I'm SURE this is not an oversight in CGAL but something that I'm missing about how the top-level code is written.
Environment
NOTE: I had to amend the CMakeLists.txt from this old project to work with cmake 3.28.3. My modified file can be found in a public repo I made here. This is for compilation of ibnot in the directory bnot-src/ibnot/build of the source linked from the papers site
- Operating system (Linux, Ubuntu 24.04):
- Compiler: gcc 12.3.0
- Release or debug mode: No flags
- CGAL version: 5.6 (1050601000)
- Boost version: 1.83.0
- Cmake output:
$ cmake -Wno-dev ..
-- The C compiler identification is GNU 12.3.0
-- The CXX compiler identification is GNU 12.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Using header-only CGAL
-- Targeting Unix Makefiles
-- Using /usr/bin/c++ compiler.
-- Found GMP: /usr/lib/x86_64-linux-gnu/libgmp.so
-- Found MPFR: /usr/lib/x86_64-linux-gnu/libmpfr.so
-- Found Boost: /usr/lib/x86_64-linux-gnu/cmake/Boost-1.83.0/BoostConfig.cmake (found suitable version "1.83.0", minimum required is "1.66")
-- Boost include dirs: /usr/include
-- Boost libraries:
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Using gcc version 4 or later. Adding -frounding-math
-- Found OpenGL: /usr/lib/x86_64-linux-gnu/libOpenGL.so
-- Build type:
-- USING CXXFLAGS = ' '
-- USING EXEFLAGS = ' '
-- Requested component: Qt5
Hi,
Not sure I understand, at the moment it is using Point_2, are you suggesting this:
typedef typename Kernel::Point_2 Point_2;
I can't see how this would help, but I did try it, and I get predictable errors that primitives.h:69:11: error: ‘Point’ does not name a type. Do you mean I should also resolve these issues, because they go all the way into CGAL headers again:
/usr/include/CGAL/Triangulation_2.h:3101:19: error: invalid initialization of reference of type ‘const CGAL::Triangulation_2<CGAL::Epick, CGAL::Triangulation_data_structure_2<My_vertex_base<CGAL::Epick, CGAL::Regular_triangulation_vertex_base_2<CGAL::Epick> >, My_face_base<CGAL::Epick, CGAL::Regular_triangulation_face_base_2<CGAL::Epick> > > >::Point&’ {aka ‘const CGAL::Point_2<CGAL::Epick>&’} from expression of type ‘CGAL::Regular_triangulation_vertex_base_2<CGAL::Epick, CGAL::Triangulation_ds_vertex_base_2<CGAL::Triangulation_data_structure_2<My_vertex_base<CGAL::Epick, CGAL::Regular_triangulation_vertex_base_2<CGAL::Epick> >, My_face_base<CGAL::Epick, CGAL::Regular_triangulation_face_base_2<CGAL::Epick> > > > >::Weighted_point’ {aka ‘CGAL::Weighted_point_2<CGAL::Epick>’}
3101 | const Point & p1 = c->vertex( 1 )->point();
You must also update the rest of your vertex class to use Point_2 and not Point anymore.
Some explanations: The vertex base of a Regular_triangulation_2 must define a "Point" type that is equal to the traits' Weighted_point_2 type (the concept describing these requirements is: https://doc.cgal.org/latest/Triangulation_2/classRegularTriangulationVertexBase__2.html). The class CGAL::Regular_triangulation_vertex_base_2 does define this typedef, but you overwrite it in your derived vertex base with typedef Point_2 Point, which hides the Point typedef of CGAL::Regular_triangulation_vertex_base_2. Thus, if you redefine your typedefs to typedef Kernel::Point_2 Point_2 in your derived vertex (You must of course update the rest of your vertex class to use Point_2 and not Point anymore), the Point typedef will no longer be hidden and things will be fine.
@afabri has a branch to share with you, see https://github.com/bforsbe/bnot/pull/1.
In the branch Mael had suggested to also change the other typedefs by adding the suffix _2, but that is not needed, but just to make it more coherent.
Just out of curiosity: Is this still state of the art? What do you need it for? Would it make sense to have that in CGAL?
Not sure about state of the art, I am exploring methods to represent rasterized microscopy data as varaible-density point clouds for registration and motion embedding that might be easier to regularize than optical flow. But at this point I just want to see how well this performs under noise and variable contrast as opposed to image value alone.
With the PR referenced in the branch above, the aforementioned compilation errors are resolved. Thank you for the very clear and concise help. There are some further compilation errors that I will note here for posterity:
I had to revert to Qt5 on my system because I'm using the apt to install libcgal-qt5-dev and it complains about not finding the Qt6 component of CGAL.
With that, i had an issue finding suitesparseQR.hpp, which was solved by using apt to install
❯ sudo apt-get install libsuitesparse-dev
<...>
❯ dpkg -L libsuitesparse-dev | grep SuiteSparseQR.hpp
/usr/include/suitesparse/SuiteSparseQR.hpp
I also needed to manually include the path in the CMakeLists.txt:
<...>
if( CGAL_FOUND AND Qt5_FOUND AND OPENGL_FOUND AND CGAL_Qt5_FOUND )
<...>
include_directories(/usr/include/suitesparse)
<...>
At the moment, there are still some things to overcome. Currently:
In file included from /home/bjornf/Install/bnot/bnot-src/bnot-src/ibnot/window.cpp:17:
/home/bjornf/Install/bnot/bnot-src/bnot-src/ibnot/dialog.h:5:10: fatal error: ui_dialog.h: No such file or directory
5 | #include "ui_dialog.h"
| ^~~~~~~~~~~~~
NOTE: The remaining issue(s) do not pertain to the original question, but are mentioned here in case they pertain to CGAL changes since the original code was written, or in any of its dependencies, and as documentation. I leave it at the discretion of CGAL maintainers to close the issue as they deem fit.
Note that Suiteparse is used in the parameterization package, maybe that can be of help with the CMakeLists and the includes: https://github.com/CGAL/cgal/blob/master/Surface_mesh_parameterization/examples/Surface_mesh_parameterization/CMakeLists.txt
I'm using cmake 3.28.3, and even if I add
set( UI_FILES dialog.ui ibnot.ui )
<...>
add_executable( ibnot ${SRCS} ${DT_UI_FILES} ${DT_RESOURCE_FILES} ${UI_FILES} )
to the CMakeLists.txt the dialog.ui file does not get processed. I had to manually call
$ uic dialog.ui -o ui_dialog.h
after which it gets picked up by make. I also had to manually call
$ umoc dialog.h -o moc_dialog.cpp
add in CMakeLists.txt further change to
add_executable( ibnot ${SRCS} ${DT_UI_FILES} ${DT_RESOURCE_FILES} ${UI_FILES} moc_dialog.cpp)
My conclusion is that there is something preventing auto UIC and MOC from completing, but as I have not worked with this before I am not sure what.
In addition, QInputDialog appears to have changed some methods from getInteger() to getInt(). I also had to use apt to install libmetis-dev in order for the -lmetis flag to be found.
But it now compiles. :sun_with_face:
Adding files generated with moc and uic does not really make sense. We can have a look.
It would also be cool to have a code without GUI. What is the input and output of the method?
At the moment I am providing generic image input like jpeg and getting eps out, which suffices for testing simple things. But I agree, cmd-line would be very nice. Down the line the image format we tend to use for input is .mrc as here, and the easiest for to evaluate would be some format of .pdb or .cif which encodes atomic positions, possibly with connectivity that is effectively a graph. The fundamental step to completely asses the method for our purposes though, is that this needs to for 3D rasterized data.
Another part of my assessment is also performance, which would need to improve more than slightly. At the moment it seems to be running single-threaded CPU only. Not sure what facilities CGAL has for native acceleration, but this is comparatively simple infrastructure to add when needed (I hope). This is more my expertise than computational geometry, at least.
Hi @bforsbe
Can we do something more to help you with your project?
Note that the suitesparse mention triggered some cleaning in CGAL itself which might be relevant if you still need it, see https://github.com/CGAL/cgal/pull/8939.