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.

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

texture atlas example: mark each loaded image as CPU persistent #11202

Closed
wants to merge 1 commit into from
Closed

texture atlas example: mark each loaded image as CPU persistent #11202

wants to merge 1 commit into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Jan 3, 2024

Objective

...
2024-01-03T17:58:18.478750Z  WARN texture_atlas: textures/rpg/tiles/generic-rpg-tile05.png did not resolve to an `Image` asset.
2024-01-03T17:58:18.478824Z  WARN texture_atlas: textures/rpg/tiles/generic-rpg-tile04.png did not resolve to an `Image` asset.
2024-01-03T17:58:18.478827Z  WARN texture_atlas: textures/rpg/tiles/generic-rpg-tile10.png did not resolve to an `Image` asset.
2024-01-03T17:58:18.478830Z  WARN texture_atlas: textures/rpg/tiles/generic-rpg-tile38.png did not resolve to an `Image` asset.
2024-01-03T17:58:18.478833Z  WARN texture_atlas: textures/rpg/tiles/generic-rpg-Slice.png did not resolve to an `Image` asset.
thread 'Compute Task Pool (0)' panicked at examples/2d/texture_atlas.rs:153:10:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `texture_atlas::setup`!
Encountered a panic in exclusive system `bevy_ecs::schedule::state::apply_state_transition<texture_atlas::AppState>`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

Solution

  • mark each image as CPU persistent as they are loaded

@mockersf mockersf added C-Examples An addition or correction to our examples A-Assets Load files from disk to use for things like images, models, and sounds labels Jan 3, 2024
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Jan 3, 2024
@JMS55
Copy link
Contributor

JMS55 commented Jan 3, 2024 •
edited
Loading

Hmm, this is kinda an unfortunate example. If we were loading sprites individually, we could use load_with_settings() and specify the persistence policy in the loader settings. Not really possible with the folder API...

We could configure meta files per sprite, but in this case I suppose this is the best way.

Really though, we should be able to still access the sprites the first frame they're loaded. So I'm not sure we need to Keep them anyways. Perhaps the asset event is getting sent a frame later?

@alice-i-cecile
Copy link
Member

Would a load_folder_with_settings API make sense here?

@JMS55
Copy link
Contributor

JMS55 commented Jan 3, 2024

Would a load_folder_with_settings API make sense here?

Load folder is untyped, as it can load multiple different types of assets at once. I suppose we could restrict the with_settings variant to only load one type of asset, and skip the others.

@mockersf
Copy link
Member Author

mockersf commented Jan 3, 2024

Really though, we should be able to still access the sprites the first frame they're loaded. So I'm not sure we need to Keep them anyways. Perhaps the asset event is getting sent a frame later?

frame 1

  • trigger folder load

frame 2

  • some files are loaded

frame 3

  • previous images are unloaded
  • new files are loaded

frame 4:

  • previous images are unloaded
  • new files are loaded
  • folder is done loading
  • trying to access all images, which fails

@JMS55
Copy link
Contributor

JMS55 commented Jan 4, 2024

Can you simply add each image as it finishes loading, instead of all at once at the end?

@hymm
Copy link
Contributor

hymm commented Jan 4, 2024

This fixes the example, but shouldn't we do something for texture atlases in general? Having the user manually mark all their images as persistent feels hacky.

@JMS55
Copy link
Contributor

JMS55 commented Jan 4, 2024

We could default to Keep for anything not loaded through the GLTF loader I suppose 🤔 .

@mockersf mockersf mentioned this pull request Jan 4, 2024
Merged
@mockersf
Copy link
Member Author

mockersf commented Jan 4, 2024

We could default to Keep for anything not loaded through the GLTF loader I suppose 🤔 .

My personal opinion is that everything should be Keep by default: #11212

github-merge-queue bot pushed a commit that referenced this pull request Jan 5, 2024
@mockersf
assets should be kept on CPU by default (#11212)
425570a 
# Objective

- Since #10520, assets are unloaded from RAM by default. This breaks a
number of scenario:
  - using `load_folder`
- loading a gltf, then going through its mesh to transform them /
compute a collider / ...
- any assets/subassets scenario should be `Keep` as you can't know what
the user will do with the assets
  - android suspension, where GPU memory is unloaded

- Alternative to #11202 

## Solution

- Keep assets on CPU memory by default
Maximetinu pushed a commit to Sophya/bevy that referenced this pull request Feb 12, 2024
@mockersf @Maximetinu
assets should be kept on CPU by default (bevyengine#11212)
b8fcece 
# Objective

- Since bevyengine#10520, assets are unloaded from RAM by default. This breaks a
number of scenario:
  - using `load_folder`
- loading a gltf, then going through its mesh to transform them /
compute a collider / ...
- any assets/subassets scenario should be `Keep` as you can't know what
the user will do with the assets
  - android suspension, where GPU memory is unloaded

- Alternative to bevyengine#11202 

## Solution

- Keep assets on CPU memory by default
Maximetinu pushed a commit to Sophya/bevy that referenced this pull request Feb 12, 2024
@mockersf @Maximetinu
assets should be kept on CPU by default (bevyengine#11212)
fe65117 
# Objective

- Since bevyengine#10520, assets are unloaded from RAM by default. This breaks a
number of scenario:
  - using `load_folder`
- loading a gltf, then going through its mesh to transform them /
compute a collider / ...
- any assets/subassets scenario should be `Keep` as you can't know what
the user will do with the assets
  - android suspension, where GPU memory is unloaded

- Alternative to bevyengine#11202 

## Solution

- Keep assets on CPU memory by default
Maximetinu pushed a commit to Sophya/bevy that referenced this pull request Feb 12, 2024
@mockersf @Maximetinu
assets should be kept on CPU by default (bevyengine#11212)
b23cb42 
# Objective

- Since bevyengine#10520, assets are unloaded from RAM by default. This breaks a
number of scenario:
  - using `load_folder`
- loading a gltf, then going through its mesh to transform them /
compute a collider / ...
- any assets/subassets scenario should be `Keep` as you can't know what
the user will do with the assets
  - android suspension, where GPU memory is unloaded

- Alternative to bevyengine#11202 

## Solution

- Keep assets on CPU memory by default
Maximetinu pushed a commit to Sophya/bevy that referenced this pull request Feb 12, 2024
@mockersf @Maximetinu
assets should be kept on CPU by default (bevyengine#11212)
7e6d817 
# Objective

- Since bevyengine#10520, assets are unloaded from RAM by default. This breaks a
number of scenario:
  - using `load_folder`
- loading a gltf, then going through its mesh to transform them /
compute a collider / ...
- any assets/subassets scenario should be `Keep` as you can't know what
the user will do with the assets
  - android suspension, where GPU memory is unloaded

- Alternative to bevyengine#11202 

## Solution

- Keep assets on CPU memory by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@JMS55 JMS55 Awaiting requested review from JMS55

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Examples An addition or correction to our examples
Projects
None yet
Milestone
0.13
Development

Successfully merging this pull request may close these issues.
4 participants
@mockersf @JMS55 @alice-i-cecile @hymm