new me.ScreenObject(true) is incompatible with me.levelDirector.loadLevel(...)

[parasyte]

Creating a new ScreenObject with addAsObject == true (http://www.melonjs.org/docs/symbols/me.ScreenObject.html#init) which loads a TMX using me.levelDirector.loadLevel(...) (http://www.melonjs.org/docs/symbols/me.levelDirector.html#loadLevel) will inadvertently wipe out the ScreenObject, due to this line: https://github.com/obiot/melonJS/blob/master/src/level/LevelDirector.js#L125

I'm trying to create a simple animated map, but my ScreenObject sub-class's update method can never be called because of this bug.

[parasyte]

This was a lot of debugging, but I got it working flawlessly! The following patch works around this bug by allowing objects to be specified "persistent". A Persistent object will not be removed automatically by me.game.removeAll().

I also decided to make the HUD object persist by default. The way HUD persistence currently works on master is bad when you want to extend the HUD object. (My HUD contains animations. The only sensible way to make it work is subclassing HUD_Object and adding an update() method that will ping the animations every frame.)

Here's a patch:

diff --git a/src/GUI/HUD.js b/src/GUI/HUD.js
index 55bf75c..a2db5ee 100644
--- a/src/GUI/HUD.js
+++ b/src/GUI/HUD.js
@@ -183,6 +183,9 @@
            // this is a little hack to ensure the HUD is always the first draw
            this.z = 999;

+           // ensure me.game.removeAll() will not remove the HUD
+           this.persist = true;
+
        },

        /**
diff --git a/src/core.js b/src/core.js
index 5c2644c..da1c34e 100755
--- a/src/core.js
+++ b/src/core.js
@@ -1170,11 +1170,6 @@ var me = me || {};
            if (api.viewport)
                api.viewport.reset();

-           // re-add the HUD if defined
-           if (api.HUD != null) {
-               api.add(api.HUD);
-           }
-
            // also reset the draw manager
            drawManager.reset();

@@ -1430,16 +1425,18 @@ var me = me || {};
        api.removeAll = function() {
            // inform all object they are about to be deleted
            for (var i = objCount, obj; i--, obj = gameObjects[i];) {
+               if (obj.persist) {
+                   continue;
+               }
                // force object deletion
                obj.autodestroy = true; // do i keep this feature?
                // notify the object
                if(obj.destroy) {
                    obj.destroy();
                }
+               gameObjects.splice(i, 1);
+               objCount--;
            }
-           //empty everything
-           objCount = 0;
-           gameObjects.length = 0;
            // make sure it's empty there as well
            drawManager.flush();
        };
@@ -1665,9 +1665,10 @@ var me = me || {};
         * allowing to override the update & draw function to add specific treatment.
         */

-       init : function(addAsObject) {
+       init : function(addAsObject, persist) {
            this.addAsObject = addAsObject;
            this.visible = (addAsObject === true) || false;
+           this.persist = (persist === true) || false;
            this.rect = new me.Rect(new me.Vector2d(0, 0), 0, 0);
        },

[melonjs]

my first intention was to actually remove this feature, as the same can be achieved using a specific entity, but everybody keeps using it :)

but using a persistent property is a nice and clean fix (better than the one I had in mind), and that would also fix this one (#39) :
https://github.com/obiot/melonJS/issues/39

only remaining issue I see is when switching screen through me.state.change (go back to title screen, or whatever), as the screen itself would then remain in the object pool, but I guess this could be fixed by checking for the screen status (persistent or not) in the me.state.change function and manually remove it if necessary. I'll check all that later :)

[melonjs]

as well at the end of the removeAll function, the array length is force to 0, which might not be correct anymore, if we have persistent object inside the array :

//empty everything
objCount = 0;
gameObjects.length = 0;

[parasyte]

Yes, the patch above removes those lines. :)

Good catch on persisting screen objects through a state change. That could be bad! :D

[parasyte]

Olivier, here is a proposal for this issue:

I was thinking last night maybe a number of different kinds of persistence options is better than a boolean. Say we define the following constants:

// Implemented as bitflags.
me.sys.PERSIST_NEVER  = 0x00000000;
me.sys.PERSIST_STATE  = 0x00000001;
me.sys.PERSIST_LEVEL  = 0x00000002;
me.sys.PERSIST_USER   = 0x00000004;
me.sys.PERSIST_ALWAYS = 0xFFFFFFFF;

• me.sys.PERSIST_NEVER objects are always removed on reset. (Like the current implementation.)
• me.sys.PERSIST_STATE objects are persisted through state changes.
• me.sys.PERSIST_LEVEL objects are only persisted through level changes.
• me.sys.PERSIST_ALWAYS objects always persisted on reset. (Like the patch above.)

Some others could be added as well, if the need arrises. That's what the USER flag is for (see below).

me.game.removeAll() will need to accept a mask argument to decide which objects it should remove. The logic will have to say if ((+obj.persist & mask) === 0). Some examples for removing objects with this method:

• If me.sys.PERSIST_ALWAYS is passed to me.game.removeAll(), it will ONLY remove objects with obj.persist == me.sys.PERSIST_NEVER.
• Likewise, if me.sys.PERSIST_NEVER is passed to me.game.removeAll(), it will remove ALL objects.
• When me.state.change() wants to remove everything except things that persist through state changes, it will pass me.sys.PERSIST_STATE to me.game.removeAll().

Groups of objects can be specified for persistence and removal, in this way.

