city41 / breakouts

A collection of JS engine implementations of Breakout
http://jsbreakouts.mattgreer.dev
542 stars 117 forks source link

MelonJS implementation not working on first gen iPad #14

Closed city41 closed 11 years ago

city41 commented 11 years ago

Start the MelonJS Breakout on a first gen iPad running iOS 5. After the title screen loads, tap to start.

expected: goes into level 1 actual: get TypeError: undefined is not a function repeatedly and a blank screen.

melonjs commented 11 years ago

That's weird, what version of iOS is it running, 4.x ? Would you mind as well activating the debug mode in Safari and check the console?

city41 commented 11 years ago

It is running 5.1.1. The error (for MelonJS) is "TypeError: undefined is not a function" repeated infinitely and very rapidly after you tap to start a game. I plan to debug into this sometime this weekend.

melonjs commented 11 years ago

that would be great :) note that there is a un-mimified version of melonJS available here (easier when debugging!): http://www.melonjs.org/download.html

thank you very much.

city41 commented 11 years ago

Ok, I debugged this for a while. Still haven't gotten to the bottom of it, but have good info.

The core problem is it fails to init the paddle, and the paddle object ends up just literally being { z: 3 } and nothing else. This happens in app.addEntity where it calls me.entityPool.newInstanceOf(...), inside of here, it ends up in Class(), where it checks if(!initializing && this.init), this.init is undefined on the iPad, and defined on all other platforms. This basically causes the paddle to never get initted.

I hope to look into this more tonight, but I have some other things I've got to do today.

city41 commented 11 years ago

back at this for the evening (in case you were thinking of looking into it, don't want wasted effort)

melonjs commented 11 years ago

you mean the.init is not the issue ?

city41 commented 11 years ago

No, it is. It looks like Object.extend is not working in general, as the paddle ends up with no methods at all. I am trying to find a decent way to debug Mobile Safari 5. This morning I just used console.log to figure all that out, and holy jeebus it was painful. Desktop Safari 6 and current Chrome will no longer connect to an iOS 5 Mobile Safari instance correctly.

city41 commented 11 years ago

@obiot I have figured out what the problem is, it's pretty nasty :-/ Curious what you think.

Mobile Safari 5 does not have a native Function.bind, so MelonJS uses the shim instead. There is a subtle and key difference between the shim bind and native Function.bind, native Function.bind ignores the binding argument when the new keyword is involved. See bind on MDN: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Function/bind

The value to be passed as the this parameter to the target function when the bound function is called. The value is ignored if the bound function is constructed using the new operator.

So inside newInstanceOf, at about line 4997 in the unminified MelonJS, this gets called:

return new (proto.bind.apply(proto, arguments))();

When the native Function.bind is used, then the constructor is actually called with an instance of the object, this is an instance of Class. But when the shim bind is used, this is the Class constructor function itself.

Here is how I got around this. It's ugly, so not yet proposing this is the real solution. I'm curious what you think:

    function Class() {
        var obj;
        if(!initializing && typeof this === 'function') {
            initializing = true;
            obj = new this();
            initializing = false;
        } else {
            obj = this;
        }

        if (!initializing && obj.init) {
            obj.init.apply(obj, arguments);
            return obj;
        }
    }

With this patch in place, Breakout works just fine on the first gen iPad. It's one ugly patch though.

city41 commented 11 years ago

oh yeah, you can reproduce the behavior in a desktop browser by just forcing the desktop browser to use the shim bind. Just comment out if(!Function.bind) and force the native bind to get overwritten.

melonjs commented 11 years ago

@city41 That's effectively pretty nasty, and I'm amazed you could have debug this to that level just on the iPad, thank you so much for your hard work on this.

I will need to look deeper into it as well, but at the first look I'd say that "my" shim is then probably not correct, as one would expect that the shim one to have the same behavior that the browser implementation, right ?

Now if behavior between shim and browser are inevitable (due to different ECMA version), the way object are created as well in the new instanceOf would probably need some rewrite as well (like avoiding using bind to create new instance).

Other option is to force the native one to be overwritten (dunno if thsi would then cause other side effects when used conjointly with other library/framework).

thank you very much again !

(i'm adding @parasyte , as I'm sure he will like this kind of tricky issue)

city41 commented 11 years ago

You might be able to detect if new is being used inside the shim by looking at this.constructor, but that feels fragile to me. I'll let you guys figure out what you want to do. No problem on figuring it out, I learned something new about JS.

melonjs commented 11 years ago

I won't be able to test anything before tonight, but in the mean time, would you mind testing with a ES5 compliant shim, like the one here : https://github.com/kriskowal/es5-shim/blob/master/es5-shim.js

parasyte commented 11 years ago

@obiot it looks like the "compatibility" implementation on the mozilla developer central link that @city41 provided above is more correct; it returns an anonymous function the same way the melonJS shim does, but also does some extra work to kludge the prototype. The es5-shim effectively does the same thing.

I'm guessing that, the first arg passed to bind, and passing the proper object as this to Function.apply() are pretty much all that's missing.

city41 commented 11 years ago

kriskowal's shim does work, the MDN one probably does too. So cool, looks like a slightly different shim is all that is needed.

melonjs commented 11 years ago

yep, and thank you so much for pointing this out ! :)

(not to mention the time you spent debugging this)

melonjs commented 11 years ago

@city41 I prepared a build of melonJS here that includes the updated bind shim, would you mind give it a try with the iPad1 to be sure it's correctly implemented ? (on my side I have no way to test).

If working, you can also use this build to upgrade the melonJS version. It appears that we have a couple of regression that sucesfully made their way into the last release, and I will release this 0.9.6 shortly to address them.

thank you !

city41 commented 11 years ago

@obiot yup, that did the trick. Thanks! I pushed out the 0.9.6 version of Breakout now (and also added parasyte's pull request). Just a heads up the melonJS version has the occasional hit detection issues for both bricks and the paddle. I can look at it, but probably won't get to it anytime soon.

melonjs commented 11 years ago

great ! thank you very much for the confirmation and feedback :)