Exceptions are disabled in Windows prebuilt library?

[patrikhuber]

Hi,

I'm using the prebuilt Windows zips from the Releases section (https://github.com/google/filament/releases).

It looks like exceptions are disabled in that build. I'm building a material and trying to catch errors, so that my program doesn't terminate if there's a problem (e.g. material version mismatch):

   const std::vector<uint8_t> material_bytes = read_bytes(material_path);
   filament::Material* material = nullptr;
   try
   {
       material = filament::Material::Builder()
                      .package(material_bytes.data(), material_bytes.size())
                      .build(*impl->engine);
   } catch (const utils::PostconditionPanic& e)
   {
       std::string error_msg =
           std::format("Failed to build material from file {}: {}", material_path.string(), e.what());
       spdlog::error(error_msg);
       throw std::runtime_error(error_msg);
   } catch (const utils::PreconditionPanic& e)
   {
       std::string error_msg = std::format("Precondition failed while building material from file {}: {}",
                                           material_path.string(), e.what());
       spdlog::error(error_msg);
       throw std::runtime_error(error_msg);
   }

However this doesn't reach the catch sections, and it looks like std::abort() is called in TPanic in Panic.cpp. If exceptions were enabled, I believe it should not reach std::abort(), but rather throw: https://github.com/google/filament/blob/a89711b006b0230d0d4514c7b621f402ff969f7f/libs/utils/src/Panic.cpp#L227

If I'm indeed right, I'd say this is a bad default, because users of Filament likely want to catch Filament exceptions and not have their programs abort. Of course I could build Filament myself with exceptions enabled, but I'd argue that's beside the point - the pre-built zips are more convenient and should cater for the most common use case, which I'd say is throwing, not calling std::abort(). If someone does want exceptions disabled, I'd consider it a special use case, and then those users can build Filament without exceptions if they need that.

Thoughts?

[romainguy]

Exceptions are disabled for historical reasons: Filament was designed primarily for Android and keeping the binary small was one of the major goals for the libraries. Disabling exceptions was part of keeping the binary size in check. In this case I'd argue that instead of panic'ing/throwing, the material builder could return a null pointer instead. But that's something for @pixelflinger to decide :)

[patrikhuber]

Thank you for the reply Romain!

The material builder returning a null pointer would certainly solve my immediate "issue", but there are quite a few other functions where Filament can panic. And thankfully those are even well-documented with Doxygen @throw annotations, so it's quite easy for users to catch them - if exceptions are enabled. So I'd strongly argue the better solution would be to switch the Windows pre-built zip to have exceptions enabled. :-)

[pixelflinger]

@patrikhuber can you not make your own build? I don't think it's too complicated.

[patrikhuber]

@patrikhuber can you not make your own build? I don't think it's too complicated.

It's not too complicated, but easier to integrate into a build (that also has other dependencies) if you can just download an updated version from the GitHub Releases and unzip it. And Filament releases are very frequent. Of course I'll probably do that and create my own build if you guys decide that having exceptions disabled in the default prebuilt Windows build is the right thing to do for this library.

In general integrating Filament into an existing CMake project is not great, we had to write our own FindFilament.cmake script that checks the platform (Linux/Windows), checks the MSVC runtime (MT/MTD/MD/MDD) on Windows, works out Filament's internal CMake target dependencies, and then creates a namespaced filament cmake target that points to the correct .lib or .a files, based on the platform/runtime.

It would be really great if Filament followed proper CMake best-practice and generated/offered a filament-config.cmake and filament-exports.cmake with proper export targets. This would make it much easier to integrate Filament into a build as an external library (and not as a submodule/subdirectory), whether self-built or a pre-built download. And this is really the standard practice for any C++ library nowadays. I'm sure I saw someone else comment on this or opened an issue for this in this repo before, but I can't find it now with GitHub's search.

Having those CMake export files would also be the first step into integrating Filament into a package manager like vcpkg, which would be really good to have too.