JonahPlusPlus / bevy_atmosphere

A procedural sky plugin for bevy
Apache License 2.0
267 stars 19 forks source link

Crash with latest 0.9 update #69

Closed VirxEC closed 7 months ago

VirxEC commented 8 months ago

With this code:

fn daylight_cycle(mut atmosphere: AtmosphereMut<Nishita>) {
    atmosphere.sun_position = Vec3::new(1., 0.5, 0.);
}

This results in a crash with the following error:

thread 'main' panicked at /home/virx/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_atmosphere-0.9.0/src/pipeline.rs:370:53:
prepare_changed_settings should have took care of making AtmosphereImage.array_value Some(TextureView)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_atmosphere::pipeline::queue_atmosphere_bind_group`!

If I don't modify any settings then it doesn't crash.

Shatur commented 8 months ago

@RobWalt could you take a look into it?

RobWalt commented 8 months ago

Sure, I'm back on monday 👍

RobWalt commented 8 months ago

I can't reproduce this. I added the following code to the nishita example to test the issue:

// ...

    App::new()
        .init_resource::<AtmosphereModel>() // <- new
        .add_plugins((DefaultPlugins, AtmospherePlugin, SpectatorPlugin))
        .add_systems(Startup, setup)
        .add_systems(Update, change_nishita)
        .add_systems(Update, daylight_cycle) // <- new
        .run();

// ...

fn daylight_cycle(mut atmosphere: AtmosphereMut<Nishita>, time: Res<Time>) {
    let (sin, cos) = (time.elapsed_seconds() * 0.1).sin_cos();
    let sun_position = Vec3::new(sin, cos, 0.0);
    atmosphere.sun_position = sun_position;
}

// ...

For me it works as expected, i.e. there's a day-night cycle. Please provide further information on what you did. It might also be related to your hardware/drivers. I'm not an expert on how to resolve those issues though.

VirxEC commented 8 months ago

I am closing this issue now. I still have no idea what caused this error but the issue appears to have resolved itself. I don't know why, I don't know how, but it's working now... gotta love software :)

Sorry for wasting your time haha

RobWalt commented 8 months ago

Glad for you it fixed itself. I guess that's the optimal scenario for us all: No need to fix something on either your or the code's side of things 😄 And don't worry about me wasting my time. I wouldn't do it if it wasn't a bit fun for me!

Neopallium commented 8 months ago

I had this crash too (in bevy_water ocean example), but then the problem went away.

Could it be caused by some caching/race condition? If the shaders are cached by the graphic driver (not sure if this happens), then the second run might load faster?

2024-02-27T17:42:48.663008Z  INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "Linux 20.04 Ubuntu", kernel: "5.13.0-30-generic", cpu: "AMD Ryzen 9 5900HX with Radeon Graphics", core_count: "8", memory: "31.2 GiB" }
thread 'main' panicked at /home/robert/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_atmosphere-0.9.0/src/pipeline.rs:370:53:
prepare_changed_settings should have took care of making AtmosphereImage.array_value Some(TextureView)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_atmosphere::pipeline::queue_atmosphere_bind_group`!
JonahPlusPlus commented 8 months ago

Probably has to do with the with what sets prepare_atmosphere_assets and queue_atmosphere_bind_group are in. In that case, this bug existed in previous version, it was just never encountered because of coincidences in scheduling.

My guess: PR authors saw prepare_..._assets and thought it was part of the PrepareAssets set, when it looks like it should be in PrepareResources and also saw queue_... and thought it should go in Queue, when it looks like it should be in PrepareBindGroups.

It used to be explicitly ExtractCommands > Prepare > Queue > PhaseSort > Render > Cleanup, but it has changed since then. It is now:

Essentially, Queue and PrepareAssets are now in parallel and so the systems are subject to a race condition like @Neopallium pondered.

Moving the systems to PrepareResources and PrepareBindGroups should fix this issue.

Edit: It would also be appropriate to rename prepare_atmosphere_assets to prepare_atmosphere_resources and queue_atmosphere_bind_group to prepare_atmosphere_bind_group.

VirxEC commented 8 months ago

I discovered that when I build in release mode, this error happens much more often. I agree that it's probably a race condition. This happens on both my laptop and my desktop.

TheDan64 commented 7 months ago

Ran into this as well in both release and debug mode

TheDan64 commented 7 months ago

Wasn't able to repro on any bevy_water examples. Definitely seems like a race condition of some sort

VirxEC commented 7 months ago

I haven't gotten this issue since I recently updated to Bevy 0.13.1 - can anyone else confirm this?

EDIT: nvm, it appears that when it's running on my igpu the error doesn't happen but it did happen when I made it run on my dgpu. Definitely a race condition.