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

feature request: add Load[DAE/GLTF]FromReader(io.Reader) #32

Open
btvoidx opened this issue Jan 11, 2024 · 3 comments
Open

feature request: add Load[DAE/GLTF]FromReader(io.Reader) #32

btvoidx opened this issue Jan 11, 2024 · 3 comments

Comments

@btvoidx
Copy link

btvoidx commented Jan 11, 2024

Right now one has to io.ReadAll before passing the data slice into LoadGLTFData which wraps it in a reader, so why not pass a reader directly? Loading from file is not ideal, since assets may be embedded via //go:embed into an embed.FS (my case!), which also brings up a point of adding Load[DAE/GLTF]FromFS.

Looking at LoadDAEData, I assume it could also use a xml.NewDecoder, but if there is some arcane reasoning behind umarshaling it three times separately from a byte slice, it could just io.ReadAll to get it from the provided reader.

@SolarLune
Copy link
Owner

SolarLune commented Feb 2, 2024

Sorry about that, I missed this issue.

OK, I see what you mean, that does make sense and seems like it would be simpler. I'll see about updating the functions to just take an io.Reader instead of a byte slice.

I haven't really returned to the DAE loader since I implemented the GLTF loader, to be honest - what you mention is one way that it could be simplified and cleaned up.

@SolarLune
Copy link
Owner

SolarLune commented Feb 6, 2024

I added a commit (bb729df) that should resolve this for GLTF - I'm not sure about DAE because I kind of feel like just deprecating DAE entirely and moving away from it.

I'm kind of ambivalent on supporting it, considering I don't use it, the Tetra3D add-on doesn't use it, and I generally wouldn't recommend anyone else to, either.

@btvoidx
Copy link
Author

btvoidx commented Feb 6, 2024

I don't think anyone uses DAE. Quick code search across GitHub revealed just one public instance of it being used not in Tetra3D fork.

If you decide to deprecate it then the issue can be closed - v0.15.0 is out and DAE part won't be done.

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

No branches or pull requests

2 participants