search

djeedai / bevy_hanabi

🎆 Hanabi — a GPU particle system plugin for the Bevy game engine.
Apache License 2.0
962 stars 76 forks source link

Panic in fn extract_effects #289

Closed robinhaecker closed 7 months ago

robinhaecker commented 8 months ago

Crate versions bevy version: 0.13.0 bevy_hanabi version: 0.10.0

I encountered this problem also in some older versions, but I did not notice it when using bevy 0.11.2 and hanabi from git commit 33936ee944913fd6a4ba8bf55116508bc3f7057c, which is somewhere in between version 0.7 and 0.8, so it was probably introduced since then.

Describe the bug I am sporadically encountering a failing unwrap() in https://github.com/djeedai/bevy_hanabi/blob/main/src/render/mod.rs#L1287 causing the application to crash.

Expected behavior There should not be a failing unwrap, but instead any of these two possibilities:

I would like to hear some thoughts on this, as I don't understand if there is some logic supposed to be in place regarding the first point or if there are already any known circumstances that may cause this issue if hanabi is used incorrectly. Alternatively, if my suggestion of softening the unwrap is fine, I am also happy to create a PR with this change.

To Reproduce I am having a hard time providing a minimal example that can reproduce this error, as it happens sporadically after a while (running the game I am developing for maybe 30 minutes) and I don't completely understand the underlying root cause. In general, hanabi seems to work fine, but sometimes this error occurs.

Thanks for any help :-)

djeedai commented 8 months ago

Hi @robinhaecker, thanks for opening this issue.

From a quick glance at the code it seems like CompiledParticleEffect only keeps a weak handle to the asset, to prevent keeping it alive forever. Therefore I'm tempted to say that unwrap() is wrong because we have no guarantee the asset is still loaded. We should properly ignore unloaded effect as you suggested.

djeedai commented 8 months ago

Note that the regression comes from introducing CompiledParticleEffect I think. Previously here we were using the ParticleEffect, which generally has a strong reference to the asset.

robinhaecker commented 8 months ago

Thanks for your input! I have made the changes and once I have confirmed they work, will create a PR with the fix.

robinhaecker commented 8 months ago

I had another, closer look at this, and now think that the main problem is that the error is confusing. My problem was, that I was spawning the Particles in PostUpdate stage (instead of Update), and that caused some problems.

These two changes could be made to prevent hanabi from panicking, but they are hiding the root cause and particle effects simply will not show up on screen then if something is wrong. I think it's probably best to keep everything as it is.

djeedai commented 8 months ago

So I've tried to analyze why there's a weak asset reference here and whether we should keep it. I've also tried hard to write a regression test, but unfortunately this is impossible at the minute due to some constraints with Bevy and winit. In the end, I'm not convinced we should keep that weak handle. And with a strong handle I expect the issue to disappear.

djeedai commented 8 months ago

@robinhaecker could you please try the fix of #306, which uses a strong handle instead? I believe this would fix the issue without adding the inconsistency of having a CompiledParticleEffect reference an unloaded asset. Thanks!

robinhaecker commented 7 months ago

I can't test this fix with the latest master version, as I'm getting a panic (failing unwrap) during startup of my game in: https://github.com/djeedai/bevy_hanabi/blob/main/src/render/mod.rs#L2754 I guess that's a different issue.

djeedai commented 7 months ago

Interesting, I think it's the same issue @NiseVoid reported on Discord but I don't have a repro so not sure how to approach it 🤔