-
-
Notifications
You must be signed in to change notification settings - Fork 922
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
feat: ⚡particles ⚡ #190
feat: ⚡particles ⚡ #190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need some time to take a better look at the code, but I love this feature! Thank you very much!
Thanks for the initial review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I also gonna need a little more time to look give this a better look, but I already left some comments, some additional comments:
Additional to the ImageParticle we could have a SpriteParticle, so we could take a lot of advantages of Flame sprites. Another thing is that the particle system is totally bound to to Flame's component system, so people that does not uses it, will have a hard time to use the Particle system, maybe we could have this outside the Component system from Flame and then create the components that uses it.
Thanks a lot for this PR, this is looking pretty awesome already.
Thanks a lot for your feedback!
Yes! Hundred times yes! That was is the plan, as well as
This was exactly my first line of thought. But i eventually realised that
Which i really liked. The rest of comments will be fixed, thanks again for your review :) |
Awesome!
@av Personally, I totally agree with you, I think that using |
Wasn't aware that this is the case! Then we definitely have to decouple these to
Yet it'd still be possible to use |
Awesome! Looking forward to it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few nits, but the API and overall idea I really, really like. Specially the composability aspect. The one thing I would change though would be to move everything to the particle folder (even the component and mixin), I think that might be more organized (though it's up for debate). Other than that, a huge LGTM from me.
About the separating component or not, I don't have a strong opinion about it, I think it would be nice to have it separated unless it adds too much overhead. In which case folks can easily use a Particle Component by hand inside the Game class without using BaseGame at all. |
@luanpotter I still think we should separate the Components, since BaseGame does a lot more than only calling update and render methods from components, using a component outside the BaseGame structure can lead to a lot of issues, a common one is when a PostitionComponent is used on a Game instance and everything that is rendered after it has a weird position on the screen, which happens because of transformations that this component does to the canvas and who is responsible today for restoring, is the BaseGame. It would agree with not separating, if we would refactor all of our components classes to not have this kind of side effect, which would then enable it to be safely used outside the BaseGame class, but I think that would be too much work at the moment |
fix: doc/examples/particles, added web to .gitignore feat: doc/examples/particles, added more examples, refactor: Particle does not extend Component refactor: Particle subclasses in separate folder refactor: ParticleComponent is now simple container fix: SingleChildParticle, asserts for child existing feat: AnimationParticle for Flame Animation feat: ComponentParticle for Flame Component feat: SpriteParticle for Flame Sprite
Just updated a PR with another iteration as per our previous discussions:
Next steps for us are: @erickzanardo @luanpotter could you please go through added https://github.com/flame-engine/flame/pull/190/files#diff-a0132f3a4aa29237ea1ffc1f534a5ca7 as well as review the chaining API? After that, I think we'll be almost ready to merge this one 💪 |
fix: doc/example/particles/readme, attempt to embed webm preview fix: doc/example/particles better sample for chaining refactor: Particle, dropped duration support
Ok, this version is ready for a final review, also added: From here now will just wait for your final approval and then we're ready to 💥 , 🎉 , 🔴 and any other particle effects in Flame 😸 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found some typos but LGTM. Awesome work on this! Thanks!
Just pushed the last fixes, please take your time for final passes, but feel free to merge any time otherwise! |
Awesome work @av I just noticed that this PR is pointing to master, can you please rebase your it to the develop branch, and also could you add an entry to the CHANGELOG.md with this changes? Be sure to add a With this final two details, this will be ready to be merged! Thanks again for this awesome contribution. |
Done and done! 🎆🎇 |
Merged! Thanks @av |
Sample implementation of particles for Flame 🔥 engine.
The central point of the solution is extreme composability, exactly as with Flutter itself. It means that there are enough basic behaviours to cover majority of the use-cases and the rest is covered by extension points.
Please see the
doc/examples/particles/lib/main.dart
for a proposed particles API and ways to compose them together. Also, please try it locally, it follows the same example format as already established here. The gif above was rendered from running this source.