Skip to content

Conversation

VitalyAnkh
Copy link
Contributor

@VitalyAnkh VitalyAnkh commented Jun 8, 2025

Objective

Solution

Testing

  • example load_fbx, asset_loading.

Showcase

TODO

This section is optional. If this PR does not include a visual change or does not add a new feature, you can delete this section.

  • Help others understand the result of this PR by showcasing your awesome work!
  • If this PR adds a new feature or public API, consider adding a brief pseudo-code snippet of it in action
  • If this PR includes a visual change, consider adding a screenshot, GIF, or video
    • If you want, you could even include a before/after comparison!
  • If the Migration Guide adequately covers the changes, you can delete this section

While a showcase should aim to be brief and digestible, you can use a toggleable section to save space on longer showcases:

Click to view showcase
println!("My super cool code.");

Copy link
Contributor

github-actions bot commented Jun 8, 2025

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@VitalyAnkh VitalyAnkh added the A-Assets Load files from disk to use for things like images, models, and sounds label Jun 8, 2025
@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 8, 2025
Copy link
Contributor

github-actions bot commented Jun 8, 2025

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

@VitalyAnkh VitalyAnkh force-pushed the add_bevy_fbx branch 2 times, most recently from 8324b93 to 8bae92e Compare June 10, 2025 10:11
@mockersf
Copy link
Member

as it uses a native dependency, I don't think it should be part of the default features

@pcwalton
Copy link
Contributor

Agreed with @mockersf. FBX support would be very nice, but C++ dependencies add a lot of pain to the out-of-the-box experience in many cases, so let's make it opt-in.

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@VitalyAnkh
Copy link
Contributor Author

ufbx is a self-contained FBX loader made up of just two files—ufbx.c and ufbx.h. It depends only on libc and has no other significant prerequisites, so it’s much simpler to build than most C++ or Rust libraries. That said, I’m happy to make FBX support an optional feature if that’s the community consensus.

Copy link
Contributor

You added a new feature but didn't add a description for it. Please update the root Cargo.toml file.

@VitalyAnkh VitalyAnkh force-pushed the add_bevy_fbx branch 4 times, most recently from 11c909c to f39dd2d Compare June 20, 2025 02:39
@IceSentry
Copy link
Contributor

I know it's still WIP, but it would be way easier to review if the main load() function was split into multiple functions. A bit like how bevy_gltf has load_materials, load_image, load_node, etc.

Also, once the PR is ready to merge I think it should only have one .fbx file in the asset folder. It's a good feature to have but we need to keep the size of the repo in check.

@VitalyAnkh
Copy link
Contributor Author

I will refactor the main load() function following bevy_gltf. And there will be only one cube.fbx file in the repo, and I have already added a scene_viewer_fbx example to inspect other fbx files easily. Thanks for the nice advice!

Copy link
Contributor

You added a new feature but didn't add a description for it. Please update the root Cargo.toml file.

@pcwalton
Copy link
Contributor

Thought: if ufbx is just a C library, can we use c2rust on it so that users don't need a C compiler installed?

@BenjaminBrienen BenjaminBrienen added the S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. label Sep 25, 2025
@BenjaminBrienen
Copy link
Contributor

BenjaminBrienen commented Sep 25, 2025

I suspect that major portions of this contribution are AI generated. Em dash, overcommenting, commenting about "in the actual implementation..." and some other points discussed on Discord with other members. Nominating to close as per bevyengine/bevy-website#2204

Please correct me if I'm wrong!

@VitalyAnkh
Copy link
Contributor Author

I admit I made use of AI tools a lot to help me in this PR. I don't mean to deliberately violate community policies when I started in June 8, at that time Bevy Org's AI policy doesn't even exist, and I didn't know about it until today. I will close this in accordance with the AI policy, and I commit to adhering to the community code of conduct.

Anyway, the PR is in good shape now (especially good animation support) and if the AI policy is released earlier, I will follow the policy and won't use any AI tools.

@VitalyAnkh VitalyAnkh closed this Sep 25, 2025
@mholiv
Copy link

mholiv commented Sep 25, 2025

As someone who has been following this, but isn’t as familiar with the core workflow as I would like, what is happening?

Will this be merged, or is it dead, or does it need more feedback?

@BenjaminBrienen
Copy link
Contributor

Many Bevy maintainers want a version of this, so someone can contribute a new PR that implements it in a similar way.

@mholiv
Copy link

mholiv commented Sep 25, 2025

@BenjaminBrienen Beyond this PR including AI work did you find and architectural flaws with the implementation? I might give this a shot in early October with this as a reference. Maybe with @VitalyAnkh support if they are up for it.

@BenjaminBrienen
Copy link
Contributor

My review and opinion is not the most valuable, but I think basing it on ufbx like this is a good idea. It is a good direction.

@VitalyAnkh
Copy link
Contributor Author

@mholiv I could help you with a new bevy_fbx crate based on ufbx, without the interverntion of AI. I'm still interested in fbx and passionated with bevy.

@mholiv
Copy link

mholiv commented Sep 25, 2025

Or it might make sense for you to take lead here @VitalyAnkh given you know the code base as it is. I am just a passionate dev. Who want's to see the engine improve. That and the 3D artist I am working with REALLY isn't a fan of gltf lol.

@james7132
Copy link
Member

My review and opinion is not the most valuable, but I think basing it on ufbx like this is a good idea. It is a good direction.

I would be in support of this, provided this remains as a optional feature if it is using ufbx. We generally try to keep non-platform C/C++ build dependencies out of the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds M-Needs-Release-Note Work that should be called out in the blog due to impact S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support FBX file loading
8 participants