Skip to content

Fix GLTF files being broken when loaded from custom asset sources. #21643

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix GLTF files being broken when loaded from custom asset sources. #21643

wants to merge 4 commits into from
+161 −16

Conversation

@andriyDev
Copy link
Contributor

Objective

Solution

  • Stop using Path::join for joining asset paths and instead use AssetPath::resolve_embed.

Testing

  • Added 2 tests!

@andriyDev andriyDev added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-glTF Related to the glTF 3D scene/model format labels Oct 23, 2025
@andriyDev
Copy link
Contributor Author

As an aside, I think we should remove LoadContext::path and just stick to supporting LoadContext::asset_path. It seems like a footgun to provide the std::path::Path since they don't work with custom asset soruces. If a user who needs the raw Path, they can just do context.asset_path().path().

That's for a future PR though.

@alice-i-cecile alice-i-cecile added this to the 0.17.3 milestone Oct 23, 2025
mockersf
mockersf reviewed Oct 23, 2025
View reviewed changes
mockersf
mockersf reviewed Oct 23, 2025
View reviewed changes
@viridia
Copy link
Contributor

viridia commented Oct 23, 2025

Yes, please, kill it with fire!

@andriyDev andriyDev requested a review from mockersf October 23, 2025 20:00
@andriyDev andriyDev added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mockersf mockersf mockersf approved these changes

@torsteingrindvik torsteingrindvik Awaiting requested review from torsteingrindvik

+1 more reviewer

@ChristopherBiscardi ChristopherBiscardi ChristopherBiscardi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Assets Load files from disk to use for things like images, models, and sounds A-glTF Related to the glTF 3D scene/model format C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

0.17.3

Development

Successfully merging this pull request may close these issues.

Custom asset source does not work with GLTF

5 participants

@andriyDev @viridia @ChristopherBiscardi @mockersf @alice-i-cecile