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

russimp: Introduce FileSystem loading #56

Merged
merged 5 commits into from
Jan 16, 2024
Merged

Conversation

waych
Copy link
Contributor

@waych waych commented Nov 24, 2023

When loading resources from memory buffers, sometimes the resources in question link to secondary files that must also be loaded. An example of this is the loading of .obj files, which store their materials in a secondary .mtl file. This file can't be passed in using the from_buffer() loading path, resulting in the need to load the files from the host native filesystem using the from_file() loading path.

This change implements a rust safe loading path to allowing applications to implement their own virtual filesystem interface with assimp. This is convenient for use in combination with an application specific resource loader.

To use, callers simply implement the fs::FileSystem trait, which neccesitates the implementation of open() which returns their own type as a trait object implementing fs::FileOperations.

This new fs module handles all the unsafe glue required to interface this safe trait-based interface with the underlying unsafe aiFileIO parts.

When loading resources from memory buffers, sometimes the resources in
question link to secondary files that must also be loaded.  An example
of this is the loading of .obj files, which store their materials in a
secondary .mtl file.  This file can't be passed in using the
from_buffer() loading path, resulting in the need to load the files from
the host native filesystem using the from_file() loading path.

This change implements a rust safe loading path to allowing applications
to implement their own virtual filesystem interface with assimp. This is
convenient for use in combination with an application specific resource
loader.

To use, callers simply implement the fs::FileSystem trait, which
neccesitates the implementation of open() which returns their own type
as a trait object implementing fs::FileOperations.

This new fs module handles all the unsafe glue required to interface
this safe trait-based interface with the underlying unsafe aiFileIO
parts.
@waych
Copy link
Contributor Author

waych commented Nov 24, 2023

Build is failing due to lack of aiFileIO, which isn't exposed in the depended-upon version of russimp-sys. I had to add "cfileio.h" to my local copy of russimp-sys's wrapper.h to have the bindings generated for the type.

I see that cfileio.h is enabled in the latest russimp-sys main branch, but I haven't been able to switch to the new build there as of yet (missing "assimp/cimport.h"?). Haven't dug into debugging why it fails for me yet.. I see that this is blocked on moving russimp itself over to the russimp-sys 2.0. I guess this can sit until #50 is resolved, or another 1.0 release is pushed with cfileio.h included in wrapper.h.

Fixed various issues when building against russimp-sys 2.0.
Remove unneccesary and wrong mut usage. FileSystem is only used as an
immutable reference, usage expects user to use internal mutability if
they so desire it, as this is the least restrictive option vs requiring
mutable FileSystem.
@jkvargas
Copy link
Owner

jkvargas commented Jan 4, 2024

hey mate, thanks so much for your contribution.
I am not having a lot of time to work on russimp lately.
but I am accepting PRs.

Are you up to update russimp to use russimp-sys 2.0?
Thanks for the patience, man! =)

@waych
Copy link
Contributor Author

waych commented Jan 6, 2024

I have russimp-sys 2.0 working locally, so this shouldn't be too much of a problem. I think I'm not hitting the issue mentioned in #50 however as russimp-sys is coming into my build as a git submodule and therefore am also pulling the nested assimp codebase.

Is the issue blocking #50 just that a copy of the assimp.h header is required in the crate distribution e.g. for when not using the build-assimp feature?

@jkvargas
Copy link
Owner

jkvargas commented Jan 16, 2024

I am going to merge it and fix possible issues. =)

Thanks for your contribution again @waych

@jkvargas jkvargas merged commit 9eb5883 into jkvargas:master Jan 16, 2024
1 of 3 checks passed
@waych
Copy link
Contributor Author

waych commented Jan 16, 2024

Thanks for the pull!

I'll admit that I tried to kick the tires here to make sure they still fit, but I kept getting hung up on the fact that the new russimp-sys doesn't include sources, only headers. This doesn't affect me for my local project as I'm providing the library in elsewhere in my project, but for russimp stand alone, it would seem feature="static-link" cannot be relied upon to the build locally anymore when pulling the crate from crates.io?

@jkvargas
Copy link
Owner

jkvargas commented Feb 2, 2024

@waych hey man, I believe that makes sense...
I wouldn't mind if you want to put your hands on that as well =)

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

Successfully merging this pull request may close these issues.

2 participants