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

Loading an asset that has been planned for removal does not clear the removed event. #824

Closed
BorisBoutillier opened this issue Nov 9, 2020 · 3 comments
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior

Comments

@BorisBoutillier
Copy link
Contributor

Bevy version

v0.3.0

Operating system & version

Ubuntu 20.04

What you did

I have migrated Kataster to bevy 0.3.0 ( https://github.com/Bobox214/Kataster ).
The HEAD compile and runs but show a regression in UI behaviour.

I have a start screen with 2 Text components.
On pressing enter:
a. On first frame, the two UI Text of the start screen are despawned.
b. On the second frame, the UI Text for the Score is added.

The three UI Text are using the same font.

What you expected to happen

The Score Text should be shown

What actually happened

The Score text is only shown for one or two frames then disappear.

Additional information

The Score UI is no more shown because the font is being removed from the Assets.

As far as I have been able to debug :

  • The removal of the 2 UI Text, decrease the font handle refCount to 0, this triggers an AssetEvent::Removed for the font.
  • The addition of the score UI Text, does not do anything because the font is still present in the assets. ( goes into the if branch of // if asset is already loaded (or is loading), don't load again )
  • The effective handling of the AssetEvent::Removed happens after the score UI Text addition, and effectively remove the font used by the Score UI.
@BorisBoutillier BorisBoutillier added the C-Bug An unexpected or incorrect behavior label Nov 9, 2020
@BorisBoutillier
Copy link
Contributor Author

No sure how to fix that.

In the asset_server, I don't think we can have access to the Events<Assets> to check for already planed for removal assets and trigger a 'force'.
In the Assets , when handling the Removed event, we don't have access to the ref_counts to double check the Handle is still ref_count 0.

If somebody can give me the appropriate direction to handle this issue, I am eager to try to implement it.

@Moxinilian Moxinilian added the A-Assets Load files from disk to use for things like images, models, and sounds label Nov 9, 2020
@memoryruins
Copy link
Contributor

memoryruins commented Nov 12, 2020

Thanks for reporting! Perhaps its worth making a branch on Kataster specific to this issue so that it preserves the behavior you ran into. I'll mention the most recent commit for posterity: BorisBoutillier/Kataster@98bafc3


Something that could avoid this behavior in Kataster today is storing a strong handle to the font in a longer living resource. Since the UI is always using the same font, the resource can be re-used in all the systems that need it. This would avoid loading the asset more than once, and would allow weak handles to be used by all UI elements if it's never removed.

example
fn main() {
    App::build()
        .add_plugins(DefaultPlugins)
        .init_resource::<GameFonts>()
        .add_system(system.system())
        .run()
}

struct GameFonts {
    primary: Handle<Font>,
}

impl FromResources for GameFonts {
    fn from_resources(resources: &Resources) -> Self {
        let assets = resources.get::<AssetServer>().unwrap();
        GameFonts { primary: assets.load("kenvector_future.ttf") }
    }
}

fn system(game_fonts: Res<GameFonts>) {
    // ... game_fonts.primary.clone() ...
    // ... game_fonts.primary.clone_weak() ...
}

@BorisBoutillier
Copy link
Contributor Author

Good idea effectively, I have created the following branch that allow to reproduce the described issue :
https://github.com/Bobox214/Kataster/tree/bevy_issue_824

Effectively, Kataster does not try to optimize assets handles, like for the laser sprite I re-get the handle from the path each new entity creation ....
I probably should do it as multiple people are taking this project as a starting point, so better provide a good one :)

@bors bors bot closed this as completed in 4f0499b May 4, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this issue Jul 27, 2021
fixes bevyengine#824
fixes bevyengine#1956 

* marked asset loading methods as `must_use`
* fixed asset re-loading while asset is still loading to work as comment is describing code
* introduced a 1 frame delay between unused asset marking and actual asset removal
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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants