urlencode image urls #397
urlencode image urls #397
Conversation
pknowlesnv
force-pushed
the
encode_image_uri
branch
from
03b319c
to
9f6da4b
Compare
tests/tester.cc
Outdated
| REQUIRE(err.empty()); | ||
| REQUIRE(!warn.empty()); // relative image path should not have been found | ||
| REQUIRE(saved.images.size() == model.images.size()); | ||
| REQUIRE(saved.images[0].uri == "+2x2+image++has+multiple++++++spaces%2Epng"); |
There was a problem hiding this comment.
This PR will re-encode all image paths. I imagine this would be fairly disruptive for anyone expecting load+save to keep strings exactly as they are. Also existing apps that expect URIs to just be paths.
Perhaps this change in behaviour should be behind some TINYGLTF_URI_ENCODE_IMAGE_PATHS switch?
There was a problem hiding this comment.
glTF 2.0 spec allows Paths are as-is, with JSON string escaping, or with percent-encoding.
https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#uris
I think it'd be better adding URI encoding option to TinyGLTF class(e.g. SetSerializeURIOption()). Then we may need to further change WriteImageDataImageFunction signature to pass URI encoding option.
There was a problem hiding this comment.
That's a good point re. the spec. I like the dynamic option much more than a macro. Along those lines, what about providing a callback object like FsCallbacks for encoding URIs. Then the application could provide its own, to be used everywhere. It'd still be needed by WriteImageDataImageFunction - either passing the whole TinyGLTF context with some getURIEncoderCallbacks() or just the object itself. There's a lot of discussion about URIs in gltf here, which looks like a can of worms to open up.
BTW, I'm not attached to this PR and am happy to just close it if it overcomplicates things!
There was a problem hiding this comment.
what about providing a callback object like FsCallbacks for encoding URIs
Oh that's nice! Is
bool URIEncodeFunction(const std::string &in_uri, std::string *out_uri, void *user_data);
TinyGLTF::SetURIEncoder(URIEncodeFunction fun, void *userdata);
Suffice for encoding URI callback API design?(and App/User passes TinyGLTF context itself through userptr if required)
There was a problem hiding this comment.
Updated. WriteImageDataFunction is currently passed the URICallbacks struct. It might be more future proof to pass the context, but it'd need a ctx->GetURICallbacks(). Do you think that'd be better?
There was a problem hiding this comment.
After reading up gltf specs and URI discussions, I think at least it'd be better to add objectType parameter hint(which glTF object the URI belongs to. either 'buffer' or 'image' at the moment ) for callback function so that app/user can use different encoding/decoding depending on glTF Object type. API is something like
bool URIEncode(const std::string &in, const std::string &objectType, std::sting *out, void *userdata) {
if (objectType == "image") {
// texture specific URI encoding
} else { // "buffer"
}
...
}
We can use enums for objectType.
There was a problem hiding this comment.
Sounds good! This reminds me that the buffer URI isn't being encoded yet either. Updated.
Since the application may be writing a custom WriteImageData, I like the idea of having an enum so they're forced to pick one from the list. That said, the application might want enums of their own, e.g. for various gltf extensions not supported by tinygltf, so a string could be nice for that. I've currently left it as a string but I'm quite happy to change it.
I saw some functions passing object types such as "Buffer" to parent_node, but I think that's just for error reporting. If using an enum, should it be a complete list of all json object types that there are C++ equivalent structs for? The following is just the top level objects from Model. Or just start with IMAGE and BUFFER and add more if they're ever used?
enum ObjectType
{
OBJECT_TYPE_INVALID = 0,
OBJECT_TYPE_ACCESSOR,
OBJECT_TYPE_ANIMATION,
OBJECT_TYPE_BUFFER,
OBJECT_TYPE_BUFFER_VIEW,
OBJECT_TYPE_MATERIAL,
OBJECT_TYPE_MESH,
OBJECT_TYPE_NODE,
OBJECT_TYPE_TEXTURE,
OBJECT_TYPE_IMAGE,
OBJECT_TYPE_SKIN,
OBJECT_TYPE_SAMPLER,
OBJECT_TYPE_CAMERA,
OBJECT_TYPE_SCENE,
OBJECT_TYPE_LIGHT,
};
There was a problem hiding this comment.
Awesome!
In glTF 2.0 spec, URI is only defined for Image and Buffer. But considering possible uri parameter in glTF extensions as you mentioned, I think leave objectType parameter type string for flexibility! For example, in the future, we may fill objectType with "audio" for KHR_audio extension KhronosGroup/glTF#2137 )
There was a problem hiding this comment.
PR is now good, I've confirmed (unit) tests passes on my local side, so its ready to merge. @pknowlesnv do you plan to add further changes? if no, I'll do a merge!
There was a problem hiding this comment.
I have a minor change to simplify the uriencode lambda in tester.cc (building the string without using find/replace).
Provided that hasn't broken anything I think it's good to go. Tests still seem to pass, and if I break the encode function they fail.
Thanks for going over this!!
|
@pknowlesnv Thanks! Let me give some time to review PR. |
pknowlesnv
force-pushed
the
encode_image_uri
branch
3 times, most recently
from
673292e
to
788340d
Compare
Tinygltf is able to write files defined by a URI, so it needs to be able to decode it. Since it may also modify the path where the image is saved it may need to re-encode the URI too. This patch provides an API to set URI encoding and decoding callbacks and exposes the default decode method. Uses the existing dlib::urldecode as a decode default. The encode callback is left null, matching existing behaviour. Updates the WriteImageDataFunction signature to include tinygltf::URICallbacks. Decodes the image and buffer uris before using them as a filename. If the encode callback is set, encodes the written image location in the default WriteImageDataFunction and encodes generated buffer locations when writing .bin files. Adds a save+load step to the test image-uri-spaces to verify uri encoding.
pknowlesnv
force-pushed
the
encode_image_uri
branch
from
788340d
to
385946d
Compare
|
@pknowlesnv Thanks for great PR! Merged! |
Decodes the image uri before using it as a filename and encodes the written location in the default WriteImageDataFunction.
Copies dlib urlencode from
http://dlib.net/dlib/server/server_http.cpp.html (ver.19.24)