search

Twinklebear / tobj

Tiny OBJ Loader in Rust
MIT License
233 stars 47 forks source link

feat: Add Async obj_load_buf #46

Closed hwoodiwiss closed 3 years ago

hwoodiwiss commented 3 years ago

fixes #45

Twinklebear commented 3 years ago

This looks good! My one thought is this is duplicating quite a bit of code, but I see the issue of pushing an async only API on to folks if we make just load_obj_buf become async when built with the async feature.

Since this does add a new MTL loading path though could we add a test that will run the async version as well? I think we can just copy the existing load_obj_buf test: https://github.com/Twinklebear/tobj/blob/master/src/tests.rs#L468-L489 and make it run the async version behind the feature gate. The other test using load_obj_buf just demonstrates opening a file during the load so I don't think we need to run that one on async (referring to this test: https://github.com/Twinklebear/tobj/blob/master/src/tests.rs#L491-L529)

hwoodiwiss commented 3 years ago

Adding a test sounds sensible to me.

In regards to the duplicated code, yeah, it's not great. I went back and forth on that a bit, but I couldn't see a way to just replace the mtllib branch and leave the rest in a shared location between the sync and async versions, although I do intend to keep thinking about it, and if I come up with something in the future, I'll try and implement that.

Twinklebear commented 3 years ago

Thanks @hwoodiwiss , this looks great! I'll merge and push a release to crates.io shortly. And I agree on the async/sync split, it's tricky to see how to unify them without making one of the code paths awkward. Maybe we could default to calling the async API from the sync one, and just do a wait there, but I'm not sure if that would still effect the public API.

For now I think this is good, and easier to put out as a minor version release since it doesn't change any existing APIs.