Closed sanghoon closed 2 years ago
Hi! Thank you for your contribution!
The first changes looks correct. Can you please also update the documentation at the top of cgltf_write.h
?
* To write custom JSON into the `extras` field, aggregate all the custom JSON
* into a single buffer, then set `file_data` to this buffer. By supplying
* start_offset and end_offset values for various objects, you can select a
* range of characters within the aggregated buffer.
That should refer to json
as well now I think.
With the second change, I'm not so sure yet. How do you end up in the situation that uri == 0
and you know that the data should be base64 encoded? Looking at cgltf_load_buffers()
, this shouldn't happen in a round-trip with a given file? Do you intend for this to be the new way to signal that the data should be encoded?
Thanks for your comment, I've updated the comment section.
Talking about the second change, I encountered such a case when I opened an GLB with an embedded binary buffer.
A GLB embedded buffer is loaded as { uri = NULL, data = (some ptr), size = (buffer size) } (At a glance, it seemed that it's the only case with a data buffer w/o a uri)
https://github.com/jkuhlmann/cgltf/blob/280ab8907d839ca414672ed1873af26419190897/cgltf.h#L1426-L1435 (which was introduced at https://github.com/jkuhlmann/cgltf/commit/3312f8fee4356e9a1fc93042ff49ab13bd690f3e commit)
With the previous impl. (which just write back the uri values only), those embedded buffers will be lost.
Alright, I think I understand the issue better now. It is actually explained in the documentation in the beginning of cgltf_write.h
:
* `cgltf_result cgltf_write_file(const cgltf_options* options, const char*
* path, const cgltf_data* data)` writes JSON to the given file path. Buffer
* files and external images are not written out.
The important point is that this will still have problems with other referenced data like images. I would suggest the better solution is to offer a new function that actually writes a GLB file which could then contain the referenced binary buffer. What do you think?
... The important point is that this will still have problems with other referenced data like images. ...
That was a good point. I reverted the base64 encoder and implemented a function to write back in a GLB format as you suggested.
Some constants (such as GlbHeaderSize
) are required both in cgltf.h
and cgltf_write.h
. In the current implementation, I introduced duplicated definitions in the headers. There must be some other ways such as adding a new header file or using macros. However, I presumed that keeping the loader in a single file was more important than creating no duplicated lines, and I wanted to keep the change as concise as possible.
Currently, the GLB write-back feature will work only if option->type
is cgltf_file_type_glb
.
Awesome, and it even includes a test! :+1: It looks like the build failed on Windows, however. Could you please take a look at that? https://github.com/jkuhlmann/cgltf/runs/7618507577?check_suite_focus=true#step:3:60
@sanghoon Otherwise, I have to do it. 😛
Sorry for the late reply. Due to a personal reason (a newborn baby), I couldn't have some spare time to work on this. I'll take a look at the issue and resolve it soon.
@jkuhlmann I resolved compile warnings by adding explicit castings.
While debugging on Windows, I found another bug and fixed it.
The bug was related to fwrite()
.
On Windows, the function automatically replaces \n
with \r\n
(CR LF) if a file is opened in text mode.
This leads to a slightly bigger written size than expected.
This behavior has existed for some (maybe from the beginning of a write function?). I assume that it wasn't an issue since we don't really need to care about its exact size when we parse a JSON file. However, now it's an issue since a GLB stores byte lengths.
Therefore, I changed the write mode to wb
to resolve this issue.
Thank you! And congratulations to the baby! 🥳 That's great news!
I've encountered two issues while writing back a glb file with extra properties. The first one was malformed extras, the other was missing binary buffers.
At a glance, it seems that start_offset is for the
data->json
buffer not for thedata->file_data
https://github.com/jkuhlmann/cgltf/blob/master/cgltf.h#L1734-L1743 Therefore, I modifiedcgltf_write_extras()
so that it can read values fromdata->json
Not to lose embedded binary buffers, I implemented a simple base64 encoder. If a URI is empty, a buffer content will be written as a data URI.