assimp / assimp

The official Open-Asset-Importer-Library Repository. Loads 40+ 3D-file-formats into one unified and clean data structure.
https://www.assimp.org
Other
10.72k stars 2.89k forks source link

Importer throws DeadlyImportException with .glb files (gltf2 version) #2778

Open pweissenbacher opened 4 years ago

pweissenbacher commented 4 years ago

I'm trying to import binary gltf2 models (.glb) with assimp importer.ReadFile(). I'm using the assimp version of commit 2d2889f73fa1b2ca09ba9f43c9785402d3a7fdd0 (Release v5.0.0).

The code in Importer.cpp at line 596:

if( pimpl->mImporter[a]->CanRead( pFile, pimpl->mIOHandler, false)) throws an "DeadlyImportError at memory location 0x00000033C4FAB3F8." at index a == 42.

But still the model is able to be loaded with pimpl->mImporter[43].

My question here is - does this exception get thrown because of some model invalidity, or is there something else wrong? I want to be sure, that there is no error within my .glb files.

kimkulling commented 4 years ago

For me this sounds like a bug. Have you tried to import a public model which can be used to reproduce this issue?

pweissenbacher commented 4 years ago

e.g. this one: https://github.com/KhronosGroup/glTF-Sample-Models/blob/master/2.0/Buggy/glTF-Binary/Buggy.glb

It gives me the same error.

My import command is as followed: Assimp::Importer importer; const aiScene* scene = importer.ReadFile(path, aiProcess_GenNormals | aiProcess_Triangulate | aiProcess_FixInfacingNormals | aiProcess_ImproveCacheLocality);

ademets commented 4 years ago

Assimp has 2 separate gltf importers - one for 2.0 files, and one for 1.x. As the extension is the same, and due to the ImporterRegistry order, it tries open it with the 1.0 importer first. Inside gltfimporter::CanRead (in gltfimporter.cpp) the file is opened and parsed, once it reads the "2.0" version tag, it throws the DeadlyImportError exception which is caught and returns the function with false. I think asset.Load(pFile, extension == "glb"); // this one throws the exception is the culprit here.

The clean way would be rewriting the file check in gltfimporter::CanRead to not depend on exceptions for version checks. There are good arguments to be made to not use exceptions inside library functions to handle program flow. The quick fix I used for now (which merges nicely) is to switch out the registration order in "ImporterRegistry.cpp", as practically all files are gonna be 2.0 anyway. Disclaimer: I have not tested how that effects loading 1.0 files, but since there is no backwards compatibility guaranteed between 2.0 and 1.0, glTF2Importer should check strictly for version>=2.0:

#if ( !defined ASSIMP_BUILD_NO_GLTF_IMPORTER )
    out.push_back( new glTF2Importer() );    // try loading via 2.0 importer first
    out.push_back( new glTFImporter() );
#endif

Parsing the file only to get to the versioning is not super load-time friendly, as every time it's a 2.0 the file has been parsed before for no reason, which is going to be a majority of the time nowadays.

kimkulling commented 4 years ago

I do agree. This kind of canRead-implementation is not sufficient for this use case.

vilbeyli commented 3 years ago

This usage of throw here had me spend time debugging only to find assimp simply tries to check if gltf1.x importer can open a gltf2.x file, generating an error message on my debugger while there's no actual error in the code intent.

@kimkulling would you be OK with me fixing this misleading exception throw with a pull request by returning bool for all the functions involved in between glTFImporter::CanRead() and the throw ?

koushikchalla commented 2 years ago

Hi guys, I had the same issue where glb 2.0 was throwing this error but glb 1.0 wasn't. My case is even worse since I'm working in wasm environment (using emscripten) and exception catching doesnt work here.

Changes i had to make to make it work : In file code/AssetLib/glTF/gltfAsset.inl

in function ::ReadBinaryHeader and AssetMetadata::Read I replaced all throw DeadlyImportError(message) to ASSIMP_LOG_INFO(message)

in function Asset::Load I added the following lines after the ReadBinaryHeader(*stream) call

std::string check_version = asset.version; if (check_version.empty() || check_version[0] != '1') { ASSIMP_LOG_INFO("GLTF: Special return case to get out of the function: "); return; }

After this I just rebuilt (emmake make) and then both glb 2.0 and glb 1.0 loads without any errors

luke10x commented 4 days ago

In Emscpripten I get Assimp code after 5.4.2 built with the following command:

export CXXFLAGS="-s DISABLE_EXCEPTION_CATCHING=0"

emcmake cmake \
    -DASSIMP_BUILD_TESTS=OFF \
    -DASSIMP_BUILD_ZLIB=OFF \
    -DASSIMP_BUILD_ASSIMP_TOOLS=OFF \
    -DASSIMP_INSTALL_PDB=OFF \
    -DBUILD_SHARED_LIBS=OFF \
    $SRC_DIR

emmake make -j4

emmake make install

The key here is DISABLE_EXCEPTION_CATCHING=0, which probably comes with a cost. In my case, it adds several MB to libassimp.a, but it can load *.glb files with this flag. (And $SRC_DIR is where I have the Assimp source code).

However, I would favor Assimp not using exceptions in its code base.

Hi guys, I had the same issue where glb 2.0 was throwing this error but glb 1.0 wasn't. My case is even worse since I'm working in wasm environment (using emscripten) and exception catching doesnt work here.

Changes i had to make to make it work : In file code/AssetLib/glTF/gltfAsset.inl

in function ::ReadBinaryHeader and AssetMetadata::Read I replaced all throw DeadlyImportError(message) to ASSIMP_LOG_INFO(message)

in function Asset::Load I added the following lines after the ReadBinaryHeader(*stream) call

std::string check_version = asset.version; if (check_version.empty() || check_version[0] != '1') { ASSIMP_LOG_INFO("GLTF: Special return case to get out of the function: "); return; }

After this I just rebuilt (emmake make) and then both glb 2.0 and glb 1.0 loads without any errors