fix: STEP part names with accented characters import as numbers (clears 6 warnings) (#15406)

src/libslic3r/Format/OBJ.cpp

@@ -55,9 +55,9 @@ bool load_obj(const char *path, TriangleMesh *meshptr, ObjInfo& obj_info, std::s
                 boost::filesystem::path temp_mtl_path(mtl_file);
                 mtl_path = temp_mtl_path;
             }
-            auto    _mtl_path = mtl_name_is_path ? mtl_abs_path.string().c_str() : mtl_path.string().c_str();
+            const std::string _mtl_path = (mtl_name_is_path ? mtl_abs_path : mtl_path).string();
             if (boost::filesystem::exists(mtl_name_is_path ? mtl_abs_path : mtl_path)) {
-                if (!ObjParser::mtlparse(_mtl_path, mtl_data)) {
+                if (!ObjParser::mtlparse(_mtl_path.c_str(), mtl_data)) {
                     BOOST_LOG_TRIVIAL(error) << "load_obj:load_mtl: failed to parse " << _mtl_path;
                     message = _L("load mtl in obj: failed to parse");
                     return false;

src/libslic3r/Format/STEP.cpp

@@ -111,14 +111,19 @@ bool StepPreProcessor::isUtf8File(const char* path)
 bool StepPreProcessor::isUtf8(const std::string str)
 {
     size_t num = 0;
-    int i = 0;
+    size_t i = 0;
     while (i < str.length()) {
-        if ((str[i] & 0x80) == 0x00) {
+        const unsigned char lead = static_cast<unsigned char>(str[i]);
+        if ((lead & 0x80) == 0x00) {
             i++;
-        } else if ((num = preNum(str[i])) > 2) {
+        // preNum() counts the leading 1 bits, and a multi-byte sequence is 2 to 4
+        // bytes long, so anything outside that range is not a lead byte.
+        } else if ((num = preNum(lead)) >= 2 && num <= 4) {
+            if (i + num > str.length())
+                return false;
             i++;
-            for (int j = 0; j < num - 1; j++) {
-                if ((str[i] & 0xc0) != 0x80)
+            for (size_t j = 0; j < num - 1; j++) {
+                if ((static_cast<unsigned char>(str[i]) & 0xc0) != 0x80)
                     return false;
                 i++;
             }
@@ -132,15 +137,20 @@ bool StepPreProcessor::isUtf8(const std::string str)
 bool StepPreProcessor::isGBK(const std::string str) {
     size_t i = 0;
     while (i < str.length()) {
-        if (str[i] <= 0x7f) {
+        // char is signed here, so every byte compares <= 0x7f unless widened first.
+        const unsigned char lead = static_cast<unsigned char>(str[i]);
+        if (lead <= 0x7f) {
             i++;
             continue;
         } else {
-            if (str[i] >= 0x81 &&
-                str[i] <= 0xfe &&
-                str[i + 1] >= 0x40 &&
-                str[i + 1] <= 0xfe &&
-                str[i + 1] != 0xf7) {
+            if (i + 1 >= str.length())
+                return false;
+            const unsigned char trail = static_cast<unsigned char>(str[i + 1]);
+            if (lead >= 0x81 &&
+                lead <= 0xfe &&
+                trail >= 0x40 &&
+                trail <= 0xfe &&
+                trail != 0xf7) {
                 i += 2;
                 continue;
             }

tests/libslic3r/CMakeLists.txt

@@ -29,6 +29,7 @@ add_executable(${_TEST_NAME}_tests
     test_mutable_polygon.cpp
     test_mutable_priority_queue.cpp
     test_nozzle_volume_type.cpp
+    test_step.cpp
     test_stl.cpp
     test_triangle_selector.cpp
     test_meshboolean.cpp

tests/libslic3r/test_step.cpp

@@ -0,0 +1,93 @@
+#include <catch2/catch_all.hpp>
+
+#include <boost/nowide/fstream.hpp>
+
+#include "libslic3r/Model.hpp"
+#include "libslic3r/Format/STEP.hpp"
+#include "test_utils.hpp"
+
+using namespace Slic3r;
+
+static void write_step_line(const std::string &path, const std::string &line)
+{
+    boost::nowide::ofstream file(path, std::ios::binary);
+    file << "ISO-10303-21;\n" << line << "\nEND-ISO-10303-21;\n";
+}
+
+// preprocess() hands back the input path unless it transcoded into a temporary.
+static std::string preprocess_result(const std::string &line)
+{
+    ScopedSlic3rTemporaryDir scratch;
+
+    ScopedTemporaryFile step(".step");
+    write_step_line(step.string(), line);
+
+    std::string      output_path;
+    StepPreProcessor preprocessor;
+    REQUIRE(preprocessor.preprocess(step.string().c_str(), output_path));
+
+    return output_path == step.string() ? "untouched" : "transcoded";
+}
+
+// data/utf8_part_names.step is three boxes written by OCCT's own STEP writer, whose
+// PRODUCT names were then patched to raw UTF-8. Most CAD exporters write non-ASCII names
+// that way rather than in the \X2\ escape form. The third part is ASCII, as a control.
+TEST_CASE("Part names with multi-byte UTF-8 survive import", "[Step]")
+{
+    // getNamedSolids() replaces a name that isUtf8() rejects with a running number.
+    const std::string path = TEST_DATA_DIR PATH_SEPARATOR "utf8_part_names.step";
+
+    Model model;
+    bool  cancel = false;
+    Step  step(path); // no isUtf8Fn, matching how Model::read_from_step builds it
+
+    REQUIRE(step.load() == Step::Step_Status::LOAD_SUCCESS);
+    REQUIRE(step.mesh(&model, cancel, false) == Step::Step_Status::MESH_SUCCESS);
+
+    REQUIRE(model.objects.size() == 1);
+    const ModelObject *object = model.objects.front();
+    REQUIRE(object->volumes.size() == 3);
+    // "ce" is split off, or the hex escape would swallow it as further hex digits.
+    CHECK(object->volumes[0]->name == "pi\xC3\xA8" "ce");
+    CHECK(object->volumes[1]->name == "Geh\xC3\xA4use");
+    CHECK(object->volumes[2]->name == "bracket");
+}
+
+TEST_CASE("isUtf8 recognises two, three and four byte sequences", "[Step]")
+{
+    CHECK(StepPreProcessor::isUtf8("\xC3\xA9"));         // U+00E9
+    CHECK(StepPreProcessor::isUtf8("\xE4\xB8\xAD"));     // U+4E2D
+    CHECK(StepPreProcessor::isUtf8("\xF0\x9F\x94\xA9")); // U+1F529
+    CHECK_FALSE(StepPreProcessor::isUtf8("\x81\x30"));   // 0x81 is not a lead byte
+    CHECK_FALSE(StepPreProcessor::isUtf8("\xC3"));       // truncated sequence
+}
+
+// The only caller of isGBK is preprocess(), which nothing calls today.
+TEST_CASE("Encoding detection decides whether a step file is transcoded", "[Step]")
+{
+    SECTION("UTF-8, so left alone")
+    {
+        // A two byte sequence also satisfies every GBK range, so misdetecting it as
+        // not-UTF-8 sends it to be transcoded.
+        const std::string sequence = GENERATE(std::string("\xC3\xA9"),          // U+00E9
+                                              std::string("\xE4\xB8\xAD"),      // U+4E2D
+                                              std::string("\xF0\x9F\x94\xA9")); // U+1F529
+
+        CHECK(preprocess_result("NAME('" + sequence + "');") == "untouched");
+    }
+
+    SECTION("neither UTF-8 nor GBK, so left alone")
+    {
+        // 0x81 is not a UTF-8 lead byte, and 0x30 is below the 0x40 floor for a GBK trail.
+        CHECK(preprocess_result("NAME('\x81\x30');") == "untouched");
+    }
+
+    SECTION("GBK, so transcoded")
+    {
+        // U+554A in GBK, whose lead byte is not valid UTF-8. Pins the other direction,
+        // since a detector that never reports GBK would pass every case above.
+        CHECK(preprocess_result("NAME('\xB0\xA1');") == "transcoded");
+    }
+
+    SECTION("plain ASCII, so left alone") { CHECK(preprocess_result("NAME('bracket');") == "untouched"); }
+}

tests/test_utils.hpp

@@ -4,6 +4,7 @@
 #include <libslic3r/TriangleMesh.hpp>
 #include <libslic3r/Format/OBJ.hpp>
 #include <libslic3r/SVG.hpp>
+#include <libslic3r/Utils.hpp>
 
 #include <boost/filesystem.hpp>
 
@@ -32,7 +33,7 @@ inline Slic3r::TriangleMesh load_model(const std::string &obj_filename)
 // ---------------------------------------------------------------------------
 
 // Owns a unique path under the system temp dir, "<prefix>-<unique>[<extension>]"
-// (parallel-safe, cross-platform). Shared base for the two RAII temp guards below.
+// (parallel-safe, cross-platform). Shared base for the RAII temp guards below.
 class ScopedTemporaryPath
 {
 public:
@@ -70,6 +71,24 @@ public:
     ~ScopedTemporaryDir() { boost::system::error_code ec; boost::filesystem::remove_all(m_path, ec); }
 };
 
+// A temp directory that is also Slic3r::temporary_dir() for its lifetime. No test
+// process sets that global, so code under test which writes there (for example
+// StepPreProcessor::preprocess) lands at the filesystem root. Restored on scope exit
+// even when an assertion throws, so it cannot leak into later tests.
+class ScopedSlic3rTemporaryDir : public ScopedTemporaryDir
+{
+public:
+    explicit ScopedSlic3rTemporaryDir(const std::string &prefix = "orca")
+        : ScopedTemporaryDir(prefix), m_previous(Slic3r::temporary_dir())
+    { Slic3r::set_temporary_dir(string()); }
+    // Runs before ~ScopedTemporaryDir, so the setting goes back while the directory
+    // it names still exists.
+    ~ScopedSlic3rTemporaryDir() { Slic3r::set_temporary_dir(m_previous); }
+
+private:
+    const std::string m_previous;
+};
+
 // ---------------------------------------------------------------------------
 // Debug-only test artifacts
 //