Hi calc

Firstly, thanks for looking into these performance issues.

Now, regarding the first bit:

    if ((this._isAndroidStockBrowser && this._clearCanvas) || !(this._useOffScreenTarget && this._renderMode == 'webgl'))    {        this._needsFullRedraw = true;    }

this._isAndroidStockBrowser should be false on Chrome, so a full redraw should not happen in Chrome. It must happen on the Android stock browser because it's buggy and doesn't allow you to reliably clear and redraw only a portion of the canvas without affecting anything else.

Saying that, it may be that the code that detects whether it's the Android stock browser or not isn't 100% right - browser detection is always inherently a bit hacky and needs to be updated quite often to make sure it's always correct. I haven't looked at that in some time, so it may be that in some newer version of Chrome it stopped working. Do you know for a fact that _isAndroidStockBrowser is true on Chrome on your phone?

About the other point:

 

The last thing I looked at was the TextSprite's _updateSize method.   As is, the code creates a new element and adds it to the DOM on every run (including parsing the html for the new elements), and then styles it twice, causing two full page synchronous layouts.  I haven't actually analysed the logic, so I'm assuming that the sequential stylings are required (the vertical align css settings), but I did test two alterations.  First I added the elements to the DOM only once, which gets rid of parsing and related jquery calls during the animation frame, and I also added some properties to the div to allow it become a layout boundary, meaning that subsequent style alterations to its children will spawn a partial layout rather than a full page one.

 

This is a very good idea! I'd really like to add this optimization to the next release of WADE.

The bit of code you quoted in TextSprite is used to determine its height with some degree of precision. However, the idea is that when you have a TextSprite that needs updating quite often (as is the case with a score counter), you probably don't want to do that, and you can disable that behavior entirely by doing this:

// scale the bounding box of the sprite so it's big enough to contain any text that we may want to set in the futuretextSprite.scaleBounds(10, 2);// disable height calculationstextSprite.setFixedSize(true);

Even then, it's a good idea to include that optimization in case users forget to do this, so thanks very much for sharing it :)

Hi Gio, 

Thanks for the reply!  I agree on the text issue, it's not even really an issue for the OM3 game in particular, it's just something I noticed when looking at the mobile problems I was having.  

For the full page render, that's not quite what I'm say ( _isAndroidStockBrowser is indeed false ).  

if ((this._isAndroidStockBrowser && this._clearCanvas) || !(this._useOffScreenTarget && this._renderMode == 'webgl'))    {        this._needsFullRedraw = true;    }

It's the second clause that's evaluating the true pretty much always.  So, say on my phone, or indeed my pc, the whole thing will be

(false && true) || !(false && false) === false || !false === true

Calc, I think you're right. It looks like a fairly big mistake on our part, I think the ! should have been inside the brackets (though I'll need to test it), so I think it should have been:

    if ((this._isAndroidStockBrowser && this._clearCanvas) || (!this._useOffScreenTarget && this._renderMode == 'webgl'))    {        this._needsFullRedraw = true;    }

Thanks for pointing that out, I'll verify that this is actually what was intended and I'll correct it.

Hi,

I've noticed a couple more things in wade.

First I was having trouble with the BasePath feature, which automatically gets stripped out and set when you use wade.init('folder/script.js') to load wade.  The various files/modules seemed somewhat inconsistent as to when paths were being stored with the base path already included (having called getFullPathAndFileName) or without.  For instance animation.js stores the the full version in _imageName during its constructor, then the getImageName method calls getFullPathAndFileName(this._imageName) again, leading to the base path part being duplicated.  

My local fix for this was to remove all getFullPathAndFileName calls outside of wade.js, except for in animation.getImageName() as the method description specifically says it returns the full path.  I changed Sprite.playAnimation to call anim.getRelativeImageName() instead of anim.getImageName().  I also made two changes to wade.js: I removed the getFullPathAndFileName from wade.setImage() and in wade.getFullPathAndFileName() itself I added a test for files starting with '__wade' so that it doesn't add the base path to them.

All this got things working for me with a clear console, but I haven't stepped back to make sure everything else is working correctly, or make sure that the "only use getFullPathAndFileName inside wade.js" concept is actually correct.

The second thing was just something I noticed, although I don't think I've hit an issue with it yet: in the assetLoader you often follow the general logic of 

if(!loaded[name]){  asyncLoad(name, callback)}else{  callback()}

but this means that the callback can be executed before the method returns or not, depending on state.  This can cause tricky bugs to catch, so it's generally recommended that you always defer your callbacks, for instance:

if(!loaded[name]){  asyncLoad(name, callback)}else{  setTimeout(callback, 0); }

so that the callback is never called before the method returns.  

Once again, thanks for reporting these problems.

The second one is a good point and an easy fix.

The first one is also a good point, but less of an easy fix, because we'd need to do some extensive testing to make sure it doesn't break anything, but generally speaking you're right, the basePath stuff is indeed a bit messy and could benefit from a clean up pass.

I'm going to look into it asap - we've had to prioritize a few other things over WADE in the past couple of months but next week we'll be back working on it actively, so these fixes should go in soon.

Thanks for sharing your solution :)

Noticed a few more things when I was developing with this.

1) The name of setMaxScreenSize and setMinScreenSize seems confusing as it turns out they're actually referring to canvas pixels, not the screen

2) If I create a Sprite and use Sprite.setPostition(), the position gets wiped if I subsequently add it to a SceneObject.  Also, even if I use .setPosition() after adding it to a SceneObject, the position will still be wiped if I reorient or resize the screen.  I ended up using setOffset instead.  

3) If I'm using a callback on, for example, a fade draw function, and I remove the Sprite during the callback ( let's say it's faded out of existence now, seems perfectly reasonable to remove it ) then the next Sprite in the list doesn't get displayed ( which generally means it flickers ).  This is because the the callback actually gets executed during the draw loop, and removes an element from the array currently being iterated, causing the next element to drop into the current position and therefore get skipped over when the loop variable gets incremented.  Actually, a setTimeout() call should fix this too.

4) (stopAudio) There was an old bug in IOS where you had to use source.stop(0) instead of just source.stop() or it threw an exception.  It's long fixed, but I did come across it during testing and there doesn't appear to be any downside to using source.stop(0) instead.

Things went pretty smoothly other than that, thanks!  

Hi calc

Thanks for letting us know.

1. Agreed it's a bit confusing. We didn't want to use the word "canvas" there as it was meant to be an abstraction layer (while it does use a canvas, it may not be the case in the future). It probably should have been "viewport" rather than "screen". There is little scope to change it while remaining backwards-compatible, but we should definitely make it clearer in the documentation at least.
2. I understand that can also be a bit confusing. It works that way by design, i.e. a SceneObject is a bit like the parent of the Sprites it contains, and setting the position of the parent should probably override the position of the children. Offsets are the way to go. There is a valid argument for not exposing Sprite.setPosition() then. That may actually be a good idea, but it is also sometimes useful to have that ability. Not sure what the best way of handling this would be.
3. Yes, I had noticed that and it will be fixed in the next release. Thanks for pointing that out.
4. I wasn't aware of that bug - sounds like an easy fix, thanks for suggesting it. It will be included in the next release.

Thanks for the feedback, it is very useful and much appreciated :)