Raise error if mesh has flipped faces.

PiperOrigin-RevId: 461860424
Change-Id: Ib23c50cf7f64de1173404d2f65ad87177fab45b3

src/user/user_mesh.cc

@@ -62,6 +62,7 @@ mjCMesh::mjCMesh(mjCModel* _model, mjCDef* _def) {
   usernormal.clear();
   usertexcoord.clear();
   userface.clear();
+  useredge.clear();
 
   // clear internal variables
   mjuu_setvec(pos, 0, 0, 0);
@@ -97,6 +98,7 @@ mjCMesh::~mjCMesh() {
   usernormal.clear();
   usertexcoord.clear();
   userface.clear();
+  useredge.clear();
 
   if (vert) mju_free(vert);
   if (normal) mju_free(normal);
@@ -200,6 +202,26 @@ void mjCMesh::Compile(const mjVFS* vfs) {
     nface = (int)userface.size()/3;
     face = (int*) mju_malloc(3*nface*sizeof(int));
     memcpy(face, userface.data(), 3*nface*sizeof(int));
+
+    // create half-edge structure (if mesh was in XML)
+    if (useredge.empty()) {
+      for (int i=0; i<nface; i++) {
+        int v0 = userface[3*i+0];
+        int v1 = userface[3*i+1];
+        int v2 = userface[3*i+2];
+        useredge.push_back(std::pair(v0, v1));
+        useredge.push_back(std::pair(v1, v2));
+        useredge.push_back(std::pair(v2, v0));
+      }
+    }
+  }
+
+  // check for inconsistent face orientations
+  if (!useredge.empty()) {
+    std::sort(useredge.begin(), useredge.end());
+    auto iterator = std::adjacent_find(useredge.begin(), useredge.end());
+    if (iterator != useredge.end())
+      throw mjCError(this, "faces have inconsistent orientation");
   }
 
   // require vertices
@@ -525,18 +547,25 @@ void mjCMesh::LoadOBJ(const mjVFS* vfs) {
             this, "only tri or quad meshes are supported for OBJ (file '%s')",
             filename.c_str());
       }
+      // add face
       std::vector<std::array<tinyobj::index_t, 3>> faces;
-      std::array<tinyobj::index_t, 3> face1 = {
+      tinyobj::index_t v0 = mesh.indices[index_in_mesh_indices];
-          mesh.indices[index_in_mesh_indices],
+      tinyobj::index_t v1 = mesh.indices[index_in_mesh_indices+1];
-          mesh.indices[index_in_mesh_indices+1],
+      tinyobj::index_t v2 = mesh.indices[index_in_mesh_indices+2];
-          mesh.indices[index_in_mesh_indices+2]};
+      std::array<tinyobj::index_t, 3> face1 = {v0, v1, v2};
       faces.push_back(face1);
-      if (mesh.num_face_vertices[face] == 4) {  // add second triangle with 4th vertex
+      // add edges
-        std::array<tinyobj::index_t, 3> face2 = {
+      useredge.push_back(std::pair(v0.vertex_index, v1.vertex_index));
-            mesh.indices[index_in_mesh_indices],
+      useredge.push_back(std::pair(v1.vertex_index, v2.vertex_index));
-            mesh.indices[index_in_mesh_indices+2],
+      useredge.push_back(std::pair(v2.vertex_index, v0.vertex_index));
-            mesh.indices[index_in_mesh_indices+3]};
+      // handle quad: add second triangle with 4th vertex
+      if (mesh.num_face_vertices[face] == 4) {
+        tinyobj::index_t v3 = mesh.indices[index_in_mesh_indices+3];
+        std::array<tinyobj::index_t, 3> face2 = {v0, v2, v3};
         faces.push_back(face2);
+        useredge.push_back(std::pair(v0.vertex_index, v2.vertex_index));
+        useredge.push_back(std::pair(v2.vertex_index, v3.vertex_index));
+        useredge.push_back(std::pair(v3.vertex_index, v0.vertex_index));
       }
       for (const auto& face_indices : faces) {
         int index_of_first_vertex = uservert.size()/3;

src/user/user_objects.h

@@ -453,10 +453,11 @@ class mjCMesh: public mjCBase {
   double scale[3];                    // rescale mesh
   bool smoothnormal;                  // do not exclude large-angle faces from normals
 
-  std::vector<float> uservert;        // user vertex data
+  std::vector<float> uservert;                   // user vertex data
-  std::vector<float> usernormal;      // user normal data
+  std::vector<float> usernormal;                 // user normal data
-  std::vector<float> usertexcoord;    // user texcoord data
+  std::vector<float> usertexcoord;               // user texcoord data
-  std::vector<int> userface;          // user face data
+  std::vector<int> userface;                     // user face data
+  std::vector< std::pair<int, int> > useredge;   // user half-edge data
 
  private:
   mjCMesh(mjCModel* = 0, mjCDef* = 0);        // constructor

test/user/testdata/malformed_face.obj

@@ -0,0 +1,9 @@
+v 0 0 0
+v 1 0 0
+v 0 1 0
+v 0 0 1
+f 3 1 4
+f 1 2 4
+f 2 3 4
+# malformed face
+f 1 2 3

test/user/testdata/malformed_face.xml

@@ -0,0 +1,8 @@
+<mujoco>
+  <asset>
+    <mesh file="malformed_face.obj"/>
+  </asset>
+  <worldbody>
+    <geom type="mesh" mesh="malformed_face"/>
+  </worldbody>
+</mujoco>

test/user/user_mesh_test.cc

@@ -43,6 +43,8 @@ static const char* const kTexturedTorusPath =
     "user/testdata/textured_torus.xml";
 static const char* const kDuplicateOBJPath =
     "user/testdata/duplicate.xml";
+static const char* const kMalformedFaceOBJPath =
+    "user/testdata/malformed_face.xml";
 
 using ::testing::HasSubstr;
 
@@ -174,5 +176,31 @@ TEST_F(MujocoTest, TinyInertiaFails) {
           "mass and inertia of moving bodies must be larger than mjMINVAL"));
 }
 
+TEST_F(MujocoTest, MalformedFaceFails) {
+  const std::string xml_path = GetTestDataFilePath(kMalformedFaceOBJPath);
+  std::array<char, 1024> error;
+  mjModel* model = mj_loadXML(xml_path.c_str(), 0, error.data(), error.size());
+  ASSERT_THAT(model, testing::IsNull());
+  EXPECT_THAT(error.data(), HasSubstr("faces have inconsistent orientation"));
+}
+
+TEST_F(MujocoTest, FlippedFaceFails) {
+  static constexpr char xml[] = R"(
+  <mujoco>
+    <asset>
+      <mesh name="example_mesh"
+        vertex="0 0 0  1 0 0  0 1 0  0 0 1"
+        face="2 0 3  0 1 3  1 2 3  0 1 2" />
+    </asset>
+    <worldbody>
+      <geom type="mesh" mesh="example_mesh"/>
+    </worldbody>
+  </mujoco>
+  )";
+  std::array<char, 1024> error;
+  LoadModelFromString(xml, error.data(), error.size());
+  EXPECT_THAT(error.data(), HasSubstr("faces have inconsistent orientation"));
+}
+
 }  // namespace
 }  // namespace mujoco

test/xml/xml_native_writer_test.cc

@@ -34,6 +34,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include <absl/container/flat_hash_set.h>
+#include <absl/strings/match.h>
 #include <mujoco/mjmodel.h>
 #include <mujoco/mjtnum.h>
 #include <mujoco/mjxmacro.h>
@@ -416,10 +417,15 @@ TEST_F(XMLWriterTest, WriteReadCompare) {
       if (p.path().extension() == ext) {
         std::string xml = p.path().string();
 
+        // if file is meant to fail, skip it
+        if (absl::StrContains(p.path().string(), "malformed_")) {
+          continue;
+        }
+
         // load model
         std::array<char, 1000> error;
         mjModel* m = mj_loadXML(xml.c_str(), nullptr, error.data(), error.size());
-        ASSERT_THAT(m, NotNull()) << "Failed to load model: " << error.data();
+        ASSERT_THAT(m, NotNull()) << "Failed to load " << xml.c_str() << ": " << error.data();
 
         // make data
         mjData* d = mj_makeData(m);