f3d icon indicating copy to clipboard operation
f3d copied to clipboard

crash on undefined `pbrMetallicRoughness`

Open snoyer opened this issue 1 year ago • 2 comments

VTK may[^1] crash when loading a glTF file containing a material that does not define the optional pbrMetallicRoughness property, see attached samples: material-pbrMetallicRoughness.zip.

VTK can however handle a material's pbrMetallicRoughness being empty; so accessing the property with an empty default value instead of the current unchecked access should be enough of a fix

diff --git a/IO/Geometry/vtkGLTFDocumentLoaderInternals.cxx b/IO/Geometry/vtkGLTFDocumentLoaderInternals.cxx
index 713e9d966c..dab4af87b9 100644
--- a/IO/Geometry/vtkGLTFDocumentLoaderInternals.cxx
+++ b/IO/Geometry/vtkGLTFDocumentLoaderInternals.cxx
@@ -657,7 +657,7 @@ bool vtkGLTFDocumentLoaderInternals::LoadMaterial(
   double metallicFactor = 1;
   double roughnessFactor = 1;
 
-  const auto& pbrRoot = root["pbrMetallicRoughness"];
+  const auto& pbrRoot = root.value("pbrMetallicRoughness", R"({})"_json);
   if (!pbrRoot.empty())
   {
     if (vtkGLTFUtils::GetDoubleValue(pbrRoot, "metallicFactor", metallicFactor))

[^1]: it might also work in some cases (eg. different builds of F3D)? but, as per nlohmann/json's documentation, "It is undefined behavior to access a const object with a non-existing key"

snoyer avatar May 24 '24 04:05 snoyer

Looks like an easy VTK fix.

Did you check if there are other fix like this to do in the file ?

Do you want to fix it yourself in VTK ? I can guide you.

mwestphal avatar May 24 '24 06:05 mwestphal

Did you check if there are other fix like this to do in the file ?

I hadn't done it before... having a quick look now it seems there's a few more.

Do you want to fix it yourself in VTK ? I can guide you.

I could have a try yes, I mean I'll take longer than you would but the bug has been there for years without being an issue so it can wait a bit more :)

snoyer avatar May 24 '24 06:05 snoyer

fixed by https://github.com/f3d-app/f3d/pull/1512 (which updates VTK)

mwestphal avatar Jul 09 '24 07:07 mwestphal