zeux / meshoptimizer

Mesh optimization library that makes meshes smaller and faster to render
MIT License
5.49k stars 473 forks source link

gltfpack: Output GLTF is invalid if input scene has missing image files #675

Closed Cyphall closed 4 months ago

Cyphall commented 5 months ago

When optimizing a GLTF file which has image entries referencing missing files, the exported image entries have neither their uri property nor their bufferView property defined. According to this JSON schema in the specs, such entries are invalid.

When optimizing, gltfpack logs warnings about skipping these missing images entries to the console, which means it already is aware of this possibility. As such, I would expect it to produce a valid GLTF file by simply copying these entries to the output file without any processing.

(related to KhronosGroup/glTF#2377)

zeux commented 5 months ago

I'm not sure if copying the URI unconditionally is always a good idea: if the output is a GLB file, I'd expect no external references and there might be issues if the target application doesn't expect that any URIs are preserved (eg maybe it's a URI that can be used maliciously somehow). Maybe in that case an empty URI would suffice...

Cyphall commented 5 months ago

Maybe in that case an empty URI would suffice...

This might cause some issues when the final program tries to recompose the absolute path of the file.

Example: An optimized GLTF File at "C:/example/file.gltf" containing an image with an empty uri. The image absolute path is recomposed to "{GLTF_PARENT_DIR}/{IMAGE_URI}" (aka "C:/example/" here). This would end up with the final program trying to read data from a directory path actually existing on disk:

std::filesystem::path absolutePath = gltfParentPath / imageUri;
if (std::filesystem::exists(absolutePath))
{
    // Read and process image
    // No need to check for error since the path exists, what could go wrong?
}

I think the best way is to remove the image entry altogether (along with texture entries referencing it) and unlink those textures from materials, as if the material's texture slot was empty from the start.

arpu commented 5 months ago

@zeux i was able to get a model with this problem ( looks like it from the hubs exporter) i send you the glb per mail

Cyphall commented 5 months ago

Yeah I didn't include a sample (probably should have, sorry). To confirm the issue before posting, I used this one from the glTF-Sample-Assets repo and simply removed the png file.

zeux commented 5 months ago

One other potential cause here is that when using -tc, if the image path is valid but it refers to an invalid image (eg format encoding issues), Basis texture compressor will fail to process the image - as it should - and the image object will be left blank, which will also make the asset's JSON invalid.