syoyo/tinygltf

Images are not saved to disk properly

ptc-brainer opened this issue · 3 comments

Describe the issue

Writing a glTF that contains images referenced as uris without embedding images does not save the images to the disk next to the glTF model.

To Reproduce

OS: MacOS
Compiler: Apple clang version 15.0.0

The following unit test fails with with the latest version of tinygltf:

TEST_CASE("write-image-issue") {

  tinygltf::Model model;
  tinygltf::TinyGLTF ctx;
  std::string err;
  std::string warn;

  REQUIRE(ctx.LoadASCIIFromFile(&model, &err, &warn, "../models/Cube/Cube.gltf"));

  REQUIRE(model.images.size() == 2);

  REQUIRE(model.images[0].uri == "Cube_BaseColor.png");
  REQUIRE(model.images[1].uri == "Cube_MetallicRoughness.png");

  REQUIRE_FALSE(model.images[0].image.empty());
  REQUIRE_FALSE(model.images[1].image.empty());

  REQUIRE(ctx.WriteGltfSceneToFile(&model, "Cube.gltf", false, true));

  for (const auto image : model.images)
  {
    std::fstream file(image.uri);
    CHECK(file.good());
  }
}

Expected behaviour

I would expect that the writer saves the images to the disk at the relative location described in the image uri.

Additional context

I traced this issue down to the WriteImageData() function that is passed to UpdateImageObject(). This function expects the FsCallback passed as the user data. However, the default write_image_user_data with is initialized withnullptr, causing the file to never be written to disk. An experimental change the initialize write_image_user_data with a pointer to the default FsCallback (&fs) fixed this issue, but caused a different unit test failure.

Confirmed the issue.

Passing FsCallback to write_image_user_data is a confusing, so we are better to modify WriteImageDataFunction API to accept FsCallbacks callback pointer as an argument.

something like...

  typedef bool (*WriteImageDataFunction)(const std::string *basepath,
                                         const std::string *filename,
                                         const Image *image, bool embedImages,
                                         const FsCallback *fs_cb, 
                                         const URICallbacks *uri_cb,
                                         std::string *out_uri,
                                         void *user_pointer);

We may also need to add overwrite existing image file flag to the argument.

Thanks for the quick investigation! Your proposal looks good to me!
One point to consider as well is to omit unneeded copies of the source and dest paths are the same

@ptc-brainer 👌

Let me give some time to figure out what is best solution for WriteImageDataFunction