feat: Add support for async loading using either futures or tokio AsyncRead traits
#69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove duplicated code between load_obj_buf() and load_obj_buf_async(), by refactoring out the majority of logic to helper structs. Now, only the async-specific logic is different in load_obj_buf_async(), which should significantly help in preventing it from bitrotting.
In order to allow proper async loading of objs and materials, the reader needs to be async itself. Unfortunately there are no standard async read/write traits, so the best solution is to implement per-framework async functions for the known big frameworks, gated behind feature flags. This commit adds support for `futures` AsyncRead traits.
In order to allow proper async loading of objs and materials, the reader needs to be async itself. Unfortunately there are no standard async read/write traits, so the best solution is to implement per-framework async functions for the known big frameworks, gated behind feature flags. This commit adds support for `tokio` AsyncRead traits.
debroejm
commented
Jan 13, 2025
debroejm
commented
Jan 13, 2025
Add the missing `async` dependency to futures/tokio features, so they also pull in the relevant async bits. Also do a slight rename to fix a warning for building with only the `futures` feature.
Twinklebear
approved these changes
Jan 20, 2025
There was a problem hiding this comment.
Thanks @debroejm I think this looks good! We can just mark the old async API deprecated and then this is good to
The original load_obj_buf_async() function is not properly async, since it doesn't use async readers. Deprecate this function and point to the new ones.
|
Added the deprecation. Hmm, there is theoretical performance improvement in the async path, where loading materials is delayed until after the entire OBJ buffer is read, and then all materials can be asynchronously loaded at the same time (instead of one at a time while reading the OBJ buffer), but this would only really matter for an OBJ with a quite large count of separate material buffers, and would require a significant change in logic to delay the material loading. Probably something that can be added in the future if deemed necessary. |
debroejm
commented
Jan 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #68
Implements AsyncRead trait support for specifically
futuresandtokio. Other frameworks can of course be added as needed.