I ran into a sporadic crash in the Editor while testing MultiplayerSample which is ultimately due to the fact that PopcornFXIntegration::_Clean() will cause any existing emitter pointers to get deleted, even though other components might still have references to them. Specifically, what seems to be happening is that on the transition from Game Mode back to Editor Mode, PopcornFXIntegration will call _Clean(), and then at some point after that some lingering network packets get processed which attempt to trigger or modify effects spawned via the GameEffect code in MultiplayerSample, which uses the standalone emitter API. Since the GameEffect code didn't know that the emitter pointers were no longer valid, it would call APIs with them and crash.
The GameEffect code has been modified in https://github.com/o3de/o3de-multiplayersample/pull/363 to wrap all the calls in IsEffectAlive() to guard from this happening, but this is a potentially costly workaround - that call is O(N) based on the number of effects - and it would be easy for API users to not know or forget to do. Ideally, we'd have a solution that's a bit more foolproof and cost-effective.
IMO, the solution to the crash could be to do one of the following:
(better) Change the SpawnEffect*() methods to return a shared_ptr or other type of lifetime-abstracting thing instead of a direct pointer so that nothing can hold onto a direct pointer and try to use it outside of its lifetime. For example, it could be a small wrapper class that knows for itself whether or not the pointer is still valid.
(medium) Change PopcornFXIntegration to check for isEnabled inside of all of its API calls and do nothing if it isn't. This is probably a reasonable thing to do anyways, and this would fix the problem in the specific case I observed, since it looked like it was probably an order of operations problem where the PopcornFXIntegration got the signal to exit game mode before some of the other entities had a chance to clean themselves up, so the other entities still tried to do things with the emitter pointers on the way out of game mode. isEnabled was definitely false at the point of the crash that I saw. The main concern I'd have with this fix is that there still might be other use cases in which the emitter pointers get held onto past their lifetimes that I just didn't observe with this one specific crash.
(worse) Change every method in PopcornFXIntegration that takes in an emitter pointer to call IsEffectAlive() before using it. This looks like it would be pretty terrible though, because it would do an O(N) lookup of that pointer on every single API call.
(worse) Add an EBus notification whenever an emitter pointer is destroyed, so that anything that has an emitter pointer can at least have a chance of knowing that it's destroyed. This would work, but it puts a burden on every system that calls SpawnEffect() to listen for this, which would be easy to get wrong.
I ran into a sporadic crash in the Editor while testing MultiplayerSample which is ultimately due to the fact that PopcornFXIntegration::_Clean() will cause any existing emitter pointers to get deleted, even though other components might still have references to them. Specifically, what seems to be happening is that on the transition from Game Mode back to Editor Mode, PopcornFXIntegration will call _Clean(), and then at some point after that some lingering network packets get processed which attempt to trigger or modify effects spawned via the GameEffect code in MultiplayerSample, which uses the standalone emitter API. Since the GameEffect code didn't know that the emitter pointers were no longer valid, it would call APIs with them and crash.
The GameEffect code has been modified in https://github.com/o3de/o3de-multiplayersample/pull/363 to wrap all the calls in IsEffectAlive() to guard from this happening, but this is a potentially costly workaround - that call is O(N) based on the number of effects - and it would be easy for API users to not know or forget to do. Ideally, we'd have a solution that's a bit more foolproof and cost-effective.
IMO, the solution to the crash could be to do one of the following: