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

improved particle system performance #372

Merged
merged 28 commits into from
Jan 21, 2014

Conversation

insidiator
Copy link
Contributor

I tried the new particle system today and noticed, that it runs a bit slow, especially when using the additive mode.
After some profiling and tinkering around I managed to improve the performance a bit.
There is still much room for improvements, but I think it's a solid start.

The examples show the following fps improvements:

                         before       |      after
blending           | normal | lighter | normal | additive
Fire               |   46   |    26   |   60   |    40
Smoke              |   48   |    27   |   60   |    56
Fountain           |   16   |     7   |   60   |    38
Fireball           |   23   |    12   |   60   |    57
Rain               |   34   |    18   |   60   |    30
Cannon             |   60   |    60   |   60   |    60
Explosion Radial   |   35   |    16   |   60   |    25
Explosion Directed |   60   |    60   |   60   |    60
Waterfall          |   15   |    10   |   60   |    24
Multiples          |   21   |    14   |   60   |    33

The rain and waterfall example look a bit different than before, but I have not looked into the cause yet.

I think the matrix class could be helpful in managing the transformation of the individual particles.
Also I am planning to try a different approach to the additive mode, because it costs too much performance to use the globalCompositionOperation.

@agmcleod
Copy link
Collaborator

Well aren't you just awesome? E-hug!

Seriously though, thanks for the pull request, and showing the numbers :D

Just one small thing, please change the tabs from L476 down to 4 spaces. Just trying to keep things consistent, as a lot of the whitespace is bad in the project as is. Don't want to increase the issue.

@insidiator
Copy link
Contributor Author

Awesome is melonJS. I am just giving back some of the time you save me ;)

I just noticed that the particle effects are dependent on the frame rate, so I will have to change a few more things.
The whitespaces will be fixed with the next commit. I usually use tabs in my projects and don't think about it :)

@agmcleod
Copy link
Collaborator

Sure, no problem :)

@parasyte
Copy link
Collaborator

Using an ObjectContainer for the particles is a good way to improve performance with the additive method, because you would set globalCompositeOperation on the ObjectContainer draw method, draw all of its child objects, and reset globalCompositeOperation at the end.

But ... it's effectively the same code as before your patch. It might be worth optimizing ObjectContainer instead of just the new particle manager?

@insidiator
Copy link
Contributor Author

I don't know what the problem with globalCompositeOperation is (yet). The "lighter" mode does not really look correct to me as it differs from the lighter mode in paint.net and photoshop.
The performance is bad regardless how often it is set, so the problem might lie within the browser (I use firefox 29) or we are just doing something wrong here.

Using an ObjectContainer might be what I finally end up with, but for now I choose to cut out all things that might introduce overhead, in order to see how fast I can get.

With my last commit I use matrix2d for managing the transformations, but I am missing some methods (rotateLocal, setScale) :)
Using setTransform will most probably pose a problem when the viewport moves around.
It might turn out to be necessary to save and restore the context for each particle individually.

@obiot
Copy link
Member

obiot commented Jan 11, 2014

+1 for Jason on the object container.

however, i actually liked the idea of having the particles in the main world container, as we could then play with z order to fake a player sprite walking within the smoke, rain, or fire.

var particle = this._particles[i];
if(!particle.isDead) {
particle.inViewport = (particle.pos.x < viewport.pos.x + viewport.width &&
viewport.pos.x < particle.pos.x + particle.width &&
Copy link
Member

Choose a reason for hiding this comment

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

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 know, I used that method before I inlined it, but there is something really bad going on with it in terms of performance.
The overlaps method of me.rect cost me around 5fps as you are using getters for left, right, top and bottom (https://bugzilla.mozilla.org/show_bug.cgi?id=626021).

@obiot
Copy link
Member

obiot commented Jan 11, 2014

I just went through the patch, and although there is indeed great improvements in terms of performances, I must say that I also agree with Jason, as we lost a lots in terms of readability.....

I was wondering if we could not find a way in between that would more compromise with both approach ? in the mean time the provided examples are probably too much (amount of particles, etc...) for a regular game, and might not need that much performance improvements ?

Jason, what is your opinion about being able to mix regular objects with the particles in the world container (see the use case I explained before) ?

@insidiator at this point, and please don't get me wrong, but I'm not sure this is ready to be merged into the 1.0.0-dev branch, and I would prefer to have it on a separate branch until this patch is finalised and known to be working (when the viewport moving, etc..)

@parasyte
Copy link
Collaborator

@obiot I think it sounds neat in theory, but then again you could just use multiple emitters, each at a different z-index, and get the same visible result. :)

@obiot
Copy link
Member

obiot commented Jan 11, 2014

@parasyte indeed, i did not think about it... ;)

@insidiator
Copy link
Contributor Author

@obiot I know that this patch is not yet ready to be merged back in.
I opened the pull request because issues don't have commits attached and I wanted to get your input early on in order to not have to change everything later. I hope this is not a problem for you :)

I will change the code once more for better readability once I have identified and eliminated all performance killers and fixed the bugs I introduced.

@obiot
Copy link
Member

obiot commented Jan 12, 2014

for sure it's not a problem for me, and I forgot to tell you it in my previous message, but thank you for your time on this one :)

@insidiator
Copy link
Contributor Author

A few more updates. I changed the emitter to be an ObjectContainer and use the addChild, removeChild and draw method. I don't want to use the update method for now, as the isVisible function is quite slow (at least in firefox).

I am also wondering if we could add a flag to ObjectContainer to specify if it should apply its position/transformation to its children, because particles should be positioned independent of the emitter.
As I cannot tell the draw method of ObjectContainer to do this at the moment, I had to set the pos to zero and introduce a new property for the start position of the particles.

If we could set the position of the emitter independently of the particles, it would also be possible to set alwaysUpdate to false and define a bounding box for the emitter.

@insidiator
Copy link
Contributor Author

I tested the examples with chrome and noticed that the texture additive mode works perfectly fine there (no slowdown). Also found the bug report for this problem: https://bugzilla.mozilla.org/show_bug.cgi?id=762973

@parasyte
Copy link
Collaborator

It sounds like it is treating position vectors as screen coordinates. This is the same effect as using floating = true. When used, floating objects do not use isVisible.

@insidiator
Copy link
Contributor Author

I removed the need for rotateLocal in the particles, so the only remaining issue would be how the ParticleContainer is added to the world.
Currently it is added when the reset function of the emitter is called, which means it is added to world in the constructor of the emitter.

I see several possible solutions:

  • let the game programmer handle adding both of them
    • - inconvenient
    • - calls for many new forum posts about how particles are not working
    • - requires updating the particle examples and documentation
  • add a stage/unstage function to emitter which adds the emitter and the container to the world.
    • + gives control over the whole process to the emitter
    • - breaks with the current way of doing things in melonJS
    • - requires updating the particle examples and documentation
  • call an onAdded method in addChild if it is defined (like destroy is called in removeChildNow)
    • + gives control over the whole process to the emitter
    • + stays true to melonJS way of adding things to the world
    • - requires a change to the ObjectContainer.addChild method
  • make a fake update method to add the container when it is first called and then exchange it with the real update method (and change it back to the fake in destroy)
    • + gives control over the whole process to the emitter
    • + stays true to melonJS way of adding things to the world
    • - a really ugly way to handle this problem

@parasyte
Copy link
Collaborator

To be quite honest, I prefer the first one. It isn't much different from the current way things are done, anyway. The only class that adds itself to the scene in its constructor is me.Tween. For everything else, the programmer is responsible for doing this. It is a very well-documented (tutorial, wiki, forum, example code, ...) pattern, and provides flexibility for adding nodes to the scene graph at any depth.

As a side note, I would like to suggest reconsidering the hierarchy in this refactored particle manager; A container is good for grouping particles, defining their boundaries, and transforming them as a single unit. That needs to stay as-is, IMHO. What needs to change is the API. It's illogical to treat the emitter as a top-level object in this hierarchy, when in fact the emitter is just a vector point (and a few other configurable parameters) within the top-level container. Here is my proposal for class hierarchy:

  • me.ParticleManager
    • me.ParticleEmitter
    • me.Renderable - individual particles

Pretty straight-forward. When I (as a game developer) want to use the particle manager, I create a me.ParticleManager (this is a class extending me.ObjectContainer), and add it to the scene:

// Create a particle manager
var pos = new me.Vector2d(20, 20);
var width = 200;
var height = 100;

var mgr = new me.ParticleManager(pos, width, height,
    /* Default emitter definition */
    {
        "image" : me.loader.getImage("explosion"),
        "pos" : new me.Vector2d(0, 0),
        "angleRange" : [ 0, Math.PI * 2 ],
        "speedRange" : [ 3, 7 ],
        "scaleRange" : [ 0.6, 1.0 ],
        "lifeRange" : [ 500, 2000 ],
        "maxParticles" : 300,
        "streamMaxParticles" : 80,
        "gravity" : 0.3
    }
);

// Emitter follows pointer
me.event.subscribe(me.event.MOUSEMOVE, function(e) {
    mgr.emitters[0].pos.set(e.gameWorldX, e.gameWorldY);
});

// Add particle manager to scene
mgr.z = 10;
me.game.world.addChild(mgr);

The manager creates a default emitter (which is added to itself ⚠️) and accessible through its emitters array. The default emitter is armed in the running state, ready to start streaming when the manager is added to the scene. Additional emitters may be added to the manager at will:

mgr.createEmitter({
    "image" : me.loader.getImage("fire"),
    "pos" : new me.Vector2d(mgr.width / 2, mgr.height / 2),
    "angleRange" : [ -0.08, 0.08 ],
    "speedRange" : [ 2, 4 ],
    "scaleRange" : [ 1.3, 0.2 ],
    "lifeRange" : [ 100, 1000 ],
    "maxParticles" : 300,
    "gravity" : -1
}).startStream();

createEmitter adds the emitter to the emitters array, and returns a reference to it, so that it may further be interacted with, for example starting the stream.

While I'm proposing API changes, I really dislike this concept of linear ranges. Using arrays like in the above snippets helps a little bit, but it still sucks a lot. What if I want to change the interpolation functions? Also, why so many numbers? The angle and speed can be expressed as a single magnitude vector. And two vectors can define their ranges.

Another peeve I have is that this code calls for a static image for the particle. What if I want to use an animation? What if I don't want to use a raster image at all? Maybe I'm making a game using the canvas vector drawing API. For these I need the full Renderable.update|Renderable.draw API.

@insidiator
Copy link
Contributor Author

I'll change it to manually adding the container to world then.

I like the change that you propose as I thought about a similar structure myself (adding emitters to a particle system and rendering all particles in that system together), but I would also like to avoid bigger API changes in this ticket, as it should be about performance optimization.

If it is okay with you, I would like to open a new ticket "particle system improvements and features", where we can discuss such ideas and bring this one to a close with the next commit(s). I will also do a new benchmark and post the results here.

@insidiator
Copy link
Contributor Author

I gathered the results of my benchmark in a spreadsheet:
https://docs.google.com/spreadsheet/ccc?key=0AvgoYWTrREK2dEdoYVhnaEM0NkRFRjRiX3ExTmYyaVE&usp=sharing
You can see the improvements for the different browsers and blend modes in the different sheets.

The values where taken from the plugin that I added with my latest commits. Before each test, I reloaded the example page and then waited until all particles have spawned and the values stabilized.
For the Explosion examples I waited for two seconds and then clicked outside the window (which stops the update loop) before taking the values.

The values labeled with particle are the accumulated update and draw times for each individual particle in the old version and for the particle containers in the new version. The overall update and draw time is measured like in the debug panel, but instead of Date.now I used performance.now to get more precise values.

The major performance killers for the particle system where the sorting which occurred when a particle was added to the world and the call to isVisible in the world container (at least in firefox).
For Internet Explorer and Firefox setting the globalCompositionMode to lighter brings a huge performance penalty due to bugs in their implementation.

@parasyte
Copy link
Collaborator

@insidiator Yeah, API discussion in another ticket. That's fair.

@obiot
Copy link
Member

obiot commented Jan 20, 2014

@insidiator @parasyte

between your last exchanges and the new ticket on API changes, i'm very confused about what we shall do here. To be quite honest myself, I also prefer the approach proposed by Jason, i like the particle manager approach (simple and efficient), but also certainly because this is closer to the engine philosophy :p

Also having a renderable as a parameter for the particle would be great, as it would allow us to use anything we want that follows the renderable pattern (and not just a sprite object here).

and finally, regarding the API, so far, and since this is completely new, we are still free to fully change it we want/need to, so no worries there ;)

@insidiator
Copy link
Contributor Author

I also think the proposed API change is a step in the right direction, as it will make the particle system more flexible and easier to handle.
The idea with using renderables instead of a plain image is also something that I would like to have.

But IMHO that are changes which should not happen in this ticket, which is about performance improvements.
From my point of view, this patch is ready to be merged.
The performance is much better than before and the API is nearly the same, so I am happy with it.

Before doing any big changes to the API I would like to coordinate them with @ciangames, because the particle system is originally his contribution and I think he is using it in his game.

So please decide what to do here @agmcleod @parasyte @obiot, because ultimately it is your engine ;)

@parasyte
Copy link
Collaborator

👍

@obiot
Copy link
Member

obiot commented Jan 20, 2014

👍 voted !

@obiot
Copy link
Member

obiot commented Jan 20, 2014

I would add a big thank you to both of you guys @insidiator and @ciangames, and I would like to add that I rather see melonJS as a community based engine, rather than just our engine :P

@aaschmitz
Copy link
Contributor

@insidiator Yes, I'm using the particle in my game, with nice effects :)

@obiot I thank you for melonJS, and the ver. 1.0.0 will be awesome!

@obiot
Copy link
Member

obiot commented Jan 21, 2014

@agmcleod good to go for you too ?

@agmcleod
Copy link
Collaborator

