Skip to content

Conversation

@greeble-dev
Copy link
Contributor

@greeble-dev greeble-dev commented May 6, 2025

Objective

Add a test that reproduces #11111 (and partially #18267). The bug is that asset loader settings are effectively ignored if the same asset is loaded multiple times with different settings.

Solution

Add a unit test to bevy_assets/lib.rs. The test will be marked as #[ignore] until #11111 is fixed.

// Load the same asset with different settings.

let handle_1 = load(asset_server, "test.u8", 1);
let handle_2 = load(asset_server, "test.u8", 2);

// Handles should be different.

assert_ne!(handle_1, handle_2);

Concerns

I'm not 100% sure that the current behaviour is actually broken - I can't see anything in the asset system design docs that explicitly says different settings should create different asset ids.

UPDATE: Sentiment from issue comments and discord varies between "bug" and "undesirable consequence of design decisions, alternatives should be explored". So I've concluded that the test is valid and desirable.

Testing

cargo test -p bevy_asset --features multi_threaded

# Or to repro the issue:
cargo test -p bevy_asset --features multi_threaded -- --ignored

@greeble-dev greeble-dev added A-Assets Load files from disk to use for things like images, models, and sounds C-Testing A change that impacts how we test Bevy or how users test their apps S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels May 6, 2025
@alice-i-cecile
Copy link
Member

I think this behavior is a bug: doing distinct things with the same asset is a relatively common pattern.

@greeble-dev greeble-dev changed the title Add test that repros #18267 (different loader settings produce same asset) Add test that repros #11111 (different loader settings produce same asset) May 7, 2025
@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 Jul 2, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 2, 2025
Merged via the queue into bevyengine:main with commit 05d954e Jul 2, 2025
34 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Aug 15, 2025
…in AssetServer::load() (#20562)

# Objective

- Fixes #19844

## Solution

- Revert #10823
- Also reverts #19094, as the ignored test added there does not compile
with the new lifetime rules.

## TODO

This PR was made > 10k issues ago, and git can do funky things. Opening
as a draft to investigate the diff before proceeding.

- [x] merge main into the old branch
- [x] revert the PR 
- [x] make sure the diff looks okay
- [x] add comments explaining why we did this weird thing

---------

Co-authored-by: Zac Harrold <zac@harrold.com.au>
andriyDev added a commit to andriyDev/bevy that referenced this pull request Oct 16, 2025
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-Testing A change that impacts how we test Bevy or how users test their apps 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 X-Contentious There are nontrivial implications that should be thought through

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants