CGAL/cgal

Several Problems in Reading OFF files

Opened this issue · 4 comments

efifogel commented

I've quickly searched the issues list and couldn't find anything similar, so I believe that most of not all of the content of this issue is relevant, but I could be wrong.

Issue Details

There are several problems with the way CGAL handles files in the OFF format. The code listed below is an attempt to cover those reported herel; thus, a bit longer than the typical minimal program. The program reads a file in the OFF format and displays it. It constructs an object the type of which is either an instance of the templates Polyhedron_3 or Surface_mesh, depending on whether the macro USE_SURFACE_MESH is defined; see Line 4 in the listing,

  1. The first line of a file in the OFF format has the letters OFF to mark the file type. (This is typically referred to as the magic string.) It turns out that the CGAL reader(s) accept either OFF or COFF as the magic string, where COFF indicates that the file has colors (with faces or vertices). The string COFF is not in the standard; thus, redundant. I can live with the fact the when COFF is indicated, colors are expected; however, I believe that if OFF is indicated, colors should still be handled properly. Currently, if OFF is indicated, colors seem to be ignored. I must add and say that I find it of high priority, because it has to do with standards,
  2. It seems that different pieces of code are used to read files in the OFF format based on the provided (named) parameters. This is, naturally bad practice, but here we actually get different behaviors. Assume that the definition of USE_SURFACE_MESH is commented out; that is, we are using Polyhedron_3. Assume we are only passing verbose and repair_polygon_soup (Line 61). Then, if the input data is inconsistent, the build of the mesh fails with a useful error message, e.g.,:

CGAL::Polyhedron_incremental_builder_3::
lookup_halfedge(): input error: facet 336 shares a halfedge from vertex 0 to vertex 1 with facet 0.

However, if we call read_polygon_mesh() and also provide a color map, the call returns false, but we do not get any error message. The split in the code occurs in the file Polyhedron_OFF_iostream.h, Line 69. When providing a color map we end up calling read_OFF_BGL(), and in turn CGAL::Euler::add_face(face, g); see Line 146 in File Generic_facegraph_builder.h. One quick fix is testing whether verbose is true, and if so, precede the call to add_face() with a check in the form of a call to can_add_face(). I think that if we use the Surface_mesh template we don't get any useful error messages whatsoever, so if we use different code, then it should be fixed there as well.

  1. The reference of CGAL::Polygon_mesh_processing::IO::read_polygon_mesh is missing the following options:

    • face_color_map
    • vertex_color_map
    • vertex_normal_map
    • vertex_texture_map

  2. While at it, it would be nice if the very simple example draw_polyhedron.cpp is enhanced with the ability to read and respect face colors as done here.

#include <iostream>
#include <string>

// #define USE_SURFACE_MESH

#include <CGAL/Exact_predicates_exact_constructions_kernel.h>
#if defined(USE_SURFACE_MESH)
#include <CGAL/Surface_mesh.h>
#include <CGAL/draw_surface_mesh.h>
#else
#include <CGAL/Polyhedron_3.h>
#include <CGAL/Polyhedron_traits_with_normals_3.h>
#include <CGAL/draw_polyhedron.h>
#endif

#include <CGAL/Polygon_mesh_processing/IO/polygon_mesh_io.h>
#include <CGAL/IO/Color.h>

#if ! defined(USE_SURFACE_MESH)
#include "Extended_polyhedron_items.h"
#endif

int main(int argc, char* argv[]) {
  using Kernel = CGAL::Exact_predicates_exact_constructions_kernel;

#if defined(USE_SURFACE_MESH)
  using Mesh   = CGAL::Surface_mesh<Kernel::Point_3>;
#else
  using Traits = CGAL::Polyhedron_traits_with_normals_3<Kernel>;
  using Mesh = CGAL::Polyhedron_3<Traits, Extended_polyhedron_items>;
  using Face_color_map = boost::property_map<Mesh, CGAL::dynamic_face_property_t<CGAL::IO::Color> >::type;
#endif

  using Vertex_descriptor = boost::graph_traits<Mesh>::vertex_descriptor;
  using Edge_descriptor = boost::graph_traits<Mesh>::edge_descriptor;
  using Face_descriptor = boost::graph_traits<Mesh>::face_descriptor;

  const char* filename = (argc > 1) ? argv[1] : "cube.off";
  std::cout << filename << std::endl;
  Mesh mesh;

#if defined(USE_SURFACE_MESH)
  if (! CGAL::IO::read_polygon_mesh(filename, mesh, CGAL::parameters::verbose(true).repair_polygon_soup(true))) {
    std::cerr << "Error: could not read mesh from '" << filename << "'.\n";
    return 1;
  }
  std::cout << "Loaded mesh with " << num_vertices(mesh) << " vertices, " << num_faces(mesh) << " faces.\n";

  auto face_property_maps = mesh.properties<Face_descriptor>();
  for (const auto& pm : face_property_maps) std::cout << pm << std::endl;

  // Show colors for faces
  auto fcolors_optional = mesh.property_map<Face_descriptor, CGAL::IO::Color >("f:color");
  if (! fcolors_optional) {
     std::cerr << "Error: could not read colors from '" << filename << "'.\n";
     return 1;
  }
  auto fcolors = fcolors_optional.value();

#else
  Face_color_map fcolors;
#if 0
  if (! CGAL::IO::read_polygon_mesh(filename, mesh, CGAL::parameters::verbose(true).repair_polygon_soup(true))) {
#else
  if (! CGAL::IO::read_polygon_mesh(filename, mesh, CGAL::parameters::verbose(true).repair_polygon_soup(true).
                                    face_color_map(fcolors))) {
#endif
    std::cerr << "Error: could not read mesh from '" << filename << "'.\n";
    return 1;
  }
  std::cout << "Loaded mesh with " << num_vertices(mesh) << " vertices, " << num_faces(mesh) << " faces.\n";
#endif

  std::size_t idx = 0;
  for (auto fd : CGAL::faces(mesh)) {
    CGAL::IO::Color c = get(fcolors, fd);
    std::cout << "face[" << idx++ << "]: ("
              << int(c.red())   << ", "
              << int(c.green()) << ", "
              << int(c.blue())  << ", "
              << int(c.alpha()) << ")\n";
  }

  CGAL::Graphics_scene_options<Mesh, Vertex_descriptor, Edge_descriptor, Face_descriptor> gso;
#if ! defined(USE_SURFACE_MESH)
  gso.colored_face=[](const Mesh&, Face_descriptor) -> bool { return true; };
  gso.face_color=[&](const Mesh& mesh, Face_descriptor fd) -> CGAL::IO::Color { return get(fcolors, fd); };
#endif
  CGAL::draw(mesh, gso, filename);

  return 0;
}

Environment

  • Operating system (Windows/Mac/Linux, 32/64 bits):
  • Compiler:
  • Release or debug mode:
  • Specific flags used (if any):
  • CGAL version:
  • Boost version:
  • Other libraries versions if used (Eigen, TBB, etc.):
afabri commented

I am not sure about the importance of off files. Good support for ply seems much more important. Also today I favor good support for Surfcace_mesh as it has the properties mechanism.

efifogel commented
efifogel commented
efifogel commented