Fix T97417: OBJ: support tab and other whitespace characters after obj/mtl keywords

Even if available OBJ/MTL format documentations don't explicitly specify
which characters can possibly separate keywords & arguments, turns out
some files out there in the wild use TAB character after the line
keywords. Which is something the new 3.2 importer was not quite
expecting (T97417).

Fix this by factoring out a utility function that checks if line starts
with a keyword followed by any whitespace, and using that across the
importer. Also fix some other "possible whitespace around name-like
parts" of obj/mtl parser as pointed out by the repro files in T97417.

Reviewed By: Howard Trickey
Differential Revision: https://developer.blender.org/D14782

2 files changed, 147 insertions, 91 deletions

diff --git a/source/blender/io/wavefront_obj/importer/obj_import_file_reader.cc b/source/blender/io/wavefront_obj/importer/obj_import_file_reader.cc
index 3e722f8a0dd..56806449733 100644
--- a/source/blender/io/wavefront_obj/importer/obj_import_file_reader.cc
+++ b/source/blender/io/wavefront_obj/importer/obj_import_file_reader.cc
@@ -310,6 +310,25 @@ OBJParser::~OBJParser()
   }
 }
 
+/* If line starts with keyword followed by whitespace, returns true and drops it from the line. */
+static bool parse_keyword(StringRef &line, StringRef keyword)
+{
+  const size_t keyword_len = keyword.size();
+  if (line.size() < keyword_len + 1) {
+    return false;
+  }
+  if (!line.startswith(keyword)) {
+    return false;
+  }
+  /* Treat any ASCII control character as whitespace; don't use isspace() for performance reasons.
+   */
+  if (line[keyword_len] > ' ') {
+    return false;
+  }
+  line = line.drop_prefix(keyword_len + 1);
+  return true;
+}
+
 void OBJParser::parse(Vector<std::unique_ptr<Geometry>> &r_all_geometries,
                       GlobalVertices &r_global_vertices)
 {
@@ -386,22 +405,18 @@ void OBJParser::parse(Vector<std::unique_ptr<Geometry>> &r_all_geometries,
       }
       /* Most common things that start with 'v': vertices, normals, UVs. */
       if (line[0] == 'v') {
-        if (line.startswith("v ")) {
-          line = line.drop_prefix(2);
+        if (parse_keyword(line, "v")) {
           geom_add_vertex(curr_geom, line, r_global_vertices);
         }
-        else if (line.startswith("vn ")) {
-          line = line.drop_prefix(3);
+        else if (parse_keyword(line, "vn")) {
           geom_add_vertex_normal(curr_geom, line, r_global_vertices);
         }
-        else if (line.startswith("vt ")) {
-          line = line.drop_prefix(3);
+        else if (parse_keyword(line, "vt")) {
           geom_add_uv_vertex(line, r_global_vertices);
         }
       }
       /* Faces. */
-      else if (line.startswith("f ")) {
-        line = line.drop_prefix(2);
+      else if (parse_keyword(line, "f")) {
         geom_add_polygon(curr_geom,
                          line,
                          r_global_vertices,
@@ -411,23 +426,20 @@ void OBJParser::parse(Vector<std::unique_ptr<Geometry>> &r_all_geometries,
                          state_shaded_smooth);
       }
       /* Faces. */
-      else if (line.startswith("l ")) {
-        line = line.drop_prefix(2);
+      else if (parse_keyword(line, "l")) {
         geom_add_edge(curr_geom, line, offsets, r_global_vertices);
       }
       /* Objects. */
-      else if (line.startswith("o ")) {
-        line = line.drop_prefix(2);
+      else if (parse_keyword(line, "o")) {
         state_shaded_smooth = false;
         state_group_name = "";
         state_material_name = "";
         curr_geom = create_geometry(
-            curr_geom, GEOM_MESH, line, r_global_vertices, r_all_geometries, offsets);
+            curr_geom, GEOM_MESH, line.trim(), r_global_vertices, r_all_geometries, offsets);
       }
       /* Groups. */
-      else if (line.startswith("g ")) {
-        line = line.drop_prefix(2);
-        geom_update_group(line, state_group_name);
+      else if (parse_keyword(line, "g")) {
+        geom_update_group(line.trim(), state_group_name);
         int new_index = curr_geom->group_indices_.size();
         state_group_index = curr_geom->group_indices_.lookup_or_add(state_group_name, new_index);
         if (new_index == state_group_index) {
@@ -435,14 +447,12 @@ void OBJParser::parse(Vector<std::unique_ptr<Geometry>> &r_all_geometries,
         }
       }
       /* Smoothing groups. */
-      else if (line.startswith("s ")) {
-        line = line.drop_prefix(2);
+      else if (parse_keyword(line, "s")) {
         geom_update_smooth_group(line, state_shaded_smooth);
       }
       /* Materials and their libraries. */
-      else if (line.startswith("usemtl ")) {
-        line = line.drop_prefix(7);
-        state_material_name = line;
+      else if (parse_keyword(line, "usemtl")) {
+        state_material_name = line.trim();
         int new_mat_index = curr_geom->material_indices_.size();
         state_material_index = curr_geom->material_indices_.lookup_or_add(state_material_name,
                                                                           new_mat_index);
@@ -450,37 +460,35 @@ void OBJParser::parse(Vector<std::unique_ptr<Geometry>> &r_all_geometries,
           curr_geom->material_order_.append(state_material_name);
         }
       }
-      else if (line.startswith("mtllib ")) {
-        line = line.drop_prefix(7);
-        mtl_libraries_.append(string(line));
+      else if (parse_keyword(line, "mtllib")) {
+        mtl_libraries_.append(line.trim());
       }
       /* Comments. */
       else if (line.startswith("#")) {
         /* Nothing to do. */
       }
       /* Curve related things. */
-      else if (line.startswith("cstype ")) {
-        line = line.drop_prefix(7);
+      else if (parse_keyword(line, "cstype")) {
         curr_geom = geom_set_curve_type(
             curr_geom, line, r_global_vertices, state_group_name, offsets, r_all_geometries);
       }
-      else if (line.startswith("deg ")) {
-        line = line.drop_prefix(4);
+      else if (parse_keyword(line, "deg")) {
         geom_set_curve_degree(curr_geom, line);
       }
-      else if (line.startswith("curv ")) {
-        line = line.drop_prefix(5);
+      else if (parse_keyword(line, "curv")) {
         geom_add_curve_vertex_indices(curr_geom, line, r_global_vertices);
       }
-      else if (line.startswith("parm ")) {
-        line = line.drop_prefix(5);
+      else if (parse_keyword(line, "parm")) {
         geom_add_curve_parameters(curr_geom, line);
       }
       else if (line.startswith("end")) {
         /* End of curve definition, nothing else to do. */
       }
+      else if (line.front() <= ' ') {
+        /* Just whitespace, skip. */
+      }
       else {
-        std::cout << "OBJ element not recognised: '" << line << "'" << std::endl;
+        std::cout << "OBJ element not recognized: '" << line << "'" << std::endl;
       }
     }
 
@@ -494,77 +502,64 @@ void OBJParser::parse(Vector<std::unique_ptr<Geometry>> &r_all_geometries,
 
 static eMTLSyntaxElement mtl_line_start_to_enum(StringRef &line)
 {
-  if (line.startswith("map_Kd")) {
-    line = line.drop_prefix(6);
+  if (parse_keyword(line, "map_Kd")) {
     return eMTLSyntaxElement::map_Kd;
   }
-  if (line.startswith("map_Ks")) {
-    line = line.drop_prefix(6);
+  if (parse_keyword(line, "map_Ks")) {
     return eMTLSyntaxElement::map_Ks;
   }
-  if (line.startswith("map_Ns")) {
-    line = line.drop_prefix(6);
+  if (parse_keyword(line, "map_Ns")) {
     return eMTLSyntaxElement::map_Ns;
   }
-  if (line.startswith("map_d")) {
-    line = line.drop_prefix(5);
+  if (parse_keyword(line, "map_d")) {
     return eMTLSyntaxElement::map_d;
   }
-  if (line.startswith("refl")) {
-    line = line.drop_prefix(4);
+  if (parse_keyword(line, "refl")) {
     return eMTLSyntaxElement::map_refl;
   }
-  if (line.startswith("map_refl")) {
-    line = line.drop_prefix(8);
+  if (parse_keyword(line, "map_refl")) {
     return eMTLSyntaxElement::map_refl;
   }
-  if (line.startswith("map_Ke")) {
-    line = line.drop_prefix(6);
+  if (parse_keyword(line, "map_Ke")) {
     return eMTLSyntaxElement::map_Ke;
   }
-  if (line.startswith("bump")) {
-    line = line.drop_prefix(4);
+  if (parse_keyword(line, "bump")) {
     return eMTLSyntaxElement::map_Bump;
   }
-  if (line.startswith("map_Bump") || line.startswith("map_bump")) {
-    line = line.drop_prefix(8);
+  if (parse_keyword(line, "map_Bump") || parse_keyword(line, "map_bump")) {
     return eMTLSyntaxElement::map_Bump;
   }
   return eMTLSyntaxElement::string;
 }
 
-static const std::pair<const char *, int> unsupported_texture_options[] = {
-    {"-blendu ", 1},
-    {"-blendv ", 1},
-    {"-boost ", 1},
-    {"-cc ", 1},
-    {"-clamp ", 1},
-    {"-imfchan ", 1},
-    {"-mm ", 2},
-    {"-t ", 3},
-    {"-texres ", 1},
+static const std::pair<StringRef, int> unsupported_texture_options[] = {
+    {"-blendu", 1},
+    {"-blendv", 1},
+    {"-boost", 1},
+    {"-cc", 1},
+    {"-clamp", 1},
+    {"-imfchan", 1},
+    {"-mm", 2},
+    {"-t", 3},
+    {"-texres", 1},
 };
 
 static bool parse_texture_option(StringRef &line, MTLMaterial *material, tex_map_XX &tex_map)
 {
   line = drop_whitespace(line);
-  if (line.startswith("-o ")) {
-    line = line.drop_prefix(3);
+  if (parse_keyword(line, "-o")) {
     line = parse_floats(line, 0.0f, tex_map.translation, 3);
     return true;
   }
-  if (line.startswith("-s ")) {
-    line = line.drop_prefix(3);
+  if (parse_keyword(line, "-s")) {
     line = parse_floats(line, 1.0f, tex_map.scale, 3);
     return true;
   }
-  if (line.startswith("-bm ")) {
-    line = line.drop_prefix(4);
+  if (parse_keyword(line, "-bm")) {
     line = parse_float(line, 0.0f, material->map_Bump_strength);
     return true;
   }
-  if (line.startswith("-type ")) {
-    line = line.drop_prefix(6);
+  if (parse_keyword(line, "-type")) {
     line = drop_whitespace(line);
     /* Only sphere is supported. */
     tex_map.projection_type = SHD_PROJ_SPHERE;
@@ -577,9 +572,7 @@ static bool parse_texture_option(StringRef &line, MTLMaterial *material, tex_map
   }
   /* Check for unsupported options and skip them. */
   for (const auto &opt : unsupported_texture_options) {
-    if (line.startswith(opt.first)) {
-      /* Drop the option name. */
-      line = line.drop_known_prefix(opt.first);
+    if (parse_keyword(line, opt.first)) {
       /* Drop the arguments. */
       for (int i = 0; i < opt.second; ++i) {
         line = drop_whitespace(line);
@@ -645,12 +638,13 @@ void MTLParser::parse_and_store(Map<string, std::unique_ptr<MTLMaterial>> &r_mat
   StringRef buffer_str{(const char *)buffer, (int64_t)buffer_len};
   while (!buffer_str.is_empty()) {
     StringRef line = read_next_line(buffer_str);
+    line = drop_whitespace(line);
     if (line.is_empty()) {
       continue;
     }
 
-    if (line.startswith("newmtl ")) {
-      line = line.drop_prefix(7);
+    if (parse_keyword(line, "newmtl")) {
+      line = line.trim();
       if (r_materials.remove_as(line)) {
         std::cerr << "Duplicate material found:'" << line
                   << "', using the last encountered Material definition." << std::endl;
@@ -658,36 +652,28 @@ void MTLParser::parse_and_store(Map<string, std::unique_ptr<MTLMaterial>> &r_mat
       material = r_materials.lookup_or_add(string(line), std::make_unique<MTLMaterial>()).get();
     }
     else if (material != nullptr) {
-      if (line.startswith("Ns ")) {
-        line = line.drop_prefix(3);
+      if (parse_keyword(line, "Ns")) {
         parse_float(line, 324.0f, material->Ns);
       }
-      else if (line.startswith("Ka ")) {
-        line = line.drop_prefix(3);
+      else if (parse_keyword(line, "Ka")) {
         parse_floats(line, 0.0f, material->Ka, 3);
       }
-      else if (line.startswith("Kd ")) {
-        line = line.drop_prefix(3);
+      else if (parse_keyword(line, "Kd")) {
         parse_floats(line, 0.8f, material->Kd, 3);
       }
-      else if (line.startswith("Ks ")) {
-        line = line.drop_prefix(3);
+      else if (parse_keyword(line, "Ks")) {
         parse_floats(line, 0.5f, material->Ks, 3);
       }
-      else if (line.startswith("Ke ")) {
-        line = line.drop_prefix(3);
+      else if (parse_keyword(line, "Ke")) {
         parse_floats(line, 0.0f, material->Ke, 3);
       }
-      else if (line.startswith("Ni ")) {
-        line = line.drop_prefix(3);
+      else if (parse_keyword(line, "Ni")) {
         parse_float(line, 1.45f, material->Ni);
       }
-      else if (line.startswith("d ")) {
-        line = line.drop_prefix(2);
+      else if (parse_keyword(line, "d")) {
         parse_float(line, 1.0f, material->d);
       }
-      else if (line.startswith("illum ")) {
-        line = line.drop_prefix(6);
+      else if (parse_keyword(line, "illum")) {
         parse_int(line, 2, material->illum);
       }
       else {
diff --git a/source/blender/io/wavefront_obj/tests/obj_mtl_parser_tests.cc b/source/blender/io/wavefront_obj/tests/obj_mtl_parser_tests.cc
index 176d32a0be4..76eac70cebf 100644
--- a/source/blender/io/wavefront_obj/tests/obj_mtl_parser_tests.cc
+++ b/source/blender/io/wavefront_obj/tests/obj_mtl_parser_tests.cc
@@ -2,6 +2,8 @@
 
 #include <gtest/gtest.h>
 
+#include "BKE_appdir.h"
+
 #include "testing/testing.h"
 
 #include "obj_import_file_reader.hh"
@@ -10,10 +12,31 @@ namespace blender::io::obj {
 
 class obj_mtl_parser_test : public testing::Test {
  public:
+  void check_string(const char *text, const MTLMaterial *expect, size_t expect_count)
+  {
+    BKE_tempdir_init(nullptr);
+    std::string tmp_dir = BKE_tempdir_base();
+    std::string tmp_file_name = "mtl_test.mtl";
+    std::string tmp_file_path = tmp_dir + "/" + tmp_file_name;
+    FILE *tmp_file = BLI_fopen(tmp_file_path.c_str(), "wb");
+    fputs(text, tmp_file);
+    fclose(tmp_file);
+
+    check_impl(tmp_file_name, tmp_dir, expect, expect_count);
+
+    BLI_delete(tmp_file_path.c_str(), false, false);
+  }
   void check(const char *file, const MTLMaterial *expect, size_t expect_count)
   {
     std::string obj_dir = blender::tests::flags_test_asset_dir() + "/io_tests/obj/";
-    MTLParser parser(file, obj_dir + "dummy.obj");
+    check_impl(file, obj_dir, expect, expect_count);
+  }
+  void check_impl(StringRef mtl_file_path,
+                  StringRefNull file_dir,
+                  const MTLMaterial *expect,
+                  size_t expect_count)
+  {
+    MTLParser parser(mtl_file_path, file_dir + "dummy.obj");
     Map<std::string, std::unique_ptr<MTLMaterial>> materials;
     parser.parse_and_store(materials);
 
@@ -48,6 +71,53 @@ class obj_mtl_parser_test : public testing::Test {
   }
 };
 
+TEST_F(obj_mtl_parser_test, string_newlines_whitespace)
+{
+  const char *text =
+      "# a comment\n"
+      "  # indented comment\n"
+      "# comment with CRLF line ending\r\n"
+      "\r\n"
+
+      "newmtl simple\n"
+      "Ka 0.1 0.2 0.3\n"
+      "illum 4\n"
+
+      "newmtl\ttab_indentation\n"
+      "Kd\t \t0.2   0.3\t0.4    \t  \n"
+
+      "newmtl space_after_name \t \n"
+      "Ks 0.4 0.5 0.6\n"
+
+      "newmtl    space_before_name\n"
+
+      "newmtl indented_values\n"
+      "  Ka 0.5 0.6 0.7\n"
+      "\t\t\tKd 0.6 0.7 0.8\n"
+
+      "newmtl crlf_ending\r\n"
+      "Ns 5.0\r\n"
+      "map_Kd    sometex_d.png\r\n"
+      "map_Ks sometex_s_spaces_after_name.png   \t   \r\n";
+  MTLMaterial mat[6];
+  mat[0].name = "simple";
+  mat[0].Ka = {0.1f, 0.2f, 0.3f};
+  mat[0].illum = 4;
+  mat[1].name = "tab_indentation";
+  mat[1].Kd = {0.2f, 0.3f, 0.4f};
+  mat[2].name = "space_after_name";
+  mat[2].Ks = {0.4f, 0.5f, 0.6f};
+  mat[3].name = "space_before_name";
+  mat[4].name = "indented_values";
+  mat[4].Ka = {0.5f, 0.6f, 0.7f};
+  mat[4].Kd = {0.6f, 0.7f, 0.8f};
+  mat[5].name = "crlf_ending";
+  mat[5].Ns = 5.0f;
+  mat[5].tex_map_of_type(eMTLSyntaxElement::map_Kd).image_path = "sometex_d.png";
+  mat[5].tex_map_of_type(eMTLSyntaxElement::map_Ks).image_path = "sometex_s_spaces_after_name.png";
+  check_string(text, mat, ARRAY_SIZE(mat));
+}
+
 TEST_F(obj_mtl_parser_test, cube)
 {
   MTLMaterial mat;