From 0ec836b2812dfffc01f2b13191ed250ceb4f6c14 Mon Sep 17 00:00:00 2001 From: Sam Haves Date: Thu, 26 Feb 2026 07:03:57 -0800 Subject: [PATCH] Rollback of Move OBJ handling to a mjpDecoder. PiperOrigin-RevId: 875705083 Change-Id: I21403d84abd56ef72de915bddc724fe3efa506e6 --- .github/workflows/build_steps.sh | 2 - CMakeLists.txt | 1 - cmake/ShellTests.cmake | 1 - plugin/obj_decoder/CMakeLists.txt | 46 ---------- plugin/obj_decoder/obj_decoder.cc | 125 -------------------------- python/mujoco/msh2obj_test.py | 1 - src/user/CMakeLists.txt | 1 + src/user/user_mesh.cc | 140 ++++++++++++++++++++---------- src/user/user_objects.h | 11 ++- test/user/CMakeLists.txt | 23 +---- test/user/user_mesh_test.cc | 9 +- test/xml/CMakeLists.txt | 7 +- 12 files changed, 109 insertions(+), 258 deletions(-) delete mode 100644 plugin/obj_decoder/CMakeLists.txt delete mode 100644 plugin/obj_decoder/obj_decoder.cc diff --git a/.github/workflows/build_steps.sh b/.github/workflows/build_steps.sh index 215b3738..6a6889bc 100755 --- a/.github/workflows/build_steps.sh +++ b/.github/workflows/build_steps.sh @@ -101,7 +101,6 @@ copy_plugins_posix() { mkdir -p ${TMPDIR}/mujoco_install/mujoco_plugin && cp lib/libactuator.* ${TMPDIR}/mujoco_install/mujoco_plugin && cp lib/libelasticity.* ${TMPDIR}/mujoco_install/mujoco_plugin && - cp lib/libobj_decoder.* ${TMPDIR}/mujoco_install/mujoco_plugin && cp lib/libsensor.* ${TMPDIR}/mujoco_install/mujoco_plugin && cp lib/libsdf_plugin.* ${TMPDIR}/mujoco_install/mujoco_plugin } @@ -112,7 +111,6 @@ copy_plugins_window() { mkdir -p ${TMPDIR}/mujoco_install/mujoco_plugin && cp bin/Release/actuator.dll ${TMPDIR}/mujoco_install/mujoco_plugin && cp bin/Release/elasticity.dll ${TMPDIR}/mujoco_install/mujoco_plugin && - cp bin/Release/obj_decoder.dll ${TMPDIR}/mujoco_install/mujoco_plugin && cp bin/Release/sensor.dll ${TMPDIR}/mujoco_install/mujoco_plugin } diff --git a/CMakeLists.txt b/CMakeLists.txt index bb6d8748..14540696 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -113,7 +113,6 @@ add_subdirectory(src/engine) add_subdirectory(src/user) add_subdirectory(src/xml) add_subdirectory(src/thread) -add_subdirectory(plugin/obj_decoder) if(MUJOCO_USE_FILAMENT AND NOT EMSCRIPTEN) # Note that, by default, the "src/render" and "src/ui" code is added directly diff --git a/cmake/ShellTests.cmake b/cmake/ShellTests.cmake index d2480a25..b26b1658 100644 --- a/cmake/ShellTests.cmake +++ b/cmake/ShellTests.cmake @@ -38,7 +38,6 @@ function(add_mujoco_shell_test TEST_NAME TARGET_BINARY) "CMAKE_SOURCE_DIR=${CMAKE_SOURCE_DIR}" "TARGET_BINARY=$" "TEST_TMPDIR=${TEST_TMPDIR}" - "MUJOCO_PLUGIN_DIR=$" ) if(WIN32) # Define the directory containing the mujoco DLL library so that it can be added to the PATH. diff --git a/plugin/obj_decoder/CMakeLists.txt b/plugin/obj_decoder/CMakeLists.txt deleted file mode 100644 index 4b50d575..00000000 --- a/plugin/obj_decoder/CMakeLists.txt +++ /dev/null @@ -1,46 +0,0 @@ -# Copyright 2026 DeepMind Technologies Limited -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# https://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE) -set(CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_LIBDIR}") - -add_library(obj_decoder SHARED obj_decoder.cc) - -target_include_directories(obj_decoder PRIVATE - ${CMAKE_CURRENT_SOURCE_DIR}/../.. -) - -target_link_libraries(obj_decoder PRIVATE - mujoco - tinyobjloader -) - -target_compile_definitions(obj_decoder PRIVATE TINYOBJLOADER_IMPLEMENTATION) - -target_compile_options(obj_decoder PRIVATE - ${AVX_COMPILE_OPTIONS} - ${MUJOCO_MACOS_COMPILE_OPTIONS} - ${EXTRA_COMPILE_OPTIONS} - ${MUJOCO_CXX_FLAGS} -) - -target_link_options(obj_decoder PRIVATE - ${MUJOCO_MACOS_LINK_OPTIONS} - ${EXTRA_LINK_OPTIONS} -) - -install( - TARGETS obj_decoder - LIBRARY DESTINATION "${CMAKE_INSTALL_BINDIR}/mujoco_plugin" -) diff --git a/plugin/obj_decoder/obj_decoder.cc b/plugin/obj_decoder/obj_decoder.cc deleted file mode 100644 index 18cfdf5f..00000000 --- a/plugin/obj_decoder/obj_decoder.cc +++ /dev/null @@ -1,125 +0,0 @@ -// Copyright 2026 DeepMind Technologies Limited -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include -#include -#include -#include - -#include -#include -#include -#include - -namespace { - -mjSpec* Decode(mjResource* resource, const mjVFS* vfs) { - const void* bytes = nullptr; - int buffer_sz = mju_readResource(resource, &bytes); - if (buffer_sz < 0) { - mju_warning("obj_decoder: could not read OBJ file '%s'", resource->name); - return nullptr; - } - - tinyobj::ObjReader obj_reader; - const char* buffer = static_cast(bytes); - obj_reader.ParseFromString(std::string(buffer, buffer_sz), std::string()); - - if (!obj_reader.Valid()) { - mju_warning("obj_decoder: could not parse OBJ file '%s'", resource->name); - return nullptr; - } - - mjSpec* spec = mj_makeSpec(); - mjsMesh* mesh = mjs_addMesh(spec, nullptr); - - const auto& attrib = obj_reader.GetAttrib(); - - std::vector usernormal = attrib.normals; - std::vector usertexcoord = attrib.texcoords; - std::vector userface; - std::vector userfacenormal; - std::vector userfacetexcoord; - - if (!obj_reader.GetShapes().empty()) { - const auto& obj_mesh = obj_reader.GetShapes()[0].mesh; - - std::vector face_indices; - for (size_t face = 0, idx = 0; idx < obj_mesh.indices.size();) { - int nfacevert = obj_mesh.num_face_vertices[face]; - if (nfacevert < 3 || nfacevert > 4) { - mju_warning( - "obj_decoder: only tri or quad meshes are supported (file '%s')", - resource->name); - mj_deleteSpec(spec); - return nullptr; - } - - face_indices.push_back(obj_mesh.indices[idx]); - face_indices.push_back(obj_mesh.indices[idx + 1]); - face_indices.push_back(obj_mesh.indices[idx + 2]); - - if (nfacevert == 4) { - face_indices.push_back(obj_mesh.indices[idx]); - face_indices.push_back(obj_mesh.indices[idx + 2]); - face_indices.push_back(obj_mesh.indices[idx + 3]); - } - idx += nfacevert; - ++face; - } - - for (const auto& mesh_index : face_indices) { - userface.push_back(mesh_index.vertex_index); - - if (!usernormal.empty()) { - userfacenormal.push_back(mesh_index.normal_index); - } - - if (!usertexcoord.empty()) { - userfacetexcoord.push_back(mesh_index.texcoord_index); - } - } - } - - for (size_t i = 0; i < usertexcoord.size() / 2; i++) { - usertexcoord[2 * i + 1] = 1 - usertexcoord[2 * i + 1]; - } - - mjs_setString(mesh->file, resource->name); - mjs_setFloat(mesh->uservert, attrib.vertices.data(), attrib.vertices.size()); - mjs_setFloat(mesh->usernormal, usernormal.data(), usernormal.size()); - mjs_setFloat(mesh->usertexcoord, usertexcoord.data(), usertexcoord.size()); - mjs_setInt(mesh->userface, userface.data(), userface.size()); - mjs_setInt(mesh->userfacenormal, userfacenormal.data(), userfacenormal.size()); - mjs_setInt(mesh->userfacetexcoord, userfacetexcoord.data(), userfacetexcoord.size()); - - return spec; -} - -int CanDecode(const mjResource* resource) { - std::string_view name(resource->name); - return name.ends_with(".obj") || name.ends_with(".OBJ"); -} - -} // namespace - -mjPLUGIN_LIB_INIT { - mjpDecoder decoder; - mjp_defaultDecoder(&decoder); - decoder.content_type = "model/obj"; - decoder.extension = ".obj"; - decoder.decode = Decode; - decoder.can_decode = CanDecode; - mjp_registerDecoder(&decoder); -} diff --git a/python/mujoco/msh2obj_test.py b/python/mujoco/msh2obj_test.py index 2fd1bdf3..97166db4 100644 --- a/python/mujoco/msh2obj_test.py +++ b/python/mujoco/msh2obj_test.py @@ -14,7 +14,6 @@ # ============================================================================== """Tests for msh2obj.py.""" - from absl.testing import absltest from etils import epath import mujoco diff --git a/src/user/CMakeLists.txt b/src/user/CMakeLists.txt index 67025afc..bd93c028 100644 --- a/src/user/CMakeLists.txt +++ b/src/user/CMakeLists.txt @@ -38,3 +38,4 @@ set(MUJOCO_USER_SRCS ) target_sources(mujoco PRIVATE ${MUJOCO_USER_SRCS}) +target_compile_definitions(mujoco PRIVATE MUJOCO_TINYOBJLOADER_IMPL) diff --git a/src/user/user_mesh.cc b/src/user/user_mesh.cc index 2cb9d8bf..812eb0df 100644 --- a/src/user/user_mesh.cc +++ b/src/user/user_mesh.cc @@ -612,7 +612,10 @@ void mjCMesh::ProcessVertices(const std::vector& vert, bool remove_repeat } } - +bool mjCMesh::IsObj(std::string_view filename, std::string_view ct) { + std::string asset_type = GetAssetContentType(filename, ct); + return asset_type == "model/obj"; +} bool mjCMesh::IsSTL(std::string_view filename, std::string_view ct) { std::string asset_type = GetAssetContentType(filename, ct); @@ -624,7 +627,9 @@ bool mjCMesh::IsMSH(std::string_view filename, std::string_view ct) { return asset_type == "model/vnd.mujoco.msh"; } - +bool mjCMesh::IsObj() const { + return content_type_ == "model/obj"; +} bool mjCMesh::IsSTL() const { return content_type_ == "model/stl"; @@ -634,62 +639,28 @@ bool mjCMesh::IsMSH() const { return content_type_ == "model/vnd.mujoco.msh"; } -// load mesh using decoder plugin -void mjCMesh::LoadFromDecoder(mjResource* resource, bool remove_repeated) { - const mjpDecoder* decoder = mjp_findDecoder(resource, content_type_.c_str()); - if (!decoder) { - throw mjCError(this, "no decoder found for mesh file '%s'", resource->name); - } - mjSpec* mesh_spec = decoder->decode(resource, nullptr); - if (!mesh_spec) { - throw mjCError(this, "decoder failed for mesh file '%s'", resource->name); - } - mjsElement* elem = mjs_firstElement(mesh_spec, mjOBJ_MESH); - if (elem) { - mjsMesh* src_mesh = mjs_asMesh(elem); - if (src_mesh) { - normal_.assign(src_mesh->usernormal->begin(), src_mesh->usernormal->end()); - texcoord_.assign(src_mesh->usertexcoord->begin(), src_mesh->usertexcoord->end()); - face_.assign(src_mesh->userface->begin(), src_mesh->userface->end()); - facenormal_.assign(src_mesh->userfacenormal->begin(), src_mesh->userfacenormal->end()); - facetexcoord_.assign(src_mesh->userfacetexcoord->begin(), src_mesh->userfacetexcoord->end()); - - // correct winding order for left-handed coordinate systems - bool righthand = scale[0] * scale[1] * scale[2] > 0; - if (!righthand) { - for (size_t i = 0; i < face_.size(); i += 3) { - std::swap(face_[i + 1], face_[i + 2]); - } - for (size_t i = 0; i < facenormal_.size(); i += 3) { - std::swap(facenormal_[i + 1], facenormal_[i + 2]); - } - for (size_t i = 0; i < facetexcoord_.size(); i += 3) { - std::swap(facetexcoord_[i + 1], facetexcoord_[i + 2]); - } - } - - std::vector vert(src_mesh->uservert->begin(), src_mesh->uservert->end()); - mj_deleteSpec(mesh_spec); - ProcessVertices(vert, remove_repeated); - return; - } - } - mj_deleteSpec(mesh_spec); -} // load mesh from resource; throw error on failure void mjCMesh::LoadFromResource(mjResource* resource, bool remove_repeated) { // set content type from resource name std::string asset_type = GetAssetContentType(resource->name, content_type_); + if (asset_type.empty()) { + if (!content_type_.empty()) { + throw mjCError(this, "invalid content type: '%s'", content_type_.c_str()); + } + throw mjCError(this, "unknown or unsupported mesh file: '%s'", resource->name); + } content_type_ = asset_type; if (IsSTL()) { LoadSTL(resource); + } else if (IsObj()) { + LoadOBJ(resource, remove_repeated); } else if (IsMSH()) { LoadMSH(resource, remove_repeated); } else { - LoadFromDecoder(resource, remove_repeated); + throw mjCError(this, "unsupported mesh type: '%s'", asset_type.c_str()); } } @@ -1038,6 +1009,83 @@ void mjCMesh::FitGeom(mjCGeom* geom, double center[3]) { } + +// load OBJ mesh +void mjCMesh::LoadOBJ(mjResource* resource, bool remove_repeated) { + tinyobj::ObjReader objReader; + const void* bytes = nullptr; + + int buffer_sz = mju_readResource(resource, &bytes); + if (buffer_sz < 0) { + throw mjCError(this, "could not read OBJ file '%s'", resource->name); + } + + // TODO(etom): support .mtl files? + const char* buffer = (const char*) bytes; + objReader.ParseFromString(std::string(buffer, buffer_sz), std::string()); + + if (!objReader.Valid()) { + throw mjCError(this, "could not parse OBJ file '%s'", resource->name); + } + + const auto& attrib = objReader.GetAttrib(); + normal_ = attrib.normals; + texcoord_ = attrib.texcoords; + facenormal_.clear(); + facetexcoord_.clear(); + + if (!objReader.GetShapes().empty()) { + const auto& mesh = objReader.GetShapes()[0].mesh; + bool righthand = scale[0] * scale[1] * scale[2] > 0; + + // iterate over mesh faces + std::vector face_indices; + for (int face = 0, idx = 0; idx < mesh.indices.size();) { + int nfacevert = mesh.num_face_vertices[face]; + if (nfacevert < 3 || nfacevert > 4) { + throw mjCError( + this, "only tri or quad meshes are supported for OBJ (file '%s')", + resource->name); + } + + face_indices.push_back(mesh.indices[idx]); + face_indices.push_back(mesh.indices[idx + (righthand == 1 ? 1 : 2)]); + face_indices.push_back(mesh.indices[idx + (righthand == 1 ? 2 : 1)]); + + if (nfacevert == 4) { + face_indices.push_back(mesh.indices[idx]); + face_indices.push_back(mesh.indices[idx + (righthand == 1 ? 2 : 3)]); + face_indices.push_back(mesh.indices[idx + (righthand == 1 ? 3 : 2)]); + } + idx += nfacevert; + ++face; + } + + // for each vertex, store index, normal, and texcoord + for (const auto& mesh_index : face_indices) { + face_.push_back(mesh_index.vertex_index); + + if (!normal_.empty()) { + facenormal_.push_back(mesh_index.normal_index); + } + + if (!texcoord_.empty()) { + facetexcoord_.push_back(mesh_index.texcoord_index); + } + } + } + + // flip the second texcoord + for (int i=0; i < texcoord_.size()/2; i++) { + texcoord_[2*i+1] = 1-texcoord_[2*i+1]; + } + + // copy vertex data + ProcessVertices(attrib.vertices, remove_repeated); +} + + + // load mesh from cached asset, return true on success bool mjCMesh::LoadCachedMesh(mjCCache *cache, const mjResource* resource) { auto process_mesh = [&](const void* data) { @@ -1728,7 +1776,7 @@ void mjCMesh::CheckInitialMesh() const { // check texcoord size if no face texcoord indices are given if (!texcoord_.empty() && texcoord_.size() != 2 * nvert() && - facetexcoord_.empty() && content_type_ != "model/obj") { + facetexcoord_.empty() && !IsObj()) { throw mjCError(this, "texcoord must be 2*nv if face texcoord indices are not provided in an OBJ file"); } diff --git a/src/user/user_objects.h b/src/user/user_objects.h index 88599740..8399a146 100644 --- a/src/user/user_objects.h +++ b/src/user/user_objects.h @@ -22,7 +22,6 @@ #include #include #include -#include #include #include #include @@ -35,6 +34,10 @@ #include #include "user/user_cache.h" #include "user/user_util.h" +#include + +using face_vertices_type = + decltype(tinyobj::mesh_t::num_face_vertices)::value_type; // forward declarations of all mjC/X classes class mjCError; @@ -1238,11 +1241,11 @@ class mjCMesh: public mjCMesh_, private mjsMesh { // load from OBJ, STL, or MSH file; throws mjCError on failure void LoadFromResource(mjResource* resource, bool remove_repeated = false); - + static bool IsObj(std::string_view filename, std::string_view ct = ""); static bool IsSTL(std::string_view filename, std::string_view ct = ""); static bool IsMSH(std::string_view filename, std::string_view ct = ""); - + bool IsObj() const; bool IsSTL() const; bool IsMSH() const; @@ -1259,7 +1262,7 @@ class mjCMesh: public mjCMesh_, private mjsMesh { void ProcessVertices(const std::vector& vert, bool remove_repeated = false); - void LoadFromDecoder(mjResource* resource, bool remove_repeated); // load mesh using decoder plugin + void LoadOBJ(mjResource* resource, bool remove_repeated); // load mesh in wavefront OBJ format void LoadSTL(mjResource* resource); // load mesh in STL BIN format void LoadMSH(mjResource* resource, bool remove_repeated); // load mesh in MSH BIN format diff --git a/test/user/CMakeLists.txt b/test/user/CMakeLists.txt index 7cbc667f..c176c3bd 100644 --- a/test/user/CMakeLists.txt +++ b/test/user/CMakeLists.txt @@ -12,13 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -mujoco_test( - user_model_test - PROPERTIES - ENVIRONMENT - "MUJOCO_PLUGIN_DIR=$" - ADDITIONAL_LINK_LIBRARIES absl::str_format -) +mujoco_test(user_model_test ADDITIONAL_LINK_LIBRARIES absl::str_format) mujoco_test(user_objects_test) @@ -29,20 +23,9 @@ mujoco_test( "MUJOCO_PLUGIN_DIR=$" ) -mujoco_test( - user_flex_test - PROPERTIES - ENVIRONMENT - "MUJOCO_PLUGIN_DIR=$" -) +mujoco_test(user_flex_test) -mujoco_test( - user_mesh_test - PROPERTIES - ENVIRONMENT - "MUJOCO_PLUGIN_DIR=$" - ADDITIONAL_LINK_LIBRARIES absl::str_format -) +mujoco_test(user_mesh_test ADDITIONAL_LINK_LIBRARIES absl::str_format) mujoco_test(user_composite_test) diff --git a/test/user/user_mesh_test.cc b/test/user/user_mesh_test.cc index 80eb76ef..88b4eed0 100644 --- a/test/user/user_mesh_test.cc +++ b/test/user/user_mesh_test.cc @@ -71,9 +71,6 @@ static const char* const kMalformedFaceOBJPath = static const char* const kCubeSkinPath = "user/testdata/cube_skin.xml"; -static const char* const kNoDecoderForMeshErrorMsh = - "no decoder found for mesh"; - using ::testing::ElementsAre; using ::testing::HasSubstr; using ::testing::IsNull; @@ -113,7 +110,7 @@ TEST_F(MjCMeshTest, UnknownMeshFormat) { LoadModelFromString(xml.c_str(), error.data(), error.size(), &vfs); ASSERT_THAT(model, testing::IsNull()) << "Should fail to load a mesh named: " << name; - EXPECT_THAT(error.data(), HasSubstr(kNoDecoderForMeshErrorMsh)); + EXPECT_THAT(error.data(), HasSubstr("unknown or unsupported mesh file: ")); EXPECT_THAT(error.data(), HasSubstr(name)); } @@ -303,7 +300,7 @@ TEST_F(MjCMeshTest, LoadMSHWithContentTypeError) { // should error with unknown file type mjModel* model = LoadModelFromString(xml, error, error_sz, &vfs); EXPECT_THAT(model, IsNull()); - EXPECT_THAT(error, HasSubstr(kNoDecoderForMeshErrorMsh)); + EXPECT_THAT(error, HasSubstr("unsupported mesh type: 'model/unknown'")); mj_deleteVFS(&vfs); } @@ -330,7 +327,7 @@ TEST_F(MjCMeshTest, LoadMSHWithInvalidContentType) { // should error with unknown file type mjModel* model = LoadModelFromString(xml, error, error_sz, &vfs); EXPECT_THAT(model, IsNull()); - EXPECT_THAT(error, HasSubstr(kNoDecoderForMeshErrorMsh)); + EXPECT_THAT(error, HasSubstr("invalid content type: 'model'")); mj_deleteVFS(&vfs); } diff --git a/test/xml/CMakeLists.txt b/test/xml/CMakeLists.txt index 63348c62..2815dcb3 100644 --- a/test/xml/CMakeLists.txt +++ b/test/xml/CMakeLists.txt @@ -14,12 +14,7 @@ mujoco_test(xml_api_test) -mujoco_test( - xml_native_reader_test - PROPERTIES - ENVIRONMENT - "MUJOCO_PLUGIN_DIR=$" -) +mujoco_test(xml_native_reader_test) mujoco_test(xml_utils_test)