Add gltf read interface and ReaderOptions - #100
Conversation
30ea97e to
716b33b
Compare
alexykot
changed the title
|
Still taking a look at this, but main now successfully builds, if you want to rebase |
716b33b to
35ba4bc
Compare
|
|
||
| // StandardBufferLoader implements the BufferLoader interface for loading buffers from | ||
| // the file system or data URIs. | ||
| type StandardBufferLoader struct { |
There was a problem hiding this comment.
If we wanted to, we could have this struct just implement both the Buffer and Image interface, and remove the duplicate BasePath field the two structs share.
Doesn't matter too much to me either way if you want to keep it how it is at the moment. Just thought I'd bring it up.
| type StandardBufferLoader struct { | |
| type StandardLoader struct { |
There was a problem hiding this comment.
Nice idea, I'll do that 🙂
Not committing because it's easier for me to do it in the IDE to replace it everywhere at once.
| } | ||
|
|
||
| // decodeDataURI extracts binary data from a data URI | ||
| func decodeDataURI(dataURI string) ([]byte, error) { |
There was a problem hiding this comment.
It might be worth making this slightly more generic for the sake of re-use. We could do this by having it also return mimetype. That way, some of the image loading can take advantage of this
| func decodeDataURI(dataURI string) ([]byte, error) { | |
| func decodeDataURI(dataURI string) ([]byte, string, error) { |
There was a problem hiding this comment.
Hmm, I assume you meant to do this for the images and not for the data buffers.
For images, there are just a couple of possible options, and the way to retrieve them is very trivial, works fine in both files and data URIs. I've added that. It's not really used right now, but it's returned from the loader interface and enables future use cases. I agree, it really makes sense here.
For data buffers, if it's a URI, then you have a content type in it, but if it's a binary buffer file, then it becomes very nontrivial to understand what mimetype that was supposed to be.
I've added this for images, that makes sense, but I skipped this for the buffers.
| // log.Fatal(err) | ||
| // } | ||
| func Parse(r io.Reader) (*Gltf, error) { | ||
| data, err := io.ReadAll(r) |
There was a problem hiding this comment.
Since we're receiving a reader, we could take care and only read what's required of us to form the GLTF document, supporting the usecase of decoding multiple values from a stream of data
decoder := json.NewDecoder(reader)
g := &Gltf{}
decoder.Decode(g)| } | ||
|
|
||
| // Test custom loaders | ||
| type mockBufferLoader struct { |
There was a problem hiding this comment.
Honestly might not be the worst thing to just have in the gltf package itself. Something like
type InMemoryBufferLoader map[string][]byteMight be overkill though, so I'll leave that up to you.
There was a problem hiding this comment.
I think it would be trivial for users to implement so including this in the package directly might not be worth it.
Also, the mock loader is not the same as a memory loader. Right now they are practically the same thing, but mock loader really should also have an Expectation method which is purely a test concern and should not be in the production code. So I wouldn't mix these things together.
2 participants
This PR adds a production interface for the GLTF read operations and support for
ReaderOptions.After a merge of the previous PR, I realized that what I needed to achieve isn't yet possible, and I needed an interface that could load glTF from a byte stream, rather than only from a file. So I updated the interface to be able to do that, and I thought that I could as well make this a production interface now, since it supports most of the features now.
Also, I need a way to load the GLTF JSON part without having to supply all the images. I need to do operations and transformations on the JSON itself, and having to always provide the images on the file system paths in whatever place this code is running in - is very cumbersome and limiting.
In my use case, I am loading glTF JSON from DB storage with the goal of including a small static model into a much bigger scene. And this code is running in a container in GCP. So having the images on a local in-memory file system in the container is completely pointless. Real images will be loaded on the client side in the browser over the web, and the URIs are specifically designed to work with that.
So I redesigned loading process and added
ReaderOptionssupport to be able to provide aNoOpLoaderthat doesn't force me to provide the images on the same paths as in the URIs in the GLTF being loaded.All the tests are updated accordingly.
@EliCDavis I'm not sure if this is ready to go out of the experimental phase in your view. I think this is developed enough to remove those prefixes, but I'll follow your lead on this one. And this is still a
v0, that doesn't go anywhere.PS:fixed nowBuild / pagesis still failing, and still not my fault 🙂