SpriteList still drawing elements after they're removed

[dmitrizagidulin]

Hi ippa,

So after beating my head against the wall on this issue all last night, this is what I found out.

If you add a sprite to a SpriteList, and then remove it, and then call draw() on the sprite list, the removed sprite is still drawn.

What this actually boils down to is:

list = new SpriteList
list.push('a')
list[0]  //=> 'a'
list.splice(0, 1)  
list.length  //=> 0. But!
list[0] //=> 'a', still

You can see a more detailed demo here: https://github.com/dmitrizagidulin/IronSanta/blob/c03fdaeb2ad6c48475e3d082751bc961867ca692/sprite_list_issue.html

And the reason for THAT behavior is, basically, you can't subclass Array, in javascript, as this excellent article explains: http://perfectionkills.com/how-ecmascript-5-still-does-not-allow-to-subclass-an-array/

Meaning, it's not just the SpriteList. You can duplicate this behavior by creating any subclass of Array (ie creating a new object and setting Array as its prototype, as JawsJS does). The resulting "subclass" of Array will not behave properly with regards to various methods, including splice()

So.. would you accept a patch to rewrite SpriteList using composition instead of inheritance? (that is, a SpriteList would have an attribute called 'contents', which would be an actual array, which would store the list of sprites, and delegate its calls to it, etc)

[ippa]

good find! .. actually your demo works just fine in both latest chrome and ie9 but fails (failing as in list[0] contains element1) in firefox 9 (and I assume earlier versions as well).

But of course it needs to work in firefox as well. Do you think some simpler patch is possible to solve this issue, maybe setting list[x] to undefined manually where it matters? Probably a bad solution though.

A patch from someone who spent time digging into the issue sounds very good in my ears :). I would most certainly accept it.. some tests for this would kick ass as well. I wouldn't mind having the internal array be called sprites.. cause what does a SpriteList contain if not sprites :). Also let's try not to break any documented APIs.. but I don't think that should be so hard.

[dmitrizagidulin]

Heh, that was my first thought too -- calling 'delete' so that list[x] is set to undefined.. and then wrapping the draw() invocation on each sprite in a if(this[i]) { this[i].draw() }.

(Also, I was /wondering/ if this might be a browser-specific issue, too! I see)

But.. I mean, if the language protests against subclassing Array, then fine, let's not. Let me get a patch together (with unit tests, definitely), see what you think.

[dmitrizagidulin]

I copy on the internal array being called 'sprites', btw. :)

[dmitrizagidulin]

Oh hey, question. Do all the current unit tests for jaws pass for you, for all browsers? (I'm working on adding some, to deal with the array issue right now). All of the current ones pass for me in Firefox 9, but I'm getting failures in Chrome 16 and IE 9 (on windows)

[ippa]

works 100% for in me in chrome.. in ie9 and ff there was some kind of race condition producing a lot of errors, I think I fixed it now.

I only get one failing test in ie9/ff now, and that's "audio loaded".. not sure why the .wav-file isn't loaded correctly just yet.

[dmitrizagidulin]

Question. In sprite_list.js, in the constructor:

jaws.SpriteList = function SpriteList(options) {
  if( !(this instanceof arguments.callee) ) return new arguments.callee( options );

What is that second line (re arguments/callee etc) for? I think I understand what it's saying, but what situation does it come useful in?

[ippa]

it makes both sprite_list = new SpriteList() and sprite_list = SpriteList() work

.. basically it will make the constructor work without "new". arguments.callee is just a more general way of writing SpriteList.

[dmitrizagidulin]

One more - what's your policy on tabs vs spaces, in contributed code? (tab chars ok, or do you want '2 spaces' tabs, or what?)

[ippa]

2 spaces