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

Make adding a subasset label return a result for if there is a duplicate label. #18013

Merged
merged 3 commits into from
Feb 24, 2025

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • Make LoadContext::add_labeled_asset and friends return an error if it finds a duplicate asset.

Testing

  • Added a test - it fails before the fix.

Migration Guide

  • AssetLoaders must now handle the case of a duplicate subasset label when using LoadContext::add_labeled_asset and its variants. If you know your subasset labels are unique by construction (e.g., they include an index number), you can simply unwrap this result.

@andriyDev andriyDev requested a review from pcwalton February 24, 2025 06:47
@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 labels Feb 24, 2025
@andriyDev andriyDev added this to the 0.16 milestone Feb 24, 2025
Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Of course we should have some kind of solution for the problem, but this will at least prevent others from suffering the same difficulty debugging the issue.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Uncontroversial This work is generally agreed upon and removed C-Bug An unexpected or incorrect behavior labels Feb 24, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 24, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 24, 2025
Merged via the queue into bevyengine:main with commit ed1143b Feb 24, 2025
34 checks passed
@andriyDev andriyDev deleted the nested-labels branch February 25, 2025 01:06
pcwalton added a commit to pcwalton/bevy that referenced this pull request Feb 25, 2025
Currently, we reload a glTF skin each time we encounter a node that
references it. By checking for duplicates, PR bevyengine#18013 turned this into a
fatal error. But this was always wasteful. This commit fixes the issue
by caching each skin by its index as we load it.

The Maya babylon.js export plugin likes to emit glTFs with multiple
nodes that reference the same skin, so this effectively unbreaks Maya
rigs.
github-merge-queue bot pushed a commit that referenced this pull request Feb 25, 2025
Currently, we reload a glTF skin each time we encounter a node that
references it. By checking for duplicates, PR #18013 turned this into a
fatal error. But this was always wasteful. This commit fixes the issue
by caching each skin by its index as we load it.

The Maya babylon.js export plugin likes to emit glTFs with multiple
nodes that reference the same skin, so this effectively unbreaks Maya
rigs.
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants