From 3d76bcb68a493bfb56d0bab5cde9cc72f578ab2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 5 Jul 2022 09:59:00 +0200 Subject: [PATCH] ObjImporter: support negative indices. What the hell, why even is that a thing? Especially given it's never actually using the feature in a way that would make the file smaller. --- doc/changelog.dox | 1 + src/MagnumPlugins/ObjImporter/ObjImporter.cpp | 38 +++++++---- src/MagnumPlugins/ObjImporter/ObjImporter.h | 5 ++ .../ObjImporter/Test/CMakeLists.txt | 1 + .../ObjImporter/Test/ObjImporterTest.cpp | 63 ++++++++++++++++++- .../ObjImporter/Test/invalid-numbers.obj | 5 +- .../Test/mesh-negative-indices.obj | 35 +++++++++++ 7 files changed, 135 insertions(+), 13 deletions(-) create mode 100644 src/MagnumPlugins/ObjImporter/Test/mesh-negative-indices.obj diff --git a/doc/changelog.dox b/doc/changelog.dox index 0ce3d5a83..0e3981ddb 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -1097,6 +1097,7 @@ See also: consistent with @ref Trade::SceneData::findFieldId(). - Added @ref Trade::MeshData::attributeId() that returns ID of an attribute in a set of attributes of the same name +- @relativeref{Trade,ObjImporter} now supports negative indices - Added @ref Trade::TextureType::Texture1DArray, @relativeref{Trade::TextureType,Texture2DArray} and @relativeref{Trade::TextureType,CubeMapArray} in order to be able to diff --git a/src/MagnumPlugins/ObjImporter/ObjImporter.cpp b/src/MagnumPlugins/ObjImporter/ObjImporter.cpp index e474b655b..e5c5af798 100644 --- a/src/MagnumPlugins/ObjImporter/ObjImporter.cpp +++ b/src/MagnumPlugins/ObjImporter/ObjImporter.cpp @@ -26,6 +26,7 @@ #include "ObjImporter.h" +#include #include #include #include @@ -109,7 +110,7 @@ inline bool parseFloat(const char* const errorPrefix, const Containers::StringVi return true; } -inline bool parseUnsignedInt(const char* const errorPrefix, const Containers::StringView string, UnsignedInt& out) { +inline bool parseInt(const char* const errorPrefix, const Containers::StringView string, Int& out) { /** @todo replace with something that can parse non-null-terminated stuff, then drop this "too long" error */ char buffer[128]; @@ -122,17 +123,17 @@ inline bool parseUnsignedInt(const char* const errorPrefix, const Containers::St std::memcpy(buffer, string.data(), size); buffer[size] = '\0'; char* end; - /* Not using strtoul() here as on Windows it's 32-bit and we wouldn't be + /* Not using strtol() here as on Windows it's 32-bit and we wouldn't be able to detect overflows */ /** @todo replace with something that can report errors in a non-insane way */ - const std::uint64_t outLong = std::strtoull(buffer, &end, 10); + const std::int64_t outLong = std::strtoll(buffer, &end, 10); if(!string || std::size_t(end - buffer) != size) { Error{} << errorPrefix << "invalid integer literal" << string; return false; } - if(outLong > ~std::uint32_t{}) { - Error{} << errorPrefix << "too large integer literal" << string; + if(outLong < INT_MIN || outLong > INT_MAX) { + Error{} << errorPrefix << "too small or large integer literal" << string; return false; } @@ -399,9 +400,14 @@ Containers::Optional ObjImporter::doMesh(const UnsignedInt id, Unsigne /* The number before first slash is a position index */ const Containers::StringView foundSlash1 = indexTuple.findOr('/', indexTuple.end()); - if(!parseUnsignedInt("Trade::ObjImporter::mesh():", indexTuple.prefix(foundSlash1.begin()), data[i][0])) + Int index; + if(!parseInt("Trade::ObjImporter::mesh():", indexTuple.prefix(foundSlash1.begin()), index)) return {}; - data[i][0] -= mesh.positionIndexOffset; + /* If the number is negative, it counts from the end (-1 is + the last known position at this point, counting from 1) */ + if(index < 0) + index += positions.size() + 1; + data[i][0] = index - mesh.positionIndexOffset; /* If there was a slash, next is a texture coordinate or empty */ @@ -409,18 +415,28 @@ Containers::Optional ObjImporter::doMesh(const UnsignedInt id, Unsigne indexTuple = indexTuple.suffix(foundSlash1.end()); const Containers::StringView foundSlash2 = indexTuple.findOr('/', indexTuple.end()); if(!foundSlash2 || foundSlash2.begin() != indexTuple.begin()) { - if(!parseUnsignedInt("Trade::ObjImporter::mesh():", indexTuple.prefix(foundSlash2.begin()), data[i][1])) + if(!parseInt("Trade::ObjImporter::mesh():", indexTuple.prefix(foundSlash2.begin()), index)) return {}; - data[i][1] -= mesh.textureCoordinateIndexOffset; + /* If the number is negative, it counts from the end + (-1 is the last known texture coordinate at this + point, counting from 1) */ + if(index < 0) + index += textureCoordinates.size() + 1; + data[i][1] = index - mesh.textureCoordinateIndexOffset; ++textureCoordinateIndexCount; } /* If there was a second slash, last is a normal */ if(foundSlash2) { indexTuple = indexTuple.suffix(foundSlash2.end()); - if(!parseUnsignedInt("Trade::ObjImporter::mesh():", indexTuple, data[i][2])) + if(!parseInt("Trade::ObjImporter::mesh():", indexTuple, index)) return {}; - data[i][2] -= mesh.normalIndexOffset; + /* If the number is negative, it counts from the end + (-1 is the last known normal at this point, counting + from 1) */ + if(index < 0) + index += normals.size() + 1; + data[i][2] = index - mesh.normalIndexOffset; ++normalIndexCount; } } diff --git a/src/MagnumPlugins/ObjImporter/ObjImporter.h b/src/MagnumPlugins/ObjImporter/ObjImporter.h index 56c9a874c..60be371dd 100644 --- a/src/MagnumPlugins/ObjImporter/ObjImporter.h +++ b/src/MagnumPlugins/ObjImporter/ObjImporter.h @@ -105,6 +105,11 @@ Meshes are imported as @ref MeshPrimitive::Triangles with positions with optional @ref VertexFormat::Vector3 normals and @ref VertexFormat::Vector2 texture coordinates, if present in the source file. +Negative indices (where @cpp -1 @ce is the last position / texture coordinate +/ normal known at given point in the file, @cpp -2 @ce is the second-to-last, +etc.), produced for example by 3ds Max or [Mineways](http://mineways.com), are +supported. + Polygons (quads etc.) and material properties are currently not supported. */ class MAGNUM_OBJIMPORTER_EXPORT ObjImporter: public AbstractImporter { diff --git a/src/MagnumPlugins/ObjImporter/Test/CMakeLists.txt b/src/MagnumPlugins/ObjImporter/Test/CMakeLists.txt index 8e1b6c4f3..138af9d11 100644 --- a/src/MagnumPlugins/ObjImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/ObjImporter/Test/CMakeLists.txt @@ -60,6 +60,7 @@ corrade_add_test(ObjImporterTest ObjImporterTest.cpp mesh-named-first-unnamed.obj mesh-named-first-unnamed-index-first.obj mesh-named.obj + mesh-negative-indices.obj mesh-normals.obj mesh-positions-optional-coordinate.obj mesh-primitive-lines.obj diff --git a/src/MagnumPlugins/ObjImporter/Test/ObjImporterTest.cpp b/src/MagnumPlugins/ObjImporter/Test/ObjImporterTest.cpp index 084c8e90a..376eb64eb 100644 --- a/src/MagnumPlugins/ObjImporter/Test/ObjImporterTest.cpp +++ b/src/MagnumPlugins/ObjImporter/Test/ObjImporterTest.cpp @@ -56,6 +56,8 @@ struct ObjImporterTest: TestSuite::Tester { void meshNormals(); void meshTextureCoordinatesNormals(); + void meshNegativeIndices(); + void meshIgnoredKeyword(); void meshNamed(); @@ -120,7 +122,8 @@ const struct { } InvalidNumbersData[]{ {"too long float literal", "too long numeric literal 1234.567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567"}, {"too long integer literal", "too long numeric literal 12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678"}, - {"too large integer literal", "too large integer literal 4294967296"}, + {"too small integer literal", "too small or large integer literal -2147483649"}, + {"too large integer literal", "too small or large integer literal 2147483648"}, {"invalid float literal", "invalid floating-point literal bleh"}, {"invalid integer literal", "invalid integer literal bleh"}, {"position index out of range", "index 3 out of range for 1 vertices"}, @@ -195,6 +198,8 @@ ObjImporterTest::ObjImporterTest() { &ObjImporterTest::meshNormals, &ObjImporterTest::meshTextureCoordinatesNormals, + &ObjImporterTest::meshNegativeIndices, + &ObjImporterTest::meshIgnoredKeyword, &ObjImporterTest::meshNamed}); @@ -448,6 +453,62 @@ void ObjImporterTest::meshTextureCoordinatesNormals() { TestSuite::Compare::Container); } +void ObjImporterTest::meshNegativeIndices() { + Containers::Pointer importer = _manager.instantiate("ObjImporter"); + CORRADE_VERIFY(importer->openFile(Utility::Path::join(OBJIMPORTER_TEST_DIR, "mesh-negative-indices.obj"))); + CORRADE_COMPARE(importer->meshCount(), 1); + + const Containers::Optional data = importer->mesh(0); + CORRADE_VERIFY(data); + CORRADE_COMPARE(data->primitive(), MeshPrimitive::Lines); + CORRADE_COMPARE(data->attributeCount(), 3); + CORRADE_COMPARE_AS(data->attribute(MeshAttribute::Position), + Containers::arrayView({ + {0.5f, 2.0f, 3.0f}, + {0.0f, 1.5f, 1.0f}, + {0.5f, 2.0f, 3.0f}, + {0.0f, 1.5f, 1.0f}, + {0.0f, 1.5f, 1.0f}, + /* The first 5 elements should be the same as in + meshTextureCoordinatesNormals() */ + {2.0f, 1.5f, 0.0f}, + {1.5f, 0.0f, 2.0f} + }), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(data->attribute(MeshAttribute::TextureCoordinates), + Containers::arrayView({ + {1.0f, 0.5f}, + {1.0f, 0.5f}, + {0.5f, 1.0f}, + {0.5f, 1.0f}, + {0.5f, 1.0f}, + /* The first 5 elements should be the same as in + meshTextureCoordinatesNormals() */ + {0.5f, 1.0f}, + {0.0f, 0.5f} + }), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(data->attribute(MeshAttribute::Normal), + Containers::arrayView({ + {1.0f, 0.5f, 3.5f}, + {0.5f, 1.0f, 0.5f}, + {0.5f, 1.0f, 0.5f}, + {1.0f, 0.5f, 3.5f}, + {0.5f, 1.0f, 0.5f}, + /* The first 5 elements should be the same as in + meshTextureCoordinatesNormals() */ + {0.5f, 1.0f, 0.5f}, + {1.0f, 0.5f, 3.5f} + }), TestSuite::Compare::Container); + CORRADE_VERIFY(data->isIndexed()); + CORRADE_COMPARE(data->indexType(), MeshIndexType::UnsignedInt); + CORRADE_COMPARE_AS(data->indices(), + Containers::arrayView({ + 0, 1, 2, 3, 1, 0, 4, 2, + /* The first 8 elements should be the same as in + meshTextureCoordinatesNormals() */ + 5, 6 + }), TestSuite::Compare::Container); +} + void ObjImporterTest::meshIgnoredKeyword() { Containers::Pointer importer = _manager.instantiate("ObjImporter"); CORRADE_VERIFY(importer->openFile(Utility::Path::join(OBJIMPORTER_TEST_DIR, "mesh-ignored-keyword.obj"))); diff --git a/src/MagnumPlugins/ObjImporter/Test/invalid-numbers.obj b/src/MagnumPlugins/ObjImporter/Test/invalid-numbers.obj index 7bdbe1914..b3786b7af 100644 --- a/src/MagnumPlugins/ObjImporter/Test/invalid-numbers.obj +++ b/src/MagnumPlugins/ObjImporter/Test/invalid-numbers.obj @@ -4,8 +4,11 @@ v 1234.5678901234567890123456789012345678901234567890123456789012345678901234567 o too long integer literal p 12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678 +o too small integer literal +p -2147483649 + o too large integer literal -p 4294967296 +p 2147483648 o invalid float literal v 1 bleh 2 diff --git a/src/MagnumPlugins/ObjImporter/Test/mesh-negative-indices.obj b/src/MagnumPlugins/ObjImporter/Test/mesh-negative-indices.obj new file mode 100644 index 000000000..220c3ec4b --- /dev/null +++ b/src/MagnumPlugins/ObjImporter/Test/mesh-negative-indices.obj @@ -0,0 +1,35 @@ +# OBJ files produced by http://mineways.com have this, for example the Rungholt +# scene from the McGuire archive (https://casual-effects.com/data/) has this. +# Not sure what's the point of having such a feature or even using such a +# feature, since the file starts with +# +# f -53570/-200/-18 -53569/-199/-18 -53568/-198/-18 -53567/-197/-18 +# +# and then the numbers just decrease going forward. I would see a point when +# the face and vertex declarations were interleaved -- to make the indices +# shorter -- but not like this. Honestly this looks like once upon a time, +# someone just randomly flipped signs until the export-import cycle started +# making sense, and then the other importers had to follow suit?! The same +# insanity is exported by 3dsMax: https://github.com/CGAL/cgal/issues/4062 + +v 0.5 2 3 +v 0 1.5 1 +vt 1 0.5 +vt 0.5 1 +vn 1 0.5 3.5 +vn 0.5 1 0.5 + +# -2 is 1, -1 is 2 for all +l -2/-2/-2 -1/-2/-1 +l -2/-1/-1 -1/-1/-2 + +v 1.5 0 2 +v 2 1.5 0 +vt 0 0.5 + +# -4 is 1, -3 is 2, -2 is 3, -1 is 4 for positions +# -3 is 1, -2 is 2, -1 is 3 for texture coordinates +# -2 is 1, -1 is 2 for normals +l -3/-3/-1 -4/-3/-2 +l -3/-2/-1 -4/-2/-1 +l -1/-2/-1 -2/-1/-2