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

Fill out ParticleSystem a little #38

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Fortune117
Copy link
Contributor

  • Added a property for the EmissionStopped property on SceneParticles
  • Added setters for control points and a get method for control point positions.
  • fixed names for the Control Point properties being off by 1 (e.g. ControlPoint1 was setting control point 0)
  • Added a PlayEffect method that just recreates the scene object, as otherwise you need to call OnDisabled() then OnEnabled() to achieve the same effect.

/// Useful for particles with intermittent or permanent durations.
/// </summary>
[Property]
public bool EmissionStopped
Copy link
Member

Choose a reason for hiding this comment

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

We should flip this so it's just "Emission", default to true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it 👍

I ran into some situations where setting Emission back to true after disabling it didn't re-enable the particle effect. It seems at some point it's considered finished if the emission is stopped and all the active particles decay.

Not necessarily a problem but it makes me wonder if changing Emission to true should recreate the scene object.

@@ -26,9 +45,9 @@ public class ParticleSystem : BaseComponent, BaseComponent.ExecuteInEditor
}
}

[Property] public GameObject ControlPoint0 { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

We can't do this, it'll break everything that currently uses it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it's not ideal but if there was ever a time to break it it'd be now. This feels like something that'd be potentially confusing for newer users later down the line if it isn't changed.

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.

None yet

2 participants