diff --git a/src/Magnum/SceneTools/Test/SceneConverterTest.cpp b/src/Magnum/SceneTools/Test/SceneConverterTest.cpp index 80d47b487..49dc67f36 100644 --- a/src/Magnum/SceneTools/Test/SceneConverterTest.cpp +++ b/src/Magnum/SceneTools/Test/SceneConverterTest.cpp @@ -1109,7 +1109,7 @@ const struct { Utility::Path::join(SCENETOOLS_TEST_OUTPUT_DIR, "SceneConverterTestFiles/whatever.ply") }}, "ObjImporter", nullptr, nullptr, nullptr, - "Trade::ObjImporter::mesh(): wrong index count for point\n" + "Trade::ObjImporter::mesh(): expected exactly 1 position index tuple for a point, got 5 5\n" "Cannot import the mesh\n"}, {"can't import a mesh for concatenation", {InPlaceInit, { "-I", "ObjImporter", "--concatenate-meshes", @@ -1117,7 +1117,7 @@ const struct { Utility::Path::join(SCENETOOLS_TEST_OUTPUT_DIR, "SceneConverterTestFiles/whatever.ply") }}, "ObjImporter", nullptr, nullptr, nullptr, - "Trade::ObjImporter::mesh(): wrong index count for point\n" + "Trade::ObjImporter::mesh(): expected exactly 1 position index tuple for a point, got 5 5\n" "Cannot import mesh 0\n"}, {"can't import a scene for concatenation", {InPlaceInit, { /** @todo change to an OBJ once ObjImporter imports materials (and @@ -1135,7 +1135,7 @@ const struct { Utility::Path::join(SCENETOOLS_TEST_OUTPUT_DIR, "SceneConverterTestFiles/whatever.ply") }}, "ObjImporter", nullptr, nullptr, nullptr, - "Trade::ObjImporter::mesh(): wrong index count for point\n" + "Trade::ObjImporter::mesh(): expected exactly 1 position index tuple for a point, got 5 5\n" "Cannot import mesh 0\n"}, {"invalid mesh attribute filter", {InPlaceInit, { /** @todo drop --mesh once it's not needed anymore again */ @@ -1188,7 +1188,7 @@ const struct { Utility::Path::join(SCENETOOLS_TEST_OUTPUT_DIR, "SceneConverterTestFiles/whatever.ply") }}, "ObjImporter", nullptr, "StanfordSceneConverter", nullptr, - "Trade::ObjImporter::mesh(): wrong index count for point\n" + "Trade::ObjImporter::mesh(): expected exactly 1 position index tuple for a point, got 5 5\n" "Cannot add importer contents\n"}, {"can't add processed meshes", {InPlaceInit, { "-I", "ObjImporter", "-C", "StanfordSceneConverter", diff --git a/src/MagnumPlugins/ObjImporter/CMakeLists.txt b/src/MagnumPlugins/ObjImporter/CMakeLists.txt index 7d094398c..c3147d85f 100644 --- a/src/MagnumPlugins/ObjImporter/CMakeLists.txt +++ b/src/MagnumPlugins/ObjImporter/CMakeLists.txt @@ -45,13 +45,6 @@ if(MAGNUM_OBJIMPORTER_BUILD_STATIC AND MAGNUM_BUILD_STATIC_PIC) set_target_properties(ObjImporter PROPERTIES POSITION_INDEPENDENT_CODE ON) endif() target_link_libraries(ObjImporter PUBLIC MagnumTrade MagnumMeshTools) -if(CORRADE_TARGET_EMSCRIPTEN) - # Since 1.39.0, this needs to be enabled on the plugin. Before (on 1.38.44 - # at least) this wasn't needed, at least for the test -- having it enabled - # for the test binary and in the linker was enough. Most probably related - # to fastcomp->llvm backend switch. - set_property(TARGET ObjImporter APPEND_STRING PROPERTY COMPILE_FLAGS " -s DISABLE_EXCEPTION_CATCHING=0") -endif() install(FILES ObjImporter.h ${CMAKE_CURRENT_BINARY_DIR}/configure.h DESTINATION ${MAGNUM_PLUGINS_INCLUDE_INSTALL_DIR}/ObjImporter) diff --git a/src/MagnumPlugins/ObjImporter/ObjImporter.cpp b/src/MagnumPlugins/ObjImporter/ObjImporter.cpp index c33a1d5eb..49d4e4f13 100644 --- a/src/MagnumPlugins/ObjImporter/ObjImporter.cpp +++ b/src/MagnumPlugins/ObjImporter/ObjImporter.cpp @@ -26,17 +26,13 @@ #include "ObjImporter.h" -#include -#include -#include #include #include #include #include #include -#include /** @todo remove once iostream is dropped */ -#include -#include +#include +#include #include "Magnum/Mesh.h" #include "Magnum/MeshTools/RemoveDuplicates.h" @@ -46,52 +42,103 @@ namespace Magnum { namespace Trade { +using namespace Containers::Literals; + namespace { struct Mesh { - std::streampos begin; - std::streampos end; + /* Points to File::fileData, the end is implicitly `(this + 1)->begin` */ + const char* begin; + /* Name of the mesh. The first mesh has a name only if it appears before + any data line (v/vt/vn/... or p/l/f/...). */ + Containers::StringView name; + /* Offset of the first position, texture coordinate and normal index. + Assuming that not only vertex data but also index data follow the mesh + name, this way we don't have to parse the whole file if just a single + mesh out of many is requested. */ UnsignedInt positionIndexOffset; UnsignedInt textureCoordinateIndexOffset; UnsignedInt normalIndexOffset; - std::string name; }; } struct ObjImporter::File { - std::unordered_map meshesForName; + std::unordered_map meshesForName; + /* Contains always n + 1 entries, with the last entry being an upper bound + on the file range and index offsets */ Containers::Array meshes; - Containers::Pointer in; + Containers::Array fileData; }; namespace { -void ignoreLine(std::istream& in) { - in.ignore(std::numeric_limits::max(), '\n'); -} - -template Math::Vector extractFloatData(const std::string& str, Float* extra = nullptr) { - std::vector data = Utility::String::splitWithoutEmptyParts(str, ' '); - if(data.size() < size || data.size() > size + (extra ? 1 : 0)) { - Error() << "Trade::ObjImporter::mesh(): invalid float array size"; - throw 0; +/* The spec doesn't say anything explicit about whitespace, but at the very + least the examples at http://paulbourke.net/dataformats/obj/ pad the values + with whitespace and various tools are also producing such files, as reported + at https://forum.babylonjs.com/t/extra-whitespace-breaks-obj-parsing/5244 + + Besides space I'm considering a tab and a CR character. *Not* a newline, + since that is a significant delimiter that has to be treated separately. */ +constexpr Containers::StringView Whitespace = " \t\r"_s; + +/* Mostly just a copy of Corrade's Utility::Json::parseFloatInternal() and + parseUnsignedIntInternal() */ +/** @todo make a common API in Corrade once we have something that can parse + numbers like a grownup */ +inline bool parseFloat(const char* const errorPrefix, const Containers::StringView string, Float& out) { + /** @todo replace with something that can parse non-null-terminated stuff, + then drop this "too long" error */ + char buffer[128]; + const std::size_t size = string.size(); + if(size > Containers::arraySize(buffer) - 1) { + Error{} << errorPrefix << "too long numeric literal" << string; + return false; } - Math::Vector output; + std::memcpy(buffer, string.data(), size); + buffer[size] = '\0'; + char* end; + out = std::strtof(buffer, &end); + if(!string || std::size_t(end - buffer) != size) { + Error{} << errorPrefix << "invalid floating-point literal" << string; + return false; + } - for(std::size_t i = 0; i != size; ++i) - output[i] = std::stof(data[i]); + /* Success; value already written above */ + return true; +} - if(data.size() == size+1) { - /* This should be obvious from the first if, but add this just to make - Clang Analyzer happy */ - CORRADE_INTERNAL_ASSERT(extra); +inline bool parseUnsignedInt(const char* const errorPrefix, const Containers::StringView string, UnsignedInt& out) { + /** @todo replace with something that can parse non-null-terminated stuff, + then drop this "too long" error */ + char buffer[128]; + const std::size_t size = string.size(); + if(size > Containers::arraySize(buffer) - 1) { + Error{} << errorPrefix << "too long numeric literal" << string; + return false; + } - *extra = std::stof(data.back()); + 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 + 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); + 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; + return false; } - return output; + /* On success convert the value to 32 bits */ + out = outLong; + return true; } } @@ -104,84 +151,72 @@ ObjImporter::~ObjImporter() = default; ImporterFeatures ObjImporter::doFeatures() const { return ImporterFeature::OpenData; } -void ObjImporter::doClose() { _file.reset(); } +void ObjImporter::doClose() { + _file = {}; +} bool ObjImporter::doIsOpened() const { return !!_file; } -void ObjImporter::doOpenFile(const Containers::StringView filename) { - /** @todo ARGH clean this up, won't work with UTF-8 */ - Containers::Pointer in{new std::ifstream{filename, std::ios::binary}}; - if(!in->good()) { - Error() << "Trade::ObjImporter::openFile(): cannot open file" << filename; - return; - } - +void ObjImporter::doOpenData(Containers::Array&& data, const DataFlags dataFlags) { _file.reset(new File); - _file->in = Utility::move(in); - parseMeshNames(); -} - -void ObjImporter::doOpenData(Containers::Array&& data, DataFlags) { - _file.reset(new File); - /** @todo ARGH MY EYES what is this cursed thing, burn it to the ground */ - _file->in.reset(new std::istringstream{{data.begin(), data.size()}}); - parseMeshNames(); -} + /* Copy file content. Take over the existing array or copy the data if we + can't. We need to keep the data around as JSON tokens are views onto it + and also for the GLB binary chunk. */ + if(dataFlags & (DataFlag::Owned|DataFlag::ExternallyOwned)) { + _file->fileData = std::move(data); + } else { + _file->fileData = Containers::Array{NoInit, data.size()}; + Utility::copy(data, _file->fileData); + } -void ObjImporter::parseMeshNames() { - /* First mesh starts at the beginning, its indices start from 1. The end - offset will be updated to proper value later. */ + /* First mesh starts at the beginning, its indices start from 1 */ UnsignedInt positionIndexOffset = 1; UnsignedInt normalIndexOffset = 1; UnsignedInt textureCoordinateIndexOffset = 1; + arrayAppend(_file->meshes, InPlaceInit, _file->fileData.begin(), Containers::StringView{}, positionIndexOffset, textureCoordinateIndexOffset, normalIndexOffset); + /* The first mesh doesn't have name by default but we might find it later, so we need to track whether there are any data before first name */ bool thisIsFirstMeshAndItHasNoData = true; - arrayAppend(_file->meshes, InPlaceInit, 0, 0, positionIndexOffset, normalIndexOffset, textureCoordinateIndexOffset, std::string{}); - while(_file->in->good()) { - /* The previous object might end at the beginning of this line */ - const std::streampos end = _file->in->tellg(); + /** @todo check size < 1G on 32b? currently it'd just assert, but it's + unlikely that such amount of contiguous memory would even be available + there, so ¯\_(ツ)_/¯ */ + Containers::StringView in{_file->fileData, _file->fileData.size()}; + while(in) { + /* Get a (trimmed) line from the input */ + const Containers::StringView lineEnd = in.findOr('\n', in.end()); + const Containers::StringView line = in.prefix(lineEnd.begin()).trimmed(Whitespace); + in = in.suffix(lineEnd.end()); - /* Comment line */ - if(_file->in->peek() == '#') { - ignoreLine(*_file->in); - continue; - } + /* Comment or empty line, skip */ + if(!line || line.hasPrefix('#')) continue; /* Parse the keyword */ - std::string keyword; - *_file->in >> keyword; + const Containers::StringView keywordEnd = line.findAnyOr(Whitespace, line.end()); + const Containers::StringView keyword = line.prefix(keywordEnd.begin()); /* Mesh name */ - if(keyword == "o") { - std::string name; - std::getline(*_file->in, name); - name = Utility::String::trim(name); + if(keyword == "o"_s) { + const Containers::StringView name = line.suffix(keywordEnd.end()).trimmed(Whitespace); /* This is the name of first mesh */ if(thisIsFirstMeshAndItHasNoData) { thisIsFirstMeshAndItHasNoData = false; /* Update its name and add it to name map */ - if(!name.empty()) + if(name) _file->meshesForName.emplace(name, _file->meshes.size() - 1); - _file->meshes.back().name = Utility::move(name); - - /* Update its begin offset to be more precise */ - _file->meshes.back().begin = _file->in->tellg(); + _file->meshes.back().name = name; /* Otherwise this is a name of new mesh */ } else { - /* Set end of the previous one */ - _file->meshes.back().end = end; - /* Save name and offset of the new one. The end offset will be updated later. */ - if(!name.empty()) + if(name) _file->meshesForName.emplace(name, _file->meshes.size()); - arrayAppend(_file->meshes, InPlaceInit, _file->in->tellg(), 0, positionIndexOffset, textureCoordinateIndexOffset, normalIndexOffset, Utility::move(name)); + arrayAppend(_file->meshes, InPlaceInit, in.begin(), name, positionIndexOffset, textureCoordinateIndexOffset, normalIndexOffset); } continue; @@ -190,43 +225,40 @@ void ObjImporter::parseMeshNames() { the first object is unnamed. We need to check for them. */ /* Vertex data, update index offset for the following meshes */ - } else if(keyword == "v") { + } else if(keyword == "v"_s) { ++positionIndexOffset; thisIsFirstMeshAndItHasNoData = false; - } else if(keyword == "vt") { + } else if(keyword == "vt"_s) { ++textureCoordinateIndexOffset; thisIsFirstMeshAndItHasNoData = false; - } else if(keyword == "vn") { + } else if(keyword == "vn"_s) { ++normalIndexOffset; thisIsFirstMeshAndItHasNoData = false; /* Index data, just mark that we found something for first unnamed object */ - } else if(thisIsFirstMeshAndItHasNoData) for(const std::string data: {"p", "l", "f"}) { - if(keyword == data) { - thisIsFirstMeshAndItHasNoData = false; - break; - } + } else if(keyword == "p"_s || + keyword == "l"_s || + keyword == "f"_s) { + thisIsFirstMeshAndItHasNoData = false; } - - /* Ignore the rest of the line */ - ignoreLine(*_file->in); } - /* Set end of the last object */ - _file->in->clear(); - _file->in->seekg(0, std::ios::end); - _file->meshes.back().end = _file->in->tellg(); + /* Save the final offset so we have an upper bound on index offsets */ + arrayAppend(_file->meshes, InPlaceInit, in.begin(), Containers::StringView{}, positionIndexOffset, textureCoordinateIndexOffset, normalIndexOffset); } -UnsignedInt ObjImporter::doMeshCount() const { return _file->meshes.size(); } +UnsignedInt ObjImporter::doMeshCount() const { + /* There's always one more item for an upper bound */ + return _file->meshes.size() - 1; +} Int ObjImporter::doMeshForName(const Containers::StringView name) { const auto it = _file->meshesForName.find(name); return it == _file->meshesForName.end() ? -1 : it->second; } -Containers::String ObjImporter::doMeshName(UnsignedInt id) { +Containers::String ObjImporter::doMeshName(const UnsignedInt id) { return _file->meshes[id].name; } @@ -246,10 +278,9 @@ template bool checkAndDuplicateInto(const Containers::StridedArrayView1 } -Containers::Optional ObjImporter::doMesh(UnsignedInt id, UnsignedInt) { +Containers::Optional ObjImporter::doMesh(const UnsignedInt id, UnsignedInt) { /* Seek the file, set mesh parsing parameters */ const Mesh& mesh = _file->meshes[id]; - _file->in->seekg(mesh.begin); Containers::Optional primitive; Containers::Array positions; @@ -260,68 +291,151 @@ Containers::Optional ObjImporter::doMesh(UnsignedInt id, UnsignedInt) Containers::Array indices; std::size_t textureCoordinateIndexCount = 0, normalIndexCount = 0; - try { while(_file->in->good() && _file->in->tellg() < mesh.end) { - /* Ignore comments */ - if(_file->in->peek() == '#') { - ignoreLine(*_file->in); + Containers::StringView in{mesh.begin, std::size_t(_file->meshes[id + 1].begin - mesh.begin)}; + while(in) { + /* Get a line from the input */ + const Containers::StringView lineEnd = in.findOr('\n', in.end()); + const Containers::StringView line = in.prefix(lineEnd.begin()).trimmed(Whitespace); + in = in.suffix(lineEnd.end()); + + /* Comment or empty line, skip */ + if(!line || line.hasPrefix('#')) continue; + + /* Parse the keyword */ + const Containers::StringView keywordEnd = line.findAnyOr(Whitespace, line.end()); + const Containers::StringView keyword = line.prefix(keywordEnd.begin()); + + /* Skip keywords that are not interesting to us or that were parsed + earlier. In particular, the `o` can be here because the mesh range + is everything until the next mesh data start, so including the next mesh name. */ + if(keyword == "o"_s || + keyword == "g"_s || + keyword == "s"_s || + keyword == "mtllib"_s || + keyword == "usemtl"_s) continue; - } - /* Get the line */ - std::string line; - std::getline(*_file->in, line); - line = Utility::String::trim(line); - - /* Ignore empty lines */ - if(line.empty()) continue; - - /* Split the line into keyword and contents */ - const std::size_t keywordEnd = line.find(' '); - const std::string keyword = line.substr(0, keywordEnd); - const std::string contents = keywordEnd != std::string::npos ? - Utility::String::ltrim(line.substr(keywordEnd+1)) : ""; - - /* Vertex position */ - if(keyword == "v") { - Float extra{1.0f}; - const Vector3 data = extractFloatData<3>(contents, &extra); - if(!Math::TypeTraits::equals(extra, 1.0f)) { - Error() << "Trade::ObjImporter::mesh(): homogeneous coordinates are not supported"; - return Containers::NullOpt; + /* Keyword contents */ + Containers::StringView contents = line.suffix(keywordEnd.end()).trimmedPrefix(Whitespace); + + /* Vertex data */ + if(keyword == "v"_s || keyword == "vt"_s || keyword == "vn"_s) { + /* Decide on how many components we expect at most. There's + optional behavior for four-component positions and + three-component texture coordinates, so it can't be an exact + count. */ + std::size_t maxComponentCount; + if(keyword == "v"_s) maxComponentCount = 4; + else if(keyword == "vt"_s) maxComponentCount = 3; + else if(keyword == "vn"_s) maxComponentCount = 3; + else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ + + /* Parse them all. If there's less than expected, `i` would be too + small; if there's more then `contents` would stay non-empty. */ + Float data[4]; + std::size_t i = 0; + for(; i != maxComponentCount && contents; ++i) { + const Containers::StringView foundSpace = contents.findAnyOr(Whitespace, contents.end()); + + if(!parseFloat("Trade::ObjImporter::mesh():", contents.prefix(foundSpace.begin()), data[i])) + return {}; + + contents = contents.suffix(foundSpace.end()).trimmedPrefix(Whitespace); } - arrayAppend(positions, data); + /* Position */ + if(keyword == "v"_s) { + if(i < 3 || contents) { + Error{} << "Trade::ObjImporter::mesh(): expected 3 or 4 position coordinates, got" << line.suffix(keywordEnd.end()); + return {}; + } + if(i == 4 && !Math::equal(data[3], 1.0f)) { + Error{} << "Trade::ObjImporter::mesh(): homogeneous coordinates are not supported"; + return {}; + } - /* Texture coordinate */ - } else if(keyword == "vt") { - Float extra{0.0f}; - const Vector2 data = extractFloatData<2>(contents, &extra); - if(!Math::TypeTraits::equals(extra, 0.0f)) { - Error() << "Trade::ObjImporter::mesh(): 3D texture coordinates are not supported"; - return Containers::NullOpt; - } + arrayAppend(positions, Vector3::from(data)); - arrayAppend(textureCoordinates, data); + /* Texture coordinate */ + } else if(keyword == "vt"_s) { + if(i < 2 || contents) { + Error{} << "Trade::ObjImporter::mesh(): expected 2 or 3 texture coordinates, got" << line.suffix(keywordEnd.end()); + return {}; + } + if(i == 3 && !Math::equal(data[2], 0.0f)) { + Error{} << "Trade::ObjImporter::mesh(): 3D texture coordinates are not supported"; + return {}; + } - /* Normal */ - } else if(keyword == "vn") { - arrayAppend(normals, Vector3{extractFloatData<3>(contents)}); + arrayAppend(textureCoordinates, Vector2::from(data)); + + /* Normal */ + } else if(keyword == "vn"_s) { + if(i < 3 || contents) { + Error{} << "Trade::ObjImporter::mesh(): expected 3 normal coordinates, got" << line.suffix(keywordEnd.end()); + return {}; + } + + arrayAppend(normals, Vector3::from(data)); + + } else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ /* Indices */ - } else if(keyword == "p" || keyword == "l" || keyword == "f") { - const std::vector indexTuples = Utility::String::splitWithoutEmptyParts(contents, ' '); + } else if(keyword == "p"_s || keyword == "l"_s || keyword == "f"_s) { + /* Decide on how many tuples we expect */ + std::size_t indexTupleCount; + if(keyword == "p"_s) indexTupleCount = 1; + else if(keyword == "l"_s) indexTupleCount = 2; + else if(keyword == "f"_s) indexTupleCount = 3; + else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ + + /* Parse them all. If there's less than expected, `i` would be too + small; if there's more then `contents` would stay non-empty. */ + Vector3ui data[3]; + std::size_t i = 0; + for(; i != indexTupleCount && contents; ++i) { + const Containers::StringView foundSpace = contents.findAnyOr(Whitespace, contents.end()); + Containers::StringView indexTuple = contents.prefix(foundSpace.begin()); + + /* 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])) + return {}; + data[i][0] -= mesh.positionIndexOffset; + + /* If there was a slash, next is a texture coordinate or + empty */ + if(foundSlash1) { + 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][2])) + return {}; + data[i][2] -= 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][1])) + return {}; + data[i][1] -= mesh.normalIndexOffset; + ++normalIndexCount; + } + } + + contents = contents.suffix(foundSpace.end()).trimmedPrefix(Whitespace); + } /* Points */ if(keyword == "p") { - /* Check that we don't mix the primitives in one mesh */ if(primitive && primitive != MeshPrimitive::Points) { Error() << "Trade::ObjImporter::mesh(): mixed primitive" << *primitive << "and" << MeshPrimitive::Points; return Containers::NullOpt; } - - /* Check vertex count per primitive */ - if(indexTuples.size() != 1) { - Error() << "Trade::ObjImporter::mesh(): wrong index count for point"; + if(i < 1 || contents) { + Error() << "Trade::ObjImporter::mesh(): expected exactly 1 position index tuple for a point, got" << line.suffix(keywordEnd.end()); return Containers::NullOpt; } @@ -329,15 +443,12 @@ Containers::Optional ObjImporter::doMesh(UnsignedInt id, UnsignedInt) /* Lines */ } else if(keyword == "l") { - /* Check that we don't mix the primitives in one mesh */ if(primitive && primitive != MeshPrimitive::Lines) { Error() << "Trade::ObjImporter::mesh(): mixed primitive" << *primitive << "and" << MeshPrimitive::Lines; return Containers::NullOpt; } - - /* Check vertex count per primitive */ - if(indexTuples.size() != 2) { - Error() << "Trade::ObjImporter::mesh(): wrong index count for line"; + if(i < 2 || contents) { + Error() << "Trade::ObjImporter::mesh(): expected exactly 2 position index tuples for a line, got" << line.suffix(keywordEnd.end()); return Containers::NullOpt; } @@ -345,18 +456,12 @@ Containers::Optional ObjImporter::doMesh(UnsignedInt id, UnsignedInt) /* Faces */ } else if(keyword == "f") { - /* Check that we don't mix the primitives in one mesh */ if(primitive && primitive != MeshPrimitive::Triangles) { Error() << "Trade::ObjImporter::mesh(): mixed primitive" << *primitive << "and" << MeshPrimitive::Triangles; return Containers::NullOpt; } - - /* Check vertex count per primitive */ - if(indexTuples.size() < 3) { - Error() << "Trade::ObjImporter::mesh(): wrong index count for triangle"; - return Containers::NullOpt; - } else if(indexTuples.size() != 3) { - Error() << "Trade::ObjImporter::mesh(): polygons are not supported"; + if(i < 3 || contents) { + Error() << "Trade::ObjImporter::mesh(): expected exactly 3 position index tuples for a triangle, got" << line.suffix(keywordEnd.end()); return Containers::NullOpt; } @@ -364,50 +469,14 @@ Containers::Optional ObjImporter::doMesh(UnsignedInt id, UnsignedInt) } else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ - for(const std::string& indexTuple: indexTuples) { - std::vector indexStrings = Utility::String::split(indexTuple, '/'); - if(indexStrings.size() > 3) { - Error() << "Trade::ObjImporter::mesh(): invalid index data"; - return Containers::NullOpt; - } - - Vector3ui index; - - /* Position indices */ - index[0] = std::stoul(indexStrings[0]) - mesh.positionIndexOffset; - - /* Texture coordinates */ - if(indexStrings.size() == 2 || (indexStrings.size() == 3 && !indexStrings[1].empty())) { - index[2] = std::stoul(indexStrings[1]) - mesh.textureCoordinateIndexOffset; - ++textureCoordinateIndexCount; - } - - /* Normal indices */ - if(indexStrings.size() == 3) { - index[1] = std::stoul(indexStrings[2]) - mesh.normalIndexOffset; - ++normalIndexCount; - } - - arrayAppend(indices, index); - } + /** @todo fix arrayAppend() to not need the cast here */ + arrayAppend(indices, Containers::ArrayView{data}.prefix(i)); - /* Ignore unsupported keywords, error out on unknown keywords */ - } else if(![&keyword](){ - /* Using lambda to emulate for-else construct like in Python */ - for(const std::string expected: {"mtllib", "usemtl", "g", "s"}) - if(keyword == expected) return true; - return false; - }()) { - Error() << "Trade::ObjImporter::mesh(): unknown keyword" << keyword; - return Containers::NullOpt; + /* Unknown keyword */ + } else { + Error{} << "Trade::ObjImporter::mesh(): unknown keyword" << keyword; + return {}; } - - }} catch(const std::exception&) { - Error() << "Trade::ObjImporter::mesh(): error while converting numeric data"; - return Containers::NullOpt; - } catch(...) { - /* Error message already printed */ - return Containers::NullOpt; } /* There should be at least indexed position data */ diff --git a/src/MagnumPlugins/ObjImporter/ObjImporter.h b/src/MagnumPlugins/ObjImporter/ObjImporter.h index 8d2e49cb8..56c9a874c 100644 --- a/src/MagnumPlugins/ObjImporter/ObjImporter.h +++ b/src/MagnumPlugins/ObjImporter/ObjImporter.h @@ -124,7 +124,6 @@ class MAGNUM_OBJIMPORTER_EXPORT ObjImporter: public AbstractImporter { MAGNUM_OBJIMPORTER_LOCAL bool doIsOpened() const override; MAGNUM_OBJIMPORTER_LOCAL void doOpenData(Containers::Array&& data, DataFlags dataFlags) override; - MAGNUM_OBJIMPORTER_LOCAL void doOpenFile(Containers::StringView filename) override; MAGNUM_OBJIMPORTER_LOCAL void doClose() override; MAGNUM_OBJIMPORTER_LOCAL UnsignedInt doMeshCount() const override; diff --git a/src/MagnumPlugins/ObjImporter/Test/CMakeLists.txt b/src/MagnumPlugins/ObjImporter/Test/CMakeLists.txt index ce82d52b3..8e1b6c4f3 100644 --- a/src/MagnumPlugins/ObjImporter/Test/CMakeLists.txt +++ b/src/MagnumPlugins/ObjImporter/Test/CMakeLists.txt @@ -50,7 +50,7 @@ corrade_add_test(ObjImporterTest ObjImporterTest.cpp empty.obj invalid-incomplete-data.obj invalid-inconsistent-index-tuple.obj - invalid-keyword.obj + invalid-keywords.obj invalid-mixed-primitives.obj invalid-number-count.obj invalid-numbers.obj diff --git a/src/MagnumPlugins/ObjImporter/Test/ObjImporterTest.cpp b/src/MagnumPlugins/ObjImporter/Test/ObjImporterTest.cpp index 0b9262c53..084c8e90a 100644 --- a/src/MagnumPlugins/ObjImporter/Test/ObjImporterTest.cpp +++ b/src/MagnumPlugins/ObjImporter/Test/ObjImporterTest.cpp @@ -68,7 +68,7 @@ struct ObjImporterTest: TestSuite::Tester { maintain. So it's instead grouped into files by a common error scenario with each case testing one file and just the invalid() case testing separate files. */ - void invalid(); + void invalidKeywords(); void invalidMixedPrimitives(); void invalidNumbers(); void invalidNumberCount(); @@ -76,6 +76,8 @@ struct ObjImporterTest: TestSuite::Tester { void invalidIncompleteData(); void invalidOptionalCoordinate(); + void whitespace(); + void openTwice(); void importTwice(); @@ -93,11 +95,11 @@ const struct { const struct { const char* name; - const char* filename; const char* message; -} InvalidData[]{ - {"unknown keyword", "invalid-keyword.obj", - "unknown keyword bleh"} +} InvalidKeywordsData[]{ + {"unknown keyword", "unknown keyword bleh"}, + {"no space between vertex keyword and number", "unknown keyword vt3"}, + {"no space between index keyword and number", "unknown keyword p6"} }; const struct { @@ -116,9 +118,12 @@ const struct { const char* name; const char* message; } InvalidNumbersData[]{ - {"invalid float literal", "error while converting numeric data"}, - {"invalid integer literal", "error while converting numeric data"}, - {"position index out of range", "index 1 out of range for 1 vertices"}, + {"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"}, + {"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"}, {"texture index out of range", "index 4 out of range for 3 vertices"}, {"normal index out of range", "index 3 out of range for 2 vertices"}, {"zero index", "index 0 out of range for 1 vertices"} @@ -128,14 +133,25 @@ const struct { const char* name; const char* message; } InvalidNumberCountData[]{ - {"two-component position", "invalid float array size"}, - {"five-component position with optional fourth component", "invalid float array size"}, - {"four-component normal", "invalid float array size"}, - {"four-component index tuple", "invalid index data"}, - {"point with two indices", "wrong index count for point"}, - {"line with one index", "wrong index count for line"}, - {"triangle with two indices", "wrong index count for triangle"}, - {"quad", "polygons are not supported"} + {"no position component at all", "expected 3 or 4 position coordinates, got "}, + {"two-component position", "expected 3 or 4 position coordinates, got 0.5 1.0"}, + {"five-component position with optional fourth component", "expected 3 or 4 position coordinates, got 0.5 1 2 1.0 3.5"}, + {"no texture coordinate component at all", "expected 2 or 3 texture coordinates, got "}, + {"one-component texture coordinate", "expected 2 or 3 texture coordinates, got 0.5"}, + {"four-component texture coordinate with optional third component", "expected 2 or 3 texture coordinates, got 0.5 1.0 0.0 7.4"}, + {"no normal component at all", "expected 3 normal coordinates, got "}, + {"two-component normal", "expected 3 normal coordinates, got 0.5 0.0"}, + {"four-component normal", "expected 3 normal coordinates, got 0.5 1.0 2.3 7.4"}, + {"no index at all", "expected exactly 1 position index tuple for a point, got "}, + /** @todo better error message (the literal is empty) */ + {"no index before first slash", "invalid integer literal "}, + {"no index after first slash", "invalid integer literal "}, + {"no index after second slash", "invalid integer literal "}, + {"four-component index tuple", "invalid integer literal 1/1"}, + {"point with two indices", "expected exactly 1 position index tuple for a point, got 9 9"}, + {"line with one index", "expected exactly 2 position index tuples for a line, got 10"}, + {"triangle with two indices", "expected exactly 3 position index tuples for a triangle, got 11 11"}, + {"quad", "expected exactly 3 position index tuples for a triangle, got 12 12 12 12"} }; const struct { @@ -188,8 +204,8 @@ ObjImporterTest::ObjImporterTest() { addTests({&ObjImporterTest::moreMeshes}); - addInstancedTests({&ObjImporterTest::invalid}, - Containers::arraySize(InvalidData)); + addInstancedTests({&ObjImporterTest::invalidKeywords}, + Containers::arraySize(InvalidKeywordsData)); addInstancedTests({&ObjImporterTest::invalidMixedPrimitives}, Containers::arraySize(InvalidMixedPrimitivesData)); @@ -209,7 +225,9 @@ ObjImporterTest::ObjImporterTest() { addInstancedTests({&ObjImporterTest::invalidOptionalCoordinate}, Containers::arraySize(InvalidOptionalCoordinateData)); - addTests({&ObjImporterTest::openTwice, + addTests({&ObjImporterTest::whitespace, + + &ObjImporterTest::openTwice, &ObjImporterTest::importTwice}); #ifdef OBJIMPORTER_PLUGIN_FILENAME @@ -554,18 +572,19 @@ void ObjImporterTest::moreMeshes() { TestSuite::Compare::Container); } -void ObjImporterTest::invalid() { - auto&& data = InvalidData[testCaseInstanceId()]; +void ObjImporterTest::invalidKeywords() { + auto&& data = InvalidKeywordsData[testCaseInstanceId()]; setTestCaseDescription(data.name); Containers::Pointer importer = _manager.instantiate("ObjImporter"); - CORRADE_VERIFY(importer->openFile(Utility::Path::join(OBJIMPORTER_TEST_DIR, data.filename))); + CORRADE_VERIFY(importer->openFile(Utility::Path::join(OBJIMPORTER_TEST_DIR, "invalid-keywords.obj"))); - CORRADE_COMPARE(importer->meshCount(), 1); + /* Ensure we didn't forget to test any case */ + CORRADE_COMPARE(importer->meshCount(), Containers::arraySize(InvalidKeywordsData)); Containers::String out; Error redirectError{&out}; - CORRADE_VERIFY(!importer->mesh(0)); + CORRADE_VERIFY(!importer->mesh(data.name)); CORRADE_COMPARE(out, Utility::format("Trade::ObjImporter::mesh(): {}\n", data.message)); } @@ -665,6 +684,54 @@ void ObjImporterTest::invalidOptionalCoordinate() { CORRADE_COMPARE(out, Utility::format("Trade::ObjImporter::mesh(): {}\n", data.message)); } +void ObjImporterTest::whitespace() { + /* Using an embedded string to have the whitespace visualized clearer */ + + Containers::StringView data = + " \t\r # This file has various \t\n" + " #whitespace that should be treated properly \n" + " \r\t o\t \tObject \t name \r\n" + " \t v\r \t 3 5 \t7 \t1\n" + " \rvt 8\t 9\t\t\r0\n" + "\n \r \n" + " vn \t 2 \t 4 \r6\n" + "\n \t \r\n" + /* There's no space expected inside the index tuple */ + " f\t 1/1/1 \r1/1/1 \t1/1/1 \t\r\n" + "\n\n" + "\n"; + + Containers::Pointer importer = _manager.instantiate("ObjImporter"); + CORRADE_VERIFY(importer->openData(data)); + + CORRADE_COMPARE(importer->meshCount(), 1); + /* The name is trimmed from both ends, but internal whitespace is left + intact */ + CORRADE_COMPARE(importer->meshName(0), "Object \t name"); + + const Containers::Optional mesh = importer->mesh(0); + CORRADE_VERIFY(mesh); + CORRADE_COMPARE(mesh->primitive(), MeshPrimitive::Triangles); + CORRADE_COMPARE(mesh->attributeCount(), 3); + CORRADE_COMPARE_AS(mesh->attribute(MeshAttribute::Position), + Containers::arrayView({ + {3.0f, 5.0f, 7.0f} + }), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(mesh->attribute(MeshAttribute::TextureCoordinates), + Containers::arrayView({ + {8.0f, 9.0f} + }), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(mesh->attribute(MeshAttribute::Normal), + Containers::arrayView({ + {2.0f, 4.0f, 6.0f} + }), TestSuite::Compare::Container); + CORRADE_VERIFY(mesh->isIndexed()); + CORRADE_COMPARE(mesh->indexType(), MeshIndexType::UnsignedInt); + CORRADE_COMPARE_AS(mesh->indices(), + Containers::arrayView({0, 0, 0}), + TestSuite::Compare::Container); +} + void ObjImporterTest::openTwice() { Containers::Pointer importer = _manager.instantiate("ObjImporter"); diff --git a/src/MagnumPlugins/ObjImporter/Test/invalid-keyword.obj b/src/MagnumPlugins/ObjImporter/Test/invalid-keyword.obj deleted file mode 100644 index 3149e4c0a..000000000 --- a/src/MagnumPlugins/ObjImporter/Test/invalid-keyword.obj +++ /dev/null @@ -1 +0,0 @@ -bleh diff --git a/src/MagnumPlugins/ObjImporter/Test/invalid-keywords.obj b/src/MagnumPlugins/ObjImporter/Test/invalid-keywords.obj new file mode 100644 index 000000000..d7a8f6ef7 --- /dev/null +++ b/src/MagnumPlugins/ObjImporter/Test/invalid-keywords.obj @@ -0,0 +1,6 @@ +o unknown keyword +bleh +o no space between vertex keyword and number +vt3 1 +o no space between index keyword and number +p6 diff --git a/src/MagnumPlugins/ObjImporter/Test/invalid-number-count.obj b/src/MagnumPlugins/ObjImporter/Test/invalid-number-count.obj index 088b54fed..a5b682b06 100644 --- a/src/MagnumPlugins/ObjImporter/Test/invalid-number-count.obj +++ b/src/MagnumPlugins/ObjImporter/Test/invalid-number-count.obj @@ -1,29 +1,62 @@ +o no position component at all +v + o two-component position v 0.5 1.0 o five-component position with optional fourth component -v 0.5 1 2 0.0 3.5 +v 0.5 1 2 1.0 3.5 + +o no texture coordinate component at all +vt + +o one-component texture coordinate +v 0.5 1 2 +vt 0.5 + +o four-component texture coordinate with optional third component +v 0.5 1 2 +vt 0.5 1.0 0.0 7.4 + +o no normal component at all +vn + +o two-component normal +v 0.5 1 2 +vn 0.5 0.0 o four-component normal v 0.5 1 2 vn 0.5 1.0 2.3 7.4 +o no index at all +p + +o no index before first slash +p / + +o no index after first slash +p 8/ + +o no index after second slash +p 8// + o four-component index tuple v 1 2 3 -p 4/1/1/1 +p 8/1/1/1 o point with two indices v 1 2 3 -p 5 5 +p 9 9 o line with one index v 1 2 3 -l 6 +l 10 o triangle with two indices v 1 2 3 -f 7 7 +f 11 11 o quad v 1 2 3 -f 8 8 8 8 +f 12 12 12 12 diff --git a/src/MagnumPlugins/ObjImporter/Test/invalid-numbers.obj b/src/MagnumPlugins/ObjImporter/Test/invalid-numbers.obj index 88eb62e51..7bdbe1914 100644 --- a/src/MagnumPlugins/ObjImporter/Test/invalid-numbers.obj +++ b/src/MagnumPlugins/ObjImporter/Test/invalid-numbers.obj @@ -1,6 +1,15 @@ +o too long float literal +v 1234.567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567 0 0 + +o too long integer literal +p 12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678 + +o too large integer literal +p 4294967296 + o invalid float literal v 1 bleh 2 -p 1 +p 2 o invalid integer literal v 1 0 2 @@ -8,8 +17,8 @@ p bleh o position index out of range v 1 0 2 -# Should be 3 -p 1 +# Should be 4 +p 3 o texture index out of range v 1 0 2 @@ -17,14 +26,14 @@ vt 0 1 vt 0 1 vt 0 1 # Should be 4/3 -p 4/4 +p 5/4 o normal index out of range v 1 0 2 vn 0 0 1 vn 0 0 1 # Should be 5/2 -p 5//3 +p 6//3 o zero index v 1 0 2