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

Add ability to load scenes with custom properties #62

Merged
merged 5 commits into from
Feb 4, 2024

Conversation

cmilatinov
Copy link
Contributor

  • Added a PropertyStore struct to hold Assimp properties
  • Added corresponding from_*_with_props functions to Scene to load scenes with a property store
  • Use match statements in Scene loader functions

Example scene loading code

let props: PropertyStore = [(
    AI_CONFIG_IMPORT_FBX_PRESERVE_PIVOTS as &[u8],
    Property::Integer(0),
)]
    .into_iter()
    .into();
let scene = Scene::from_file_with_props(
    path.to_str().unwrap(),
    vec![
        PostProcess::Triangulate,
        PostProcess::GenerateSmoothNormals,
        PostProcess::FlipUVs,
        PostProcess::FlipWindingOrder,
        PostProcess::JoinIdenticalVertices,
    ],
    &props,
)?;

@jkvargas
Copy link
Owner

jkvargas commented Feb 2, 2024

Hey mate, thanks for your contribution,
I would like to ask you somethings before merging your work.

  • Raise the version to 3.2.0
  • Add to the changelog your new feature, would be cool if you can add an example.
  • I don't see tests on your changes. Would you mind to add a test to validate the features you're exposing to the end user?
  • finally, run cargo fmt =)

@cmilatinov
Copy link
Contributor Author

Hey,

Yessir, no problem I will go through the checklist ASAP and update the PR.

Cheers

@cmilatinov
Copy link
Contributor Author

Hey @jkvargas,

I've made the changes you requested, just requesting a final review.

I added an example usage for props in the examples folder :)

Also, I'm a windows user atm, so I hope running the tests with --features prebuilt is the correct thing to do

PS: Very cool rust package, I'm glad to be able to contribute!

Have a nice day!

@jkvargas
Copy link
Owner

jkvargas commented Feb 3, 2024

Thanks for the additions, man.
There are some tests failing.
With those fixed I believe we are good to go.

@cmilatinov
Copy link
Contributor Author

Yea its a compilation error since the bindings.rs file isn't generated for the tests I believe so rust can't find the AI_* constants.

error[E0432]: unresolved imports `crate::sys::AI_CONFIG_IMPORT_COLLADA_IGNORE_UP_DIRECTION`, `crate::sys::AI_CONFIG_IMPORT_COLLADA_USE_COLLADA_NAMES`, `crate::sys::AI_CONFIG_IMPORT_FBX_PRESERVE_PIVOTS`, `crate::sys::AI_CONFIG_IMPORT_FBX_READ_WEIGHTS`, `crate::sys::AI_CONFIG_PP_OG_EXCLUDE_LIST`, `crate::sys::AI_CONFIG_PP_PTV_ADD_ROOT_TRANSFORMATION`, `crate::sys::AI_CONFIG_PP_PTV_ROOT_TRANSFORMATION`
   --> src/property.rs:110:9
    |
110 |         AI_CONFIG_IMPORT_COLLADA_IGNORE_UP_DIRECTION, AI_CONFIG_IMPORT_COLLADA_USE_COLLADA_NAMES,
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `AI_CONFIG_IMPORT_COLLADA_USE_COLLADA_NAMES` in the root
    |         |
    |         no `AI_CONFIG_IMPORT_COLLADA_IGNORE_UP_DIRECTION` in the root
111 |         AI_CONFIG_IMPORT_FBX_PRESERVE_PIVOTS, AI_CONFIG_IMPORT_FBX_READ_WEIGHTS,
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `AI_CONFIG_IMPORT_FBX_READ_WEIGHTS` in the root
    |         |
    |         no `AI_CONFIG_IMPORT_FBX_PRESERVE_PIVOTS` in the root
112 |         AI_CONFIG_PP_OG_EXCLUDE_LIST, AI_CONFIG_PP_PTV_ADD_ROOT_TRANSFORMATION,
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `AI_CONFIG_PP_PTV_ADD_ROOT_TRANSFORMATION` in the root
    |         |
    |         no `AI_CONFIG_PP_OG_EXCLUDE_LIST` in the root
113 |         AI_CONFIG_PP_PTV_ROOT_TRANSFORMATION,
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `AI_CONFIG_PP_PTV_ROOT_TRANSFORMATION` in the root

For more information about this error, try `rustc --explain E0432`.
error: could not compile `russimp` (lib test) due to previous error

That's why I ran the tests with --features prebuilt locally. Should I just manually copy-paste the constant definitions in the tests?

@jkvargas
Copy link
Owner

jkvargas commented Feb 4, 2024

Hey man, I will disable the tests for now till I get a better understanding on what is going on. =)
Thanks brother.

@jkvargas jkvargas merged commit 8fc62d1 into jkvargas:master Feb 4, 2024
1 of 3 checks passed
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