From e68d63755c54af4c907e7782729a5819e2b4a597 Mon Sep 17 00:00:00 2001 From: Calder Phillips-Grafflin Date: Tue, 16 Feb 2021 21:38:33 -0500 Subject: [PATCH 1/6] Update to latest tinyobjloader with patched tryParseDouble --- geometry/proximity/obj_to_surface_mesh.cc | 10 ++++++++-- geometry/proximity_engine.cc | 6 +++++- geometry/render/gl_renderer/shape_meshes.cc | 11 ++++++----- geometry/render/gl_renderer/test/shape_meshes_test.cc | 2 +- tools/workspace/tinyobjloader/repository.bzl | 6 +++--- 5 files changed, 23 insertions(+), 12 deletions(-) diff --git a/geometry/proximity/obj_to_surface_mesh.cc b/geometry/proximity/obj_to_surface_mesh.cc index 9de8df12c5fc..cff83a1bb24c 100644 --- a/geometry/proximity/obj_to_surface_mesh.cc +++ b/geometry/proximity/obj_to_surface_mesh.cc @@ -12,6 +12,7 @@ #include #include "drake/common/drake_assert.h" +#include "drake/common/text_logging.h" #include "drake/geometry/proximity/surface_mesh.h" namespace drake { @@ -128,17 +129,22 @@ SurfaceMesh ReadObjToSurfaceMesh(std::istream* input_stream, tinyobj::attrib_t attrib; // Used for vertices. std::vector shapes; // Used for triangles. std::vector materials; // Not used. + std::string warn; std::string err; // Ignore material-library file. tinyobj::MaterialReader* readMatFn = nullptr; // triangulate non-triangle faces. bool triangulate = true; - bool ret = tinyobj::LoadObj(&attrib, &shapes, &materials, &err, input_stream, - readMatFn, triangulate); + bool ret = tinyobj::LoadObj( + &attrib, &shapes, &materials, &warn, &err, input_stream, readMatFn, + triangulate); if (!ret || !err.empty()) { throw std::runtime_error("Error parsing Wavefront obj file : " + err); } + if (!warn.empty()) { + drake::log()->warn("Warning parsing Wavefront obj file : {}", warn); + } if (shapes.size() == 0) { throw std::runtime_error("The Wavefront obj file has no faces."); } diff --git a/geometry/proximity_engine.cc b/geometry/proximity_engine.cc index 22a51225d726..96f55459e053 100644 --- a/geometry/proximity_engine.cc +++ b/geometry/proximity_engine.cc @@ -139,6 +139,7 @@ namespace { tinyobj::attrib_t attrib; std::vector shapes; std::vector materials; + std::string warn; std::string err; // Tinyobj doesn't infer the search directory from the directory containing @@ -148,12 +149,15 @@ namespace { const std::string obj_folder = filename.substr(0, pos + 1); const char* mtl_basedir = obj_folder.c_str(); - bool ret = tinyobj::LoadObj(&attrib, &shapes, &materials, &err, + bool ret = tinyobj::LoadObj(&attrib, &shapes, &materials, &warn, &err, filename.c_str(), mtl_basedir, triangulate); if (!ret || !err.empty()) { throw std::runtime_error("Error parsing file '" + filename + "' : " + err); } + if (!warn.empty()) { + drake::log()->warn("Warning parsing file '{}' : {}", filename, warn); + } if (shapes.size() != 1) { throw std::runtime_error( diff --git a/geometry/render/gl_renderer/shape_meshes.cc b/geometry/render/gl_renderer/shape_meshes.cc index 7e971ed80889..af2ad09dde98 100644 --- a/geometry/render/gl_renderer/shape_meshes.cc +++ b/geometry/render/gl_renderer/shape_meshes.cc @@ -35,6 +35,7 @@ MeshData LoadMeshFromObj(std::istream* input_stream, tinyobj::attrib_t attrib; vector shapes; vector materials; + string warn; string err; /* This renderer assumes everything is triangles -- we rely on tinyobj to triangulate for us. */ @@ -48,12 +49,12 @@ MeshData LoadMeshFromObj(std::istream* input_stream, Ignore material-library file. */ tinyobj::MaterialReader* material_reader = nullptr; const bool ret = - tinyobj::LoadObj(&attrib, &shapes, &materials, &err, input_stream, + tinyobj::LoadObj(&attrib, &shapes, &materials, &warn, &err, input_stream, material_reader, do_tinyobj_triangulation); - /* As of tinyobj v1.0.6, we expect that `ret` will *always* be true. We are - capturing it and asserting it so that if the version advances, and false is - ever returned, CI will inform us so we can update the error messages. */ - DRAKE_DEMAND(ret == true); + if (!ret) { + throw std::runtime_error( + fmt::format("tinyobj::LoadObj failed to load file: {}", filename)); + } if (shapes.empty()) { throw std::runtime_error(fmt::format( diff --git a/geometry/render/gl_renderer/test/shape_meshes_test.cc b/geometry/render/gl_renderer/test/shape_meshes_test.cc index 21d6660aa047..6b5675958912 100644 --- a/geometry/render/gl_renderer/test/shape_meshes_test.cc +++ b/geometry/render/gl_renderer/test/shape_meshes_test.cc @@ -70,7 +70,7 @@ f 1//0 2//0 3//0 )"""); DRAKE_EXPECT_THROWS_MESSAGE( LoadMeshFromObj(&in_stream), std::runtime_error, - "Not all faces reference texture coordinates.+"); + "tinyobj::LoadObj failed to load file.+"); } } diff --git a/tools/workspace/tinyobjloader/repository.bzl b/tools/workspace/tinyobjloader/repository.bzl index 7f713f05cea1..4ac024652172 100644 --- a/tools/workspace/tinyobjloader/repository.bzl +++ b/tools/workspace/tinyobjloader/repository.bzl @@ -7,9 +7,9 @@ def tinyobjloader_repository( mirrors = None): github_archive( name = name, - repository = "syoyo/tinyobjloader", - commit = "v1.0.6", - sha256 = "19ee82cd201761954dd833de551edb570e33b320d6027e0d91455faf7cd4c341", # noqa + repository = "calderpg-tri/tinyobjloader", + commit = "129a88b40f115951163c284dc9d62de5bbfc3bd1", + sha256 = "96bb917d81ac7fbfeb391bc39a7cc5e6a409a12a39fbf5e7169189dfdd7ef284", # noqa build_file = "@drake//tools/workspace/tinyobjloader:package.BUILD.bazel", # noqa mirrors = mirrors, patches = [ From 773e61b5dcec6e0e7d29c89371a7303673133d06 Mon Sep 17 00:00:00 2001 From: Calder Phillips-Grafflin Date: Wed, 17 Feb 2021 09:28:43 -0500 Subject: [PATCH 2/6] Bump tinyobjloader patches --- tools/workspace/tinyobjloader/repository.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/workspace/tinyobjloader/repository.bzl b/tools/workspace/tinyobjloader/repository.bzl index 4ac024652172..7e723aa533fa 100644 --- a/tools/workspace/tinyobjloader/repository.bzl +++ b/tools/workspace/tinyobjloader/repository.bzl @@ -8,8 +8,8 @@ def tinyobjloader_repository( github_archive( name = name, repository = "calderpg-tri/tinyobjloader", - commit = "129a88b40f115951163c284dc9d62de5bbfc3bd1", - sha256 = "96bb917d81ac7fbfeb391bc39a7cc5e6a409a12a39fbf5e7169189dfdd7ef284", # noqa + commit = "f11dd777c09889530945ced9a39aa3d145a2f40e", + sha256 = "62f2af8ae89a5eeef011a1d381f9495373f1fe95a25ac6933ea3cc0ec8497fb7", # noqa build_file = "@drake//tools/workspace/tinyobjloader:package.BUILD.bazel", # noqa mirrors = mirrors, patches = [ From 8d7974fe66683798cce187cd882aab7921afca6e Mon Sep 17 00:00:00 2001 From: Calder Phillips-Grafflin Date: Wed, 17 Feb 2021 11:27:06 -0500 Subject: [PATCH 3/6] Switch to upstream + patch --- .../tinyobjloader/faster_float_parsing.patch | 158 ++++++++++++++++++ tools/workspace/tinyobjloader/repository.bzl | 9 +- 2 files changed, 164 insertions(+), 3 deletions(-) create mode 100644 tools/workspace/tinyobjloader/faster_float_parsing.patch diff --git a/tools/workspace/tinyobjloader/faster_float_parsing.patch b/tools/workspace/tinyobjloader/faster_float_parsing.patch new file mode 100644 index 000000000000..ab750543c3c9 --- /dev/null +++ b/tools/workspace/tinyobjloader/faster_float_parsing.patch @@ -0,0 +1,158 @@ +--- tiny_obj_loader.h.orig 2017-04-24 15:34:45.000000000 -0400 ++++ tiny_obj_loader.h 2019-06-19 15:57:29.479457051 -0400 +@@ -62,6 +62,14 @@ THE SOFTWARE. + #include + #include + #include ++#include ++#include ++ ++#if defined(__APPLE__) ++#include ++#else ++#include ++#endif + + namespace tinyobj { + +@@ -838,126 +846,23 @@ static bool tryParseDouble(const char *s, const char *s_end, double *result) { + if (s >= s_end) { + return false; + } ++ ++#if defined(__APPLE__) ++ static locale_t c_locale = newlocale(LC_ALL_MASK, NULL, NULL); ++#else ++ static locale_t c_locale = newlocale(LC_ALL_MASK, "C", (locale_t)0); ++#endif + +- double mantissa = 0.0; +- // This exponent is base 2 rather than 10. +- // However the exponent we parse is supposed to be one of ten, +- // thus we must take care to convert the exponent/and or the +- // mantissa to a * 2^E, where a is the mantissa and E is the +- // exponent. +- // To get the final double we will use ldexp, it requires the +- // exponent to be in base 2. +- int exponent = 0; +- +- // NOTE: THESE MUST BE DECLARED HERE SINCE WE ARE NOT ALLOWED +- // TO JUMP OVER DEFINITIONS. +- char sign = '+'; +- char exp_sign = '+'; +- char const *curr = s; +- +- // How many characters were read in a loop. +- int read = 0; +- // Tells whether a loop terminated due to reaching s_end. +- bool end_not_reached = false; +- bool leading_decimal_dots = false; +- +- /* +- BEGIN PARSING. +- */ +- +- // Find out what sign we've got. +- if (*curr == '+' || *curr == '-') { +- sign = *curr; +- curr++; +- if ((curr != s_end) && (*curr == '.')) { +- // accept. Somethig like `.7e+2`, `-.5234` +- leading_decimal_dots = true; +- } +- } else if (IS_DIGIT(*curr)) { /* Pass through. */ +- } else if (*curr == '.') { +- // accept. Somethig like `.7e+2`, `-.5234` +- leading_decimal_dots = true; +- } else { +- goto fail; +- } +- +- // Read the integer part. +- end_not_reached = (curr != s_end); +- if (!leading_decimal_dots) { +- while (end_not_reached && IS_DIGIT(*curr)) { +- mantissa *= 10; +- mantissa += static_cast(*curr - 0x30); +- curr++; +- read++; +- end_not_reached = (curr != s_end); +- } +- +- // We must make sure we actually got something. +- if (read == 0) goto fail; +- } +- +- // We allow numbers of form "#", "###" etc. +- if (!end_not_reached) goto assemble; +- +- // Read the decimal part. +- if (*curr == '.') { +- curr++; +- read = 1; +- end_not_reached = (curr != s_end); +- while (end_not_reached && IS_DIGIT(*curr)) { +- static const double pow_lut[] = { +- 1.0, 0.1, 0.01, 0.001, 0.0001, 0.00001, 0.000001, 0.0000001, +- }; +- const int lut_entries = sizeof pow_lut / sizeof pow_lut[0]; +- +- // NOTE: Don't use powf here, it will absolutely murder precision. +- mantissa += static_cast(*curr - 0x30) * +- (read < lut_entries ? pow_lut[read] : std::pow(10.0, -read)); +- read++; +- curr++; +- end_not_reached = (curr != s_end); +- } +- } else if (*curr == 'e' || *curr == 'E') { ++ errno = 0; ++ char *str_end = NULL; ++ const double val = strtod_l(s, &str_end, c_locale); ++ if (str_end != s && errno != ERANGE && isfinite(val)) { ++ errno = 0; ++ *result = val; ++ return true; + } else { +- goto assemble; +- } +- +- if (!end_not_reached) goto assemble; +- +- // Read the exponent part. +- if (*curr == 'e' || *curr == 'E') { +- curr++; +- // Figure out if a sign is present and if it is. +- end_not_reached = (curr != s_end); +- if (end_not_reached && (*curr == '+' || *curr == '-')) { +- exp_sign = *curr; +- curr++; +- } else if (IS_DIGIT(*curr)) { /* Pass through. */ +- } else { +- // Empty E is not allowed. +- goto fail; +- } +- +- read = 0; +- end_not_reached = (curr != s_end); +- while (end_not_reached && IS_DIGIT(*curr)) { +- exponent *= 10; +- exponent += static_cast(*curr - 0x30); +- curr++; +- read++; +- end_not_reached = (curr != s_end); +- } +- exponent *= (exp_sign == '+' ? 1 : -1); +- if (read == 0) goto fail; ++ return false; + } +- +-assemble: +- *result = (sign == '+' ? 1 : -1) * +- (exponent ? std::ldexp(mantissa * std::pow(5.0, exponent), exponent) +- : mantissa); +- return true; +-fail: +- return false; + } + + static inline real_t parseReal(const char **token, double default_value = 0.0) { diff --git a/tools/workspace/tinyobjloader/repository.bzl b/tools/workspace/tinyobjloader/repository.bzl index 7e723aa533fa..60f38d7b1404 100644 --- a/tools/workspace/tinyobjloader/repository.bzl +++ b/tools/workspace/tinyobjloader/repository.bzl @@ -7,9 +7,9 @@ def tinyobjloader_repository( mirrors = None): github_archive( name = name, - repository = "calderpg-tri/tinyobjloader", - commit = "f11dd777c09889530945ced9a39aa3d145a2f40e", - sha256 = "62f2af8ae89a5eeef011a1d381f9495373f1fe95a25ac6933ea3cc0ec8497fb7", # noqa + repository = "tinyobjloader/tinyobjloader", + commit = "9173980d1de273b17eba5e10eb189e8b4be89425", + sha256 = "fe06bf462bf386ea7f6bf34f94c78099c849df348d4a6df681707fba5b5b643c", # noqa build_file = "@drake//tools/workspace/tinyobjloader:package.BUILD.bazel", # noqa mirrors = mirrors, patches = [ @@ -18,5 +18,8 @@ def tinyobjloader_repository( # dependency of Drake and we don't want the definition to leak into # all target that consume Drake. "@drake//tools/workspace/tinyobjloader:double_precision.patch", + # We replace tinyobjloader's implementation of float parsing with a + # faster call to strtod_l. + "@drake//tools/workspace/tinyobjloader:faster_float_parsing.patch", ], ) From 50cf80fd84bf2a659be94f035736999b3e3fde41 Mon Sep 17 00:00:00 2001 From: Calder Phillips-Grafflin Date: Thu, 18 Feb 2021 10:58:52 -0500 Subject: [PATCH 4/6] Remove patch whitespace and add todo --- geometry/render/gl_renderer/shape_meshes.cc | 2 ++ tools/workspace/tinyobjloader/faster_float_parsing.patch | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/geometry/render/gl_renderer/shape_meshes.cc b/geometry/render/gl_renderer/shape_meshes.cc index af2ad09dde98..f6ac464b5af3 100644 --- a/geometry/render/gl_renderer/shape_meshes.cc +++ b/geometry/render/gl_renderer/shape_meshes.cc @@ -51,6 +51,8 @@ MeshData LoadMeshFromObj(std::istream* input_stream, const bool ret = tinyobj::LoadObj(&attrib, &shapes, &materials, &warn, &err, input_stream, material_reader, do_tinyobj_triangulation); + // TODO(SeanCurtis-TRI) Consider improving this exception message to clarify + // why LoadObj failed to load the file. if (!ret) { throw std::runtime_error( fmt::format("tinyobj::LoadObj failed to load file: {}", filename)); diff --git a/tools/workspace/tinyobjloader/faster_float_parsing.patch b/tools/workspace/tinyobjloader/faster_float_parsing.patch index ab750543c3c9..22fef84e07fb 100644 --- a/tools/workspace/tinyobjloader/faster_float_parsing.patch +++ b/tools/workspace/tinyobjloader/faster_float_parsing.patch @@ -19,7 +19,7 @@ if (s >= s_end) { return false; } -+ ++ +#if defined(__APPLE__) + static locale_t c_locale = newlocale(LC_ALL_MASK, NULL, NULL); +#else From 739c27d36c376d850e14e56f3582e8792428dd12 Mon Sep 17 00:00:00 2001 From: Calder Phillips-Grafflin Date: Thu, 18 Feb 2021 11:33:17 -0500 Subject: [PATCH 5/6] Replace mesh loading error TODO --- geometry/render/gl_renderer/shape_meshes.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/geometry/render/gl_renderer/shape_meshes.cc b/geometry/render/gl_renderer/shape_meshes.cc index f6ac464b5af3..caf0bbdbe7c0 100644 --- a/geometry/render/gl_renderer/shape_meshes.cc +++ b/geometry/render/gl_renderer/shape_meshes.cc @@ -51,8 +51,6 @@ MeshData LoadMeshFromObj(std::istream* input_stream, const bool ret = tinyobj::LoadObj(&attrib, &shapes, &materials, &warn, &err, input_stream, material_reader, do_tinyobj_triangulation); - // TODO(SeanCurtis-TRI) Consider improving this exception message to clarify - // why LoadObj failed to load the file. if (!ret) { throw std::runtime_error( fmt::format("tinyobj::LoadObj failed to load file: {}", filename)); @@ -137,6 +135,11 @@ MeshData LoadMeshFromObj(std::istream* input_stream, const int position_index = shape_mesh.indices[v_index].vertex_index; const int norm_index = shape_mesh.indices[v_index].normal_index; const int uv_index = shape_mesh.indices[v_index].texcoord_index; + // TODO(SeanCurtis-TRI) PR 14656 changed parse semantics. This error + // condition appears to no longer be reachable (it no longer appears + // in the unit tests) and the condition that this detects won't trigger + // this helpful message. Either clean up this case or find a way to give + // this feedback under the new tinyobj. if (norm_index < 0) { throw std::runtime_error( fmt::format("Not all faces reference normals: {}", filename)); From 3359891ad16b59588c940ec74f99803d190cde1e Mon Sep 17 00:00:00 2001 From: Calder Phillips-Grafflin Date: Thu, 18 Feb 2021 12:28:18 -0500 Subject: [PATCH 6/6] Remove errno handling from patch --- .../tinyobjloader/faster_float_parsing.patch | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tools/workspace/tinyobjloader/faster_float_parsing.patch b/tools/workspace/tinyobjloader/faster_float_parsing.patch index 22fef84e07fb..df50e76c8d10 100644 --- a/tools/workspace/tinyobjloader/faster_float_parsing.patch +++ b/tools/workspace/tinyobjloader/faster_float_parsing.patch @@ -15,16 +15,9 @@ namespace tinyobj { -@@ -838,126 +846,23 @@ static bool tryParseDouble(const char *s, const char *s_end, double *result) { - if (s >= s_end) { +@@ -839,125 +847,20 @@ static bool tryParseDouble(const char *s, const char *s_end, double *result) { return false; } -+ -+#if defined(__APPLE__) -+ static locale_t c_locale = newlocale(LC_ALL_MASK, NULL, NULL); -+#else -+ static locale_t c_locale = newlocale(LC_ALL_MASK, "C", (locale_t)0); -+#endif - double mantissa = 0.0; - // This exponent is base 2 rather than 10. @@ -82,7 +75,12 @@ - // We must make sure we actually got something. - if (read == 0) goto fail; - } -- ++#if defined(__APPLE__) ++ static locale_t c_locale = newlocale(LC_ALL_MASK, NULL, NULL); ++#else ++ static locale_t c_locale = newlocale(LC_ALL_MASK, "C", (locale_t)0); ++#endif + - // We allow numbers of form "#", "###" etc. - if (!end_not_reached) goto assemble; - @@ -105,11 +103,9 @@ - end_not_reached = (curr != s_end); - } - } else if (*curr == 'e' || *curr == 'E') { -+ errno = 0; + char *str_end = NULL; + const double val = strtod_l(s, &str_end, c_locale); -+ if (str_end != s && errno != ERANGE && isfinite(val)) { -+ errno = 0; ++ if (str_end != s && isfinite(val)) { + *result = val; + return true; } else {