Yep. I'll have to play with it at somepoint, as particle systems are a bit foreign to me. But it looks good. Given @ciangames the original writer also gives it a +1, I think you can merge it :)

@obiot
Copy link
Member

obiot commented Jan 21, 2014

ok, merging this baby then :P

obiot added a commit that referenced this pull request Jan 21, 2014
improved particle system performance (Insidiator)
@obiot obiot merged commit f41a83f into melonjs:1.0.0-dev Jan 21, 2014
@obiot
Copy link
Member

obiot commented Jan 21, 2014

i did not notice that there is a new particleDebugPanel with a flame graph ! i always wanted to add a proper one into the regular debug panel, but never had the time to !

a good merge of both would nice now ;P

@insidiator insidiator deleted the particle-performance branch January 21, 2014 07:00
@insidiator
Copy link
Contributor Author

@obiot proper is a bit of an overstatement. I just copied the one from the debug panel over and changed it a bit to not eat 5ms per frame ;)
If you want, I can open a ticket for the debug panel, as I would love to see some changes to the current one and clean it up a bit when I have time for it (and merge in the particle panel of course).

@obiot
Copy link
Member

obiot commented Jan 21, 2014

that would be great !

see as well #108 and #324 :P

parasyte referenced this pull request Mar 10, 2016
…… …rticles

the issue was also reported here :
https://groups.google.com/forum/m/#!topic/melonjs/dzqIAjMuyGY
and is caused by the onDeactivate function that attempt to remove the
particle container from its ancestor, where (previously) the ancestor
reference was clear by the removeChild function before calling the
`onDeactivateEvent` function :
https://github.com/melonjs/melonJS/blob/master/src/renderable/container.
js#L499
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