Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Images are not saved to disk properly #473

Open
ptc-brainer opened this issue Mar 14, 2024 · 3 comments
Open

Images are not saved to disk properly #473

ptc-brainer opened this issue Mar 14, 2024 · 3 comments

Comments

@ptc-brainer
Copy link

ptc-brainer commented Mar 14, 2024

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.

@syoyo
Copy link
Owner

syoyo commented Mar 14, 2024

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.

@ptc-brainer
Copy link
Author

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

@syoyo
Copy link
Owner

syoyo commented Mar 14, 2024

@ptc-brainer 👌

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants