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

Energy Ball Fixes #363

Conversation

mbalfour-amzn
Copy link
Contributor

This PR fixes #359 .

There are a number of different fixes contained in this PR:

  • When transitioning in and out of game mode in the Editor, order of operation issues can cause PopcornFX to delete the m_emitter pointer before GameEffect is done trying to use it. The IsEffectAlive() checks added to GameEffect detect when this case occurs and prevents the pointer from getting dereferenced and crashing. The asserts are there to continue to notify about the underlying problem, which is that we need to find a more foolproof way to know that PopcornFX has deleted the m_emitter pointer.
  • The root cause of the disappearing energy balls was that if Kill() got called on a particle effect in the first frame, the particle effect would start to delete itself, but never complete the deletion, so subsequent Restart() calls would never revive it. Kill() could sometimes get called in the first frame depending on RPC timing. This replaces Kill() with Terminate() as a workaround to prevent that from happening.
  • The energy ball should call Restart() instead of Start(). Start() will fail if the the particle effect is still finishing its on death animation. Restart will wait for it to finish then restart it.
  • The BallExplosion / LaunchBall events weren't guaranteed to arrive in order, so the logic wasn't always necessarily managing the particle state correctly. By switching to a "BallActive" property and removing "LaunchBall", the order of operations problem goes away.
  • The physics call to disable/enable physics should use the SimulatedBody bus, not the RigidBody bus.
  • There were multiple redundant calls to BallExplosion and the collision checking that could happen during a single launched ball due to bad state management. This adds checks for "is ball active" server-side to avoid the redundant calls.
  • Added @kberg-amzn 's optional debug draws to make it easier to visualize network problems vs particle problems and touched it up a bit so that it draws a single sphere that's the same size as the physics representation instead of a hard-coded size and multiple spheres.

When transitioning in and out of game mode in the Editor, the order of operations can cause PopcornFX to delete the m_emitter pointer before GameEffect is done using it. The IsEffectAlive() checks detect when this case occurs and prevents the pointer from getting dereferenced and crashing. The asserts exist because the root cause should still get fixed as well so that there aren't lifetime management issues.

Signed-off-by: Mike Balfour <[email protected]>
This actually contains a number of fixes:
- The disappearing energy balls were caused if Kill() got called on a particle effect in the first frame, which could happen depending on RPC timing. This replaces Kill() with Terminate() as a workaround to prevent that from happening.
- The BallExplosion / LaunchBall events weren't guaranteed to arrive in order, so the logic wasn't always necessarily managing the particle state correctly. By switching to a "BallActive" property and removing "LaunchBall", the order of operations problem goes away.
- The physics call to disable/enable physics should use the SimulatedBody bus, not the RigidBody bus.
- There were multiple redundant calls to BallExplosion and the collision checking that could happen during a single launched ball due to bad state management.
- Added Karl's optional debug draws to make it easier to visualize network problems vs particle problems.

Signed-off-by: Mike Balfour <[email protected]>
}

#if AZ_TRAIT_CLIENT
void EnergyBallComponent::HandleRPC_BallLaunched([[maybe_unused]] AzNetworking::IConnection* invokingConnection, [[maybe_unused]] const AZ::Vector3& location)
void EnergyBallComponent::OnTick([[maybe_unused]] float deltaTime, [[maybe_unused]] AZ::ScriptTimePoint time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could actually, if you wanted, attach an event handler to the BallActive network property so that when the value changes your callback gets invoked. Since it's just an on/off thing I think it's safe to just say if (active) startfx else killfx although you've clearly run into some hair pulling bugs with pkfx.

Only benefit is if somebody goes crazy and adds 100,000 energy balls to a level the game doesn't have to go through and tick all 100,000 every frame on the client and the server, you'd only process effects changes when the state of BallActive changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh, neat, i didn't know the "on changed" events existed! i'll switch over to using that.

@mbalfour-amzn mbalfour-amzn merged commit 263d4af into o3de:stabilization/2305 Apr 18, 2023
@mbalfour-amzn mbalfour-amzn deleted the mbalfour/energy_ball_fixes branch April 18, 2023 19:16
return;
}

m_collisionCheckEvent.Enqueue(AZ::TimeMs{ 10 }, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that makes a lot more sense..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants