OFF binary reader - regression

[janetournois]

Issue Details

The function read_off() from Surface_mesh.h is not able anymore to read a binary OFF file.

Source Code

From what I found, commit 26c86d28be1908a972b1fbb85777b735767b227b (introduced in CGAL-4.14.3) is responsible for that issue. The following patch is enough to fix the issue (reading of binary off files is fixed), but I believe it breaks something that the initial commit was itself fixing (?)

diff --git a/Surface_mesh/include/CGAL/Surface_mesh/Surface_mesh.h b/Surface_mesh/include/CGAL/Surface_mesh/Surface_mesh.h
index a8df78b8ae..5743923f6e 100644
--- a/Surface_mesh/include/CGAL/Surface_mesh/Surface_mesh.h
+++ b/Surface_mesh/include/CGAL/Surface_mesh/Surface_mesh.h
@@ -2506,6 +2506,7 @@ private: //------------------------------------------------------- private data
     }
     sm.reserve(sm.num_vertices()+n, sm.num_edges()+e, sm.num_faces()+f);
     std::vector<Vertex_index> vertexmap(n);
+    P p;

     Vector_3 v;
     typename Mesh::template Property_map<Vertex_index,CGAL::Color> vcolor;
@@ -2521,18 +2522,15 @@ private: //------------------------------------------------------- private data
     char ci;
     for(int i=0; i < n; i++){
       is >> sm_skip_comments;
-      std::getline(is, line);
-      std::istringstream iss(line);
-      double x, y, z;
-      iss >> iformat(x) >> iformat(y) >> iformat(z);
+      is >> p;

       Vertex_index vi = sm.add_vertex();
-      put(vpm, vi, P(x, y, z));
+      put(vpm, vi, p);

       vertexmap[i] = vi;
       if(v_has_normals){
-        if(!(iss >> v))
+        if(!(is >> v))
         {
           std::cerr<<"This doesn't seem to be a correct NOFF file. Aborting."<<std::endl;
           is.setstate(std::ios::failbit);
@@ -2544,7 +2542,7 @@ private: //------------------------------------------------------- private data

       if(i == 0 && ((off == "COFF") || (off == "CNOFF"))){
         std::string col;
-        std::getline(iss, col);
+        std::getline(is, col);
         std::istringstream iss2(col);
         if(iss2 >> ci){
          bool created;
@@ -2562,7 +2560,7 @@ private: //------------------------------------------------------- private data
       }else{
          if(vcolored){
            //stores the RGB value
-           vcolor[vi] = File_scanner_OFF::get_color_from_line(iss);
+           vcolor[vi] = File_scanner_OFF::get_color_from_line(is);
          }
        }
     }

Environment

• Operating system (Windows/Mac/Linux, 32/64 bits): Windows
• Compiler: MSVC 2019
• Release or debug mode: Both
• CGAL version: 5.2.1

[janetournois]

note : the reader has moved to another file in master but still can't read binary OFF

[lrineau]

I do not know if we can call that a bug. The exist an official "binary OFF", and that is a different file format.

Here you are talking of the ASCII OFF format, produced by older versions of CGAL, with an std::ostream where CGAL::set_binary_mode was called. That produced an invalid OFF file, where all the file was in ASCII, but the coordinates of the 3D points, that were in the file in binary.

[lrineau]

Maybe that patch can be sufficient:

diff --git a/Stream_support/include/CGAL/IO/OFF/File_scanner_OFF.h b/Stream_support/include/CGAL/IO/OFF/File_scanner_OFF.h
index 90c1ed4305f..e956d9b9062 100644
--- a/Stream_support/include/CGAL/IO/OFF/File_scanner_OFF.h
+++ b/Stream_support/include/CGAL/IO/OFF/File_scanner_OFF.h
@@ -88,6 +88,7 @@ public:

       // Read all numbers in the line
       std::istringstream issline(line);
+      set_mode(issline, get_mode(m_in));
       entries.clear();
       double d;
       while(issline >> IO::iformat(d)){
@@ -539,6 +540,7 @@ public:
     std::getline(is, col);
     //split it into strings
     std::istringstream iss(col);
+    set_mode(iss, get_mode(m_in));
     //holds the rgb values
     unsigned char rgb[3]{};
     int index =0;
@@ -702,6 +704,7 @@ public:

       // Read all numbers in the line
       std::istringstream issline(line);
+      set_mode(issline, get_mode(m_in));
       entries.clear();
       double d;
       while(issline >> IO::iformat(d)){

It copies the CGAL I/O mode of the input stream to the temporary std::istringstream. And then CGAL::iformat(double) should do whatever has to be done.

[sloriot]

AFAIR, this issue could be closed as it is not a bug, right? @janetournois @MaelRL

[janetournois]

Well we still can't read binary off files, so I would not say it's solved, but it seems we don't want to re-introduce it. I was just concerned by the fact that some users could have created some files with CGAL in the past, that they cannot open anymore with recent versions.

[sloriot]

Shall a note in CHANGES.md be the minimum we should do?

[lrineau]

Has anyone tried the patch I suggested at https://github.com/CGAL/cgal/issues/5788#issuecomment-872223637? If the solution was as simple as that, then that would solve our issue.

[janetournois]

I'll give it a try

[janetournois]

(sorry, I clicked too fast)

[janetournois]

@lrineau the patch is not enough. Actually, the if(binary()) at the beginning of scan_vertex() returns false, and get_mode(m_in) returns ASCII...