Setting the persistence flags on objects is just a matter of doing bitwise OR and AND operations on the persist property. My HUD is going to use this.persist = me.sys.PERSIST_LEVEL; to persist through level loads. My debug display object (FPS, object counter, etc.) will have this.persist = me.sys.PERSIST_ALWAYS;. And pretty much everything else will use me.sys.PERSIST_NEVER.

Users could make use of it to remove all objects of a type. My game code might define its own constants:

var c = {
    PERSIST_PLAYER  : me.sys.PERSIST_USER << 0,
    PERSIST_COINS   : me.sys.PERSIST_USER << 1,
    PERSIST_ENEMIES : me.sys.PERSIST_USER << 2,
    PERSIST_NPCS    : me.sys.PERSIST_USER << 3,
    PERSIST_BULLETS : me.sys.PERSIST_USER << 4
};

Now if the player drops a "power bomb", I can remove all enemies by using me.game.removeAll(~c.PERSIST_ENEMIES); Or if I have spawned a hundred bonus coins (think Super Mario Bros. "P" block) I can remove them all when the timer expires using this feature, instead of keeping track of all the bonus coins myself (melonJS is already keeping track of them!) There are many, many other use cases.

How does this sound?

[melonjs]

Hi !

that seems to be a pretty nice idea, and adds a lots of flexibility :)

So for the screen issue, it would then be using me.sys.PERSIST_LEVEL, right ?
the screen itself (when added as an object) would be persistent through levels, but would be removed when change state?

[parasyte]

That's it! I think it would be handy, too. And I could replace the above patch for good if the proposal was implemented. :)

[melonjs]

hehe, I promise I'll look into it tomorrow :)
(here is 12PM already)

[melonjs]

well well well, this has been actually much more complicated than I initially thought, as each time I was doing something, something else was not working... :) But at least it forced me to clean some code :)

For now I just integrated the persist boolean property, as I wanted to be sure it was working for any possible scenario (as discussed before).

How to enable it : just pass a double boolean to the init method (true, true), and voila ! (but I guess that if you want the screen to be visible, you also need it to be persistent, else it won't work, so a second parameter might not be really useful here).

On my side, it's working with most of my test cases, but I would highly appreciate your feedback as well to ensure everything is working 100% and before we proceed with the next step ("advanced persistent mode");

[melonjs]

Forgot to mention, that for a reason I could not find, I could not managed to get the following line to work in the first time :
https://github.com/obiot/melonJS/blob/master/src/core.js#L1990

that's why I ended up adding a "force" parameter to the remove function (i guess it's linked to some lost of context or object reference, but with that defering function, it' making it almost impossible to debug)

[parasyte]

I had similar troubles debugging this one. Anyway I removed my patch and pulled your changes. Everything in my game runs the way I expect, so I say it looks fine from my end.

One comment: I think it's really really useful to make any game object persistent, not just the Screen objects. Some examples are listed above; HUD and debug objects. More importantly for me, I can create a persistent "Chipmunk object" that will do the Chipmunk space stepping without any bad melonJS hooks! It's not going to be a "visible" object, but it will have its update() method called once per frame. Perfect.

Sounds like a good start for a plugin system, don't you think?

[melonjs]

Great ! furthermore I also corrected a couple of things and made some improvements, so all in one it’s a better version !

I did not put a lots of though in this, but yes I could agree as well on extending the persistent property to other objects, as this could be useful. I’ve been looking at your chipmunk code, and one of the thing I really don’t like is the additional setInterval you are using. melonJS already define a main loop and from my vision chipmunk should also rely on that one for synchronization but rather by using a, to be defined, callback in the timer system, or why not a persistent object (Though the first seems to be cleaner to me)

(btw I think I will rename “persist” to “isPersistent” to be more clear, but it’s more like internal cooking for now)

I think that except of couple of tweaks , once you finished with your game, I will freeze the feature set for this release of melonJS. There is a tons of changes in this 0.9.4 and once I’ll be confident enough this is stable version it will be time to make it public and then go towards the 0.9.5 version.

And a plugin system would definitely be great ! we just need to find a clean way to replace portion of melonJS without any hack-ish solution :)

[parasyte]

About the chipmelon hack: by default, the 'sync' property is true, so the setInterval is not used. Instead, this is: https://bitbucket.org/parasyte/lpcgame/src/3768752ed7ca/lib/cm.js#cl-325

Both are really quite poor solutions.

[melonjs]

oh, I didn't notice it :)

Else (just to comment on your blog), honestly I think you should look at the chipmunk things (I mean the plugin), on my side i'm pretty confident the (if this is it) memory leak is not coming from melonJS (though it's possible as well, but in this release I specially took time to chase any possible memory leaks).

also if you disable requestAnimationFrame, you could also consider lowering the fps rate (a default 60 might be to high for a RPG game ??), this might give you more juice during each frame (however you will need to adjust all animation speed).

[ghost]

Yep. The limited bit of profiling I did pointed at Chipmunk-js as the source of memory leaks. It seems to allocate new vectors (cp.v) very often, and may not reuse them. Needs more investigation.

Also I think the fastest animation I use is the blinking eyes. The slowest I can run it with current settings is 30 fps. That might be ideal.

[melonjs]

Norb, any comments on your side, or shall I consider you are ok as well with it ?

[melonjs]

I'm closing this one, as everybody seems to be happy with it (at least so far, no more complains!)

thanks Jason :)