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

Anchor point always 0.5/0.5 even if defined #307

Closed
agmcleod opened this Issue Nov 13, 2013 · 41 comments

Comments

Projects
None yet
4 participants
@agmcleod
Member

agmcleod commented Nov 13, 2013

https://github.com/melonjs/melonJS/blob/master/src/renderable/base.js#L136

Should that not be in the if === null constructor instead? Otherwise just depend that the developer has set it manually? Something I've run into with spine on setting the anchorpoint.

@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 13, 2013

Member

This is another case where the classical inheritance pattern fails us. Actually, this case pinpoints the biggest issue with property-overrides as an inheritance pattern;

Imagine we were to allow something like this:

var MyClass = me.Renderable.extend({
    anchorPoint : new Vector2d(0.5, 1.0),

    // ...
});

This seems like it should be ok. But when you instantiate multiple objects with a class like this, they will all reference the same anchorPoint Vector2d object. Such that all objects will see the changes made to it. And even worse, subclasses also inherit this bad behavior.

It is also not possible to use simple objects for the same reason. So using { x : 0.5, y : 1.0 } in place of new Vector2d(0.5, 1.0) has the same exact problem.

Numbers and Booleans can be used this way, because they cannot be referenced in JavaScript. String laterals are also ok, because they are immutable. But String objects are objects, so also cannot be used this way.

If you have a better recommendation than overriding the constructor and taking care to update object properties after calling the superconstructor, I would be interested. :)

Member

parasyte commented Nov 13, 2013

This is another case where the classical inheritance pattern fails us. Actually, this case pinpoints the biggest issue with property-overrides as an inheritance pattern;

Imagine we were to allow something like this:

var MyClass = me.Renderable.extend({
    anchorPoint : new Vector2d(0.5, 1.0),

    // ...
});

This seems like it should be ok. But when you instantiate multiple objects with a class like this, they will all reference the same anchorPoint Vector2d object. Such that all objects will see the changes made to it. And even worse, subclasses also inherit this bad behavior.

It is also not possible to use simple objects for the same reason. So using { x : 0.5, y : 1.0 } in place of new Vector2d(0.5, 1.0) has the same exact problem.

Numbers and Booleans can be used this way, because they cannot be referenced in JavaScript. String laterals are also ok, because they are immutable. But String objects are objects, so also cannot be used this way.

If you have a better recommendation than overriding the constructor and taking care to update object properties after calling the superconstructor, I would be interested. :)

@agmcleod

This comment has been minimized.

Show comment
Hide comment
@agmcleod

agmcleod Nov 13, 2013

Member

Write it in C?

Yeah i agree that kinda property thing is a pain. It's why i've mostly setup properties for my own classes inside the constructor. Your last sentence is what I'm trying to avoid lol. I need to set the anchorPoint properly before I call the parent constructor, so the me.Spine.Entity constructor is called (it sets up the collision rectangle based on the anchorPoint). I agree we will continue to run into issues, but is there an issue with setting the value to 0.5,0.5 only when it invokes a new Vector2d() ?

Referring to https://github.com/melonjs/melonJS/blob/master/src/renderable/base.js#L134

Member

agmcleod commented Nov 13, 2013

Write it in C?

Yeah i agree that kinda property thing is a pain. It's why i've mostly setup properties for my own classes inside the constructor. Your last sentence is what I'm trying to avoid lol. I need to set the anchorPoint properly before I call the parent constructor, so the me.Spine.Entity constructor is called (it sets up the collision rectangle based on the anchorPoint). I agree we will continue to run into issues, but is there an issue with setting the value to 0.5,0.5 only when it invokes a new Vector2d() ?

Referring to https://github.com/melonjs/melonJS/blob/master/src/renderable/base.js#L134

@agmcleod

This comment has been minimized.

Show comment
Hide comment
@agmcleod

agmcleod Nov 13, 2013

Member

Okay i see exactly what you mean. X is 1 on both outputs.

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8" />
    <title></title>
    <meta name="viewport" content="width=device-width, minimum-scale=1.0" />
    <script>
      Object.extend = function(prop) {
        // _super rename to parent to ease code reading
        var parent = this.prototype;

        // Instantiate a base class (but only create the instance,
        // don't run the init constructor)
        initializing = true;
        var proto = new this();
        initializing = false;

        // Copy the properties over onto the new prototype
        for ( var name in prop) {
          // Check if we're overwriting an existing function
          proto[name] = typeof prop[name] === "function" &&
                  typeof parent[name] === "function" &&
                  fnTest.test(prop[name]) ? (function(name, fn) {
            return function() {
              var tmp = this.parent;

              // Add a new ._super() method that is the same method
              // but on the super-class
              this.parent = parent[name];

              // The method only need to be bound temporarily, so we
              // remove it when we're done executing
              var ret = fn.apply(this, arguments);
              this.parent = tmp;

              return ret;
            };
          })(name, prop[name]) : prop[name];
        }

        // The dummy class constructor
        function Class() {
          if (!initializing && this.init) {
            this.init.apply(this, arguments);
          }
          //return this;
        }
        // Populate our constructed prototype object
        Class.prototype = proto;
        // Enforce the constructor to be what we expect
        Class.constructor = Class;
        // And make this class extendable
        Class.extend = Object.extend;//arguments.callee;

        return Class;
      };

      var me = {};
      me.Vector2d = Object.extend({
        init : function(x, y) {
          this.x = x;
          this.y = y;
        }
      });

      me.Sprite = Object.extend({
        anchorPoint : new me.Vector2d(10, 10),

        init : function() {

        }
      });

      var one = new me.Sprite();
      var two = new me.Sprite();
      one.anchorPoint.x = 1;

      console.log(one.anchorPoint);
      console.log(two.anchorPoint);
    </script>
  </head>
  <body>
  </body>
</html>

Damn it.

Member

agmcleod commented Nov 13, 2013

Okay i see exactly what you mean. X is 1 on both outputs.

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8" />
    <title></title>
    <meta name="viewport" content="width=device-width, minimum-scale=1.0" />
    <script>
      Object.extend = function(prop) {
        // _super rename to parent to ease code reading
        var parent = this.prototype;

        // Instantiate a base class (but only create the instance,
        // don't run the init constructor)
        initializing = true;
        var proto = new this();
        initializing = false;

        // Copy the properties over onto the new prototype
        for ( var name in prop) {
          // Check if we're overwriting an existing function
          proto[name] = typeof prop[name] === "function" &&
                  typeof parent[name] === "function" &&
                  fnTest.test(prop[name]) ? (function(name, fn) {
            return function() {
              var tmp = this.parent;

              // Add a new ._super() method that is the same method
              // but on the super-class
              this.parent = parent[name];

              // The method only need to be bound temporarily, so we
              // remove it when we're done executing
              var ret = fn.apply(this, arguments);
              this.parent = tmp;

              return ret;
            };
          })(name, prop[name]) : prop[name];
        }

        // The dummy class constructor
        function Class() {
          if (!initializing && this.init) {
            this.init.apply(this, arguments);
          }
          //return this;
        }
        // Populate our constructed prototype object
        Class.prototype = proto;
        // Enforce the constructor to be what we expect
        Class.constructor = Class;
        // And make this class extendable
        Class.extend = Object.extend;//arguments.callee;

        return Class;
      };

      var me = {};
      me.Vector2d = Object.extend({
        init : function(x, y) {
          this.x = x;
          this.y = y;
        }
      });

      me.Sprite = Object.extend({
        anchorPoint : new me.Vector2d(10, 10),

        init : function() {

        }
      });

      var one = new me.Sprite();
      var two = new me.Sprite();
      one.anchorPoint.x = 1;

      console.log(one.anchorPoint);
      console.log(two.anchorPoint);
    </script>
  </head>
  <body>
  </body>
</html>

Damn it.

@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 13, 2013

Member

Yes. We have hit this issue many times before:

Member

parasyte commented Nov 13, 2013

Yes. We have hit this issue many times before:

@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 17, 2013

Member

The "maybe-initialize-and-always-reset" pattern mostly showed up in a27f084 - It's a way to reset the vectors for the entity pool. Rather than tearing down the object completely, its composed objects are simply reset to defaults without initiating the GC more than necessary.

I've assigned this ticket to the 0.9.11 Milestone. We have some time to decide how to handle it.

Here are some comments from IRC about this a few days ago:

[16:15.01] <agmcleod> Might be a good idea for us to put together some coding/style guidelines for contributors, including ourselves
[16:15.04] <agmcleod> keeping note of things like that
[16:16.31] <pubjay> I'm wondering if the extend method should just make deep copies of non-function keys, and be done with it...
[16:16.56] <agmcleod> yeah perhaps
[16:17.09] <agmcleod> any pitfalls?
[16:17.17] <agmcleod> Not sure if that would cause us any GC hell
[16:17.30] <pubjay> When it comes across `anchorPoint` which is an instantiation of me.Vector2d ...
[16:18.03] <pubjay> It would have to know that it can deep-copy by calling its .copy() method.
[16:18.11] <agmcleod> right
[16:20.10] <pubjay> That could be by convention, though
[16:20.50] <pubjay> It won't be any harder on the GC than the constructor creating these objects. :)
[16:23.17] <agmcleod> yeah you're probably right

But really, there are two conflicting ideas at play here:

  1. Resetting objects in the entity pool
  2. Instantiating objects using class definition

The first one is probably best solved with a separate method. ScreenObject has an onResetEvent() method, it makes sense for entity pool to use the same pattern.

The second one is a bit trickier, but could be solved directly in Object.extend() using a "smart" deep-copy function that knows when to recursively iterate native JS Objects and Arrays, and when to call a copy() method on melonJS objects. This will at least allow the class definition pattern that I described above: #307 (comment)

@obiot Any thoughts?

Member

parasyte commented Nov 17, 2013

The "maybe-initialize-and-always-reset" pattern mostly showed up in a27f084 - It's a way to reset the vectors for the entity pool. Rather than tearing down the object completely, its composed objects are simply reset to defaults without initiating the GC more than necessary.

I've assigned this ticket to the 0.9.11 Milestone. We have some time to decide how to handle it.

Here are some comments from IRC about this a few days ago:

[16:15.01] <agmcleod> Might be a good idea for us to put together some coding/style guidelines for contributors, including ourselves
[16:15.04] <agmcleod> keeping note of things like that
[16:16.31] <pubjay> I'm wondering if the extend method should just make deep copies of non-function keys, and be done with it...
[16:16.56] <agmcleod> yeah perhaps
[16:17.09] <agmcleod> any pitfalls?
[16:17.17] <agmcleod> Not sure if that would cause us any GC hell
[16:17.30] <pubjay> When it comes across `anchorPoint` which is an instantiation of me.Vector2d ...
[16:18.03] <pubjay> It would have to know that it can deep-copy by calling its .copy() method.
[16:18.11] <agmcleod> right
[16:20.10] <pubjay> That could be by convention, though
[16:20.50] <pubjay> It won't be any harder on the GC than the constructor creating these objects. :)
[16:23.17] <agmcleod> yeah you're probably right

But really, there are two conflicting ideas at play here:

  1. Resetting objects in the entity pool
  2. Instantiating objects using class definition

The first one is probably best solved with a separate method. ScreenObject has an onResetEvent() method, it makes sense for entity pool to use the same pattern.

The second one is a bit trickier, but could be solved directly in Object.extend() using a "smart" deep-copy function that knows when to recursively iterate native JS Objects and Arrays, and when to call a copy() method on melonJS objects. This will at least allow the class definition pattern that I described above: #307 (comment)

@obiot Any thoughts?

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Nov 25, 2013

Member

sorry guys for the late feedback !

from my viewpoint, unless we do a major rewrite of the inheritance mechanism in melonJS, I think that we should just state that we cannot trust property definition in the class itself, as Jason you explained it very well two or three messages earlier.

In the mean time, I think that the mix of the 2 following improvements could greatly help with all this :

  • the global reference issue shoul be fixable by adding the missing deep-copy here before the call to init :
    https://github.com/melonjs/melonJS/blob/master/src/core.js#L364
  • the onResetEvent() could however and effectively be used to at least re-initialize the object default properties when using object pooling feature.
Member

obiot commented Nov 25, 2013

sorry guys for the late feedback !

from my viewpoint, unless we do a major rewrite of the inheritance mechanism in melonJS, I think that we should just state that we cannot trust property definition in the class itself, as Jason you explained it very well two or three messages earlier.

In the mean time, I think that the mix of the 2 following improvements could greatly help with all this :

  • the global reference issue shoul be fixable by adding the missing deep-copy here before the call to init :
    https://github.com/melonjs/melonJS/blob/master/src/core.js#L364
  • the onResetEvent() could however and effectively be used to at least re-initialize the object default properties when using object pooling feature.
@agmcleod

This comment has been minimized.

Show comment
Hide comment
@agmcleod

agmcleod Nov 25, 2013

Member

There an easy way for point 2 though? I suppose we'd have a "defaults" hash or something that could be referred to for each object, but no doubt that would have to be constructed. Personally, I like the solution of keeping the properties initialized in a constructor, much like how you would do it in most static-typed languages. At least it's how I do it :)

Member

agmcleod commented Nov 25, 2013

There an easy way for point 2 though? I suppose we'd have a "defaults" hash or something that could be referred to for each object, but no doubt that would have to be constructed. Personally, I like the solution of keeping the properties initialized in a constructor, much like how you would do it in most static-typed languages. At least it's how I do it :)

@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 25, 2013

Member

})(name, prop[name]) : prop[name];

... : deepcopy(prop[name]) ...

Member

parasyte commented Nov 25, 2013

})(name, prop[name]) : prop[name];

... : deepcopy(prop[name]) ...

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Nov 26, 2013

Member

after spending one hour on stackoverflow, here is what I think is a mix of the best of the best on the topic :

 function deepcopy( obj ) {

    if (null == obj || "object" != typeof obj) {
        return obj;
    } 

    // Array copy
    if( obj instanceof Array ) {
        var copy = [];
        for( var i = 0, l = obj.length; i < l; i++) {
            copy[i] = deepcopy(obj[i]);
        }
        return copy;
    }

    // Date copy
    if (obj instanceof Date) {
        var copy = new Date();
        copy.setTime(obj.getTime());
        return copy;
    }

    // else instanceof Object
    var copy = {};
    for( var attr in obj ) {
        if (obj.hasOwnProperty(attr)) copy[attr] = deepcopy(obj[attr]);
    }
    return copy;
}

i have not tried it, but the code is simple enough, and should basically do the trick ;)

who wants to give it a try ?? :):):)

Member

obiot commented Nov 26, 2013

after spending one hour on stackoverflow, here is what I think is a mix of the best of the best on the topic :

 function deepcopy( obj ) {

    if (null == obj || "object" != typeof obj) {
        return obj;
    } 

    // Array copy
    if( obj instanceof Array ) {
        var copy = [];
        for( var i = 0, l = obj.length; i < l; i++) {
            copy[i] = deepcopy(obj[i]);
        }
        return copy;
    }

    // Date copy
    if (obj instanceof Date) {
        var copy = new Date();
        copy.setTime(obj.getTime());
        return copy;
    }

    // else instanceof Object
    var copy = {};
    for( var attr in obj ) {
        if (obj.hasOwnProperty(attr)) copy[attr] = deepcopy(obj[attr]);
    }
    return copy;
}

i have not tried it, but the code is simple enough, and should basically do the trick ;)

who wants to give it a try ?? :):):)

@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 26, 2013

Member

@obiot Looks good for normal JavaScript objects, but will not work well with melonJS objects! Try for example:

var orig = new me.Vector2d(50, 50);
var copy = deepcopy(orig);

console.log(orig.add); // function (a){return this.x+=a.x,this.y+=a.y,this}
console.log(orig instanceof me.Vector2d); // true

console.log(copy.add); // undefined
console.log(copy instanceof me.Vector2d); // false

We would need to use Eg.:

else if (obj instanceof me.Vector2d || obj instanceof me.Shape) {
    return obj.copy();
}
Member

parasyte commented Nov 26, 2013

@obiot Looks good for normal JavaScript objects, but will not work well with melonJS objects! Try for example:

var orig = new me.Vector2d(50, 50);
var copy = deepcopy(orig);

console.log(orig.add); // function (a){return this.x+=a.x,this.y+=a.y,this}
console.log(orig instanceof me.Vector2d); // true

console.log(copy.add); // undefined
console.log(copy instanceof me.Vector2d); // false

We would need to use Eg.:

else if (obj instanceof me.Vector2d || obj instanceof me.Shape) {
    return obj.copy();
}
@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Nov 26, 2013

Member

hmm...

what about removing the test on hasOwnProperty for the last case then :

 // else instanceof Object
    var copy = {};
    for( var attr in obj ) {
        copy[attr] = deepcopy(obj[attr]);
    }
    return copy;
Member

obiot commented Nov 26, 2013

hmm...

what about removing the test on hasOwnProperty for the last case then :

 // else instanceof Object
    var copy = {};
    for( var attr in obj ) {
        copy[attr] = deepcopy(obj[attr]);
    }
    return copy;
@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 26, 2013

Member

Functionally, that works, but the instanceof operator will fail. I'm not sure if it will break anything.

Member

parasyte commented Nov 26, 2013

Functionally, that works, but the instanceof operator will fail. I'm not sure if it will break anything.

obiot added a commit that referenced this issue Nov 26, 2013

[#307] fixed deep copy of class defined objects references
also added more tests in the provided example
@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Nov 26, 2013

Member

with this last commit, and based on my test and understanding, this is now fully working :

as the below test :
https://github.com/melonjs/melonJS/blob/deepcopy/examples/deepcopy/main.js

correctly generates the following output :

Object {x: 0.5, y: 0.5, init: function, set: function, setZero: function…}
true 
true 
Object {x: 1.5, y: 1.5, init: function, set: function, setZero: function…}
true 
true 
true 
1 !== 3 
true 
true
Member

obiot commented Nov 26, 2013

with this last commit, and based on my test and understanding, this is now fully working :

as the below test :
https://github.com/melonjs/melonJS/blob/deepcopy/examples/deepcopy/main.js

correctly generates the following output :

Object {x: 0.5, y: 0.5, init: function, set: function, setZero: function…}
true 
true 
Object {x: 1.5, y: 1.5, init: function, set: function, setZero: function…}
true 
true 
true 
1 !== 3 
true 
true
@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 26, 2013

Member

Weird. The last three fail for me.

3 !== 3 main.js:42
false main.js:43
false main.js:44
Member

parasyte commented Nov 26, 2013

Weird. The last three fail for me.

3 !== 3 main.js:42
false main.js:43
false main.js:44
@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Nov 26, 2013

Member

sorry but I need to ask : did you recompile before testing ? (and pulled the last version)

Member

obiot commented Nov 26, 2013

sorry but I need to ask : did you recompile before testing ? (and pulled the last version)

@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 26, 2013

Member

HA! I did not. ;) That also explains why my other tests unexpectedly succeeded...

// Expected
console.log(vec1.a instanceof me.Vector2d); // true
console.log(vec2.a instanceof me.Vector2d); // true

Both return false.

Member

parasyte commented Nov 26, 2013

HA! I did not. ;) That also explains why my other tests unexpectedly succeeded...

// Expected
console.log(vec1.a instanceof me.Vector2d); // true
console.log(vec2.a instanceof me.Vector2d); // true

Both return false.

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Nov 26, 2013

Member

just wondering, now that property deep copy is working as expected, what is the status of this one, in regards to the initial ticket content ?

Member

obiot commented Nov 26, 2013

just wondering, now that property deep copy is working as expected, what is the status of this one, in regards to the initial ticket content ?

@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 26, 2013

Member

@obiot Part one is solved by deepcopy; code like #307 (comment) is acceptable now. But the original report was about properties like anchorPoint and pos getting wiped out by the constructors.

Here's a test to try out:

var AnchoredRenderable = me.Renderable.extend({
    anchorPoint : new me.Vector2d(0.5, 1.0)
});

it("is anchored at 0.5,1.0", function () {
    var obj = new AnchoredRenderable(
        new me.Vector2d(),
        20,
        20
    );
    expect(obj.anchorPoint.x).toEqual(0.5);
    expect(obj.anchorPoint.y).toEqual(1.0);
});
Member

parasyte commented Nov 26, 2013

@obiot Part one is solved by deepcopy; code like #307 (comment) is acceptable now. But the original report was about properties like anchorPoint and pos getting wiped out by the constructors.

Here's a test to try out:

var AnchoredRenderable = me.Renderable.extend({
    anchorPoint : new me.Vector2d(0.5, 1.0)
});

it("is anchored at 0.5,1.0", function () {
    var obj = new AnchoredRenderable(
        new me.Vector2d(),
        20,
        20
    );
    expect(obj.anchorPoint.x).toEqual(0.5);
    expect(obj.anchorPoint.y).toEqual(1.0);
});
@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Nov 28, 2013

Member

so my last thoughts on this one :

now that we can properly do something like this in melonJS :

me.Rect = Object.extend(
    /** @scope me.Rect.prototype */ {

        /**
         * position of the Rectange
         * @public
         * @type me.Vector2d
         * @name pos
         * @memberOf me.Rect
         */
        pos : new me.Vector2d(),
    // ...
});

we can remove all properties object initialization from the constructor, so typically this kind of line :

    /** @ignore */
    init : function(v, w, h) {
       if (this.pos === null) { 
            this.pos = new me.Vector2d();
       }
       .....
    }

With this code removed, we can both defined the default value we want in the default object, while letting full liberty to anyone to overwrite it (either by extending the object and redefining the porperty, or by just changing the default value in the constructor).

the only cons I see is when using the entity pool mechanism, as we don't provide anymore any built-in way to restore objects to their default value.

In my opinion, the last point is acceptable, as anyway it's impossible to provide such a default "restorable" state for all object easily, and since it's the user choice to use entity pooling, it's also kind of it's responsibility to ensure objects are restored when required to their default state?

now as a compromised we could add a empty function onRecyling (with a better name) that would be call each time an object is re-instantiated from the pool ? so that user are then free to do whatever they need from inside ?

Member

obiot commented Nov 28, 2013

so my last thoughts on this one :

now that we can properly do something like this in melonJS :

me.Rect = Object.extend(
    /** @scope me.Rect.prototype */ {

        /**
         * position of the Rectange
         * @public
         * @type me.Vector2d
         * @name pos
         * @memberOf me.Rect
         */
        pos : new me.Vector2d(),
    // ...
});

we can remove all properties object initialization from the constructor, so typically this kind of line :

    /** @ignore */
    init : function(v, w, h) {
       if (this.pos === null) { 
            this.pos = new me.Vector2d();
       }
       .....
    }

With this code removed, we can both defined the default value we want in the default object, while letting full liberty to anyone to overwrite it (either by extending the object and redefining the porperty, or by just changing the default value in the constructor).

the only cons I see is when using the entity pool mechanism, as we don't provide anymore any built-in way to restore objects to their default value.

In my opinion, the last point is acceptable, as anyway it's impossible to provide such a default "restorable" state for all object easily, and since it's the user choice to use entity pooling, it's also kind of it's responsibility to ensure objects are restored when required to their default state?

now as a compromised we could add a empty function onRecyling (with a better name) that would be call each time an object is re-instantiated from the pool ? so that user are then free to do whatever they need from inside ?

@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Nov 28, 2013

Member

@obiot Good points. I'm ok with using the onResetEvent name for this kind of callback, but I really think it is necessary; not just a nice feature. I was thinking about it just this morning. You remember the old HUD_Object class that had a reset() method for resetting to the default value? It's a useful pattern in a lot of cases! I would like to see a callback like onResetEvent available for any object that can be reset to a default value. My thoughts went something like this:

/** me.Vector2d */
init : function (x, y) {
    this._defaults = arguments;
    this.onResetEvent();
},

onResetEvent : function () {
    this.x = this._defaults[0];
    this.y = this._defaults[1];
}
/** me.ObjectEntity */
onResetEvent : function () {
    this.pos.onResetEvent();
    // ...
}
Member

parasyte commented Nov 28, 2013

@obiot Good points. I'm ok with using the onResetEvent name for this kind of callback, but I really think it is necessary; not just a nice feature. I was thinking about it just this morning. You remember the old HUD_Object class that had a reset() method for resetting to the default value? It's a useful pattern in a lot of cases! I would like to see a callback like onResetEvent available for any object that can be reset to a default value. My thoughts went something like this:

/** me.Vector2d */
init : function (x, y) {
    this._defaults = arguments;
    this.onResetEvent();
},

onResetEvent : function () {
    this.x = this._defaults[0];
    this.y = this._defaults[1];
}
/** me.ObjectEntity */
onResetEvent : function () {
    this.pos.onResetEvent();
    // ...
}
@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Nov 28, 2013

Member

I would be more enclined then to normalize the onResetEvent callback and do something like this :

me.Rect = Object.extend(
    /** @scope me.Rect.prototype */ {

        /**
         * position of the Rectange
         * @public
         * @type me.Vector2d
         * @name pos
         * @memberOf me.Rect
         */
        pos : new me.Vector2d(0, 0),
    // ...


    /** @ignore */
    init : function(v, w, h) {
       ....
    },

    onResetEvent : function () {
        this.pos.set(0.0);
        ...
    }

});

My concern being that we would then have to maintain for each object a map of default value for each properties.

Note that with this approach, people modifying the default value of a property will still have to extend the onResetEvent function to take in account their default value.

Member

obiot commented Nov 28, 2013

I would be more enclined then to normalize the onResetEvent callback and do something like this :

me.Rect = Object.extend(
    /** @scope me.Rect.prototype */ {

        /**
         * position of the Rectange
         * @public
         * @type me.Vector2d
         * @name pos
         * @memberOf me.Rect
         */
        pos : new me.Vector2d(0, 0),
    // ...


    /** @ignore */
    init : function(v, w, h) {
       ....
    },

    onResetEvent : function () {
        this.pos.set(0.0);
        ...
    }

});

My concern being that we would then have to maintain for each object a map of default value for each properties.

Note that with this approach, people modifying the default value of a property will still have to extend the onResetEvent function to take in account their default value.

obiot added a commit that referenced this issue Nov 28, 2013

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Dec 11, 2013

Member

Hi guys.

I don't know what you think but I propose to move the rest of this ticket to the 1.0.0 branch, as I'm afraid that rewriting (moving/changing the way properties are declared/inherited) in the current object definition would too much impact (i'm thinking potential regression) the current branch.

the deep object property feature is already a very good additition for the 0.9.11 branch, but unless you have any specific object you do want to improve, i really think it's better to wait for 1.0.0

Member

obiot commented Dec 11, 2013

Hi guys.

I don't know what you think but I propose to move the rest of this ticket to the 1.0.0 branch, as I'm afraid that rewriting (moving/changing the way properties are declared/inherited) in the current object definition would too much impact (i'm thinking potential regression) the current branch.

the deep object property feature is already a very good additition for the 0.9.11 branch, but unless you have any specific object you do want to improve, i really think it's better to wait for 1.0.0

@parasyte

This comment has been minimized.

Show comment
Hide comment
@parasyte

parasyte Dec 11, 2013

Member

👍

Member

parasyte commented Dec 11, 2013

👍

@agmcleod

This comment has been minimized.

Show comment
Hide comment
@agmcleod

agmcleod Dec 11, 2013

Member

Yeah I agree.

Member

agmcleod commented Dec 11, 2013

Yeah I agree.

obiot referenced this issue Feb 20, 2014

@obiot obiot modified the milestones: 1.1.0, 1.0.0 Mar 4, 2014

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Mar 4, 2014

Member

same here : 1.1.0

Member

obiot commented Mar 4, 2014

same here : 1.1.0

@agmcleod

This comment has been minimized.

Show comment
Hide comment
@agmcleod

agmcleod Mar 25, 2014

Member

Given ticket #430, i think we can close this one.

Member

agmcleod commented Mar 25, 2014

Given ticket #430, i think we can close this one.

@agmcleod agmcleod closed this Mar 25, 2014

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Mar 25, 2014

Member

we should probably change back the test in the corresponding jasmine test unit .

btw, as part of ticket #430, was the John Resig inheritance test unit also updated with Jay Inheritance ? :):):)

Member

obiot commented Mar 25, 2014

we should probably change back the test in the corresponding jasmine test unit .

btw, as part of ticket #430, was the John Resig inheritance test unit also updated with Jay Inheritance ? :):):)

@agmcleod

This comment has been minimized.

Show comment
Hide comment
@agmcleod

agmcleod Mar 25, 2014

Member

Nope, definitely was not lol. I'll give the tests a run tomorrow :)

Member

agmcleod commented Mar 25, 2014

Nope, definitely was not lol. I'll give the tests a run tomorrow :)

@pierrebeitz

This comment has been minimized.

Show comment
Hide comment
@pierrebeitz

pierrebeitz Jun 11, 2014

Hi, i'm pretty new to MelonJS and i got an issue regarding Internet Explorer <11. I shouldn't be the only one, since the majority of games in the gallery-section can't be played with IE9, IE10.

I tinkered a little with MelonJS v1.0.1 and it seems that removing the deep-copy-functionality in core.js that was added in a4f8acd resolves my issues. It guess it has to do with the handling of proto vs. prototype and the setPrototypeOf-"polyfill" in older IEs.

So for now i claim, that the mentioned commit broke IE-support, but maybe i'm just using MelonJS the wrong way or did not read the docs properly, so please share your thoughts or knowledge with me :)

pierrebeitz commented Jun 11, 2014

Hi, i'm pretty new to MelonJS and i got an issue regarding Internet Explorer <11. I shouldn't be the only one, since the majority of games in the gallery-section can't be played with IE9, IE10.

I tinkered a little with MelonJS v1.0.1 and it seems that removing the deep-copy-functionality in core.js that was added in a4f8acd resolves my issues. It guess it has to do with the handling of proto vs. prototype and the setPrototypeOf-"polyfill" in older IEs.

So for now i claim, that the mentioned commit broke IE-support, but maybe i'm just using MelonJS the wrong way or did not read the docs properly, so please share your thoughts or knowledge with me :)

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Jun 12, 2014

Member

Hi @pierrebeitz just to be sure we are on the same page :

which branch are you using, master or the 1.0.x one ?
also can point out which games from the gallery are not playable exactly under IE <11 ?

thanks!

(re-opening this one for now as well, for better visibility)

Member

obiot commented Jun 12, 2014

Hi @pierrebeitz just to be sure we are on the same page :

which branch are you using, master or the 1.0.x one ?
also can point out which games from the gallery are not playable exactly under IE <11 ?

thanks!

(re-opening this one for now as well, for better visibility)

@obiot obiot reopened this Jun 12, 2014

@pierrebeitz

This comment has been minimized.

Show comment
Hide comment
@pierrebeitz

pierrebeitz Jun 16, 2014

Hi @obiot!
I was using 1.0.x and almost none of the games are working. For example the first on the gallery page ("WWW by Alexandre Dantas").
It seems that the inheritance-system does not work properly since IE9/10 dont support setPrototypeOf() and do not handle methods (and attributes) added to proto as prototype-methods but as methods within a "normal" attribute called proto. So the little "polyfill" for setPrototypeOf does not work.

I fixed my problem by manually copying the prototype-methods for every call of setPrototypeOf. That does not sound performant to me, but since the game is not very complicated it does not matter in my case.

I did not try to rewrite my game to run with the master-branch. Maybe the Problem is fixed there, since you switched to another inheritance-system, but still: the current production-version (1.0.1) seems broken to me. (but now i see that this might not be the right issue for my concern)

pierrebeitz commented Jun 16, 2014

Hi @obiot!
I was using 1.0.x and almost none of the games are working. For example the first on the gallery page ("WWW by Alexandre Dantas").
It seems that the inheritance-system does not work properly since IE9/10 dont support setPrototypeOf() and do not handle methods (and attributes) added to proto as prototype-methods but as methods within a "normal" attribute called proto. So the little "polyfill" for setPrototypeOf does not work.

I fixed my problem by manually copying the prototype-methods for every call of setPrototypeOf. That does not sound performant to me, but since the game is not very complicated it does not matter in my case.

I did not try to rewrite my game to run with the master-branch. Maybe the Problem is fixed there, since you switched to another inheritance-system, but still: the current production-version (1.0.1) seems broken to me. (but now i see that this might not be the right issue for my concern)

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Jun 17, 2014

Member

indeed, I did not noticed it, but that's a silly regression from when we actually introduced the deep copy mechanism for JR's Inheritance.

In the 1.1.x branch, deepcopy has been removed for the moment (as it greatly improve performances) and we have introduced new recommendation for object definition, but that would be nice to fix the function anyway.

there is workaround here :
http://stackoverflow.com/questions/22500685/ie9-does-not-recognize-prototype-function
might worth trying, but of course extending the test to all IE9+ versions

Object.setPrototypeOf = Object.setPrototypeOf || function (obj, proto) {
    if (!isIE9()) {
        obj.__proto__ = proto;
    } else {
        /** IE9 fix - copy object methods from the protype to the new object **/
        for (var prop in proto) {
            obj[prop] = proto[prop];
        }
    }

    return obj;
};

var isIE9 = function() {
    return navigator.appVersion.indexOf("MSIE 9") > 0;
};

@parasyte any thoughts on this ?

Member

obiot commented Jun 17, 2014

indeed, I did not noticed it, but that's a silly regression from when we actually introduced the deep copy mechanism for JR's Inheritance.

In the 1.1.x branch, deepcopy has been removed for the moment (as it greatly improve performances) and we have introduced new recommendation for object definition, but that would be nice to fix the function anyway.

there is workaround here :
http://stackoverflow.com/questions/22500685/ie9-does-not-recognize-prototype-function
might worth trying, but of course extending the test to all IE9+ versions

Object.setPrototypeOf = Object.setPrototypeOf || function (obj, proto) {
    if (!isIE9()) {
        obj.__proto__ = proto;
    } else {
        /** IE9 fix - copy object methods from the protype to the new object **/
        for (var prop in proto) {
            obj[prop] = proto[prop];
        }
    }

    return obj;
};

var isIE9 = function() {
    return navigator.appVersion.indexOf("MSIE 9") > 0;
};

@parasyte any thoughts on this ?

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Jun 17, 2014

Member

and another polyfill here :
http://webreflection.blogspot.nl/2013/05/objectsetprototypeofo-proto-is-in-es6.html

/*jslint devel: true, indent: 2 */
// 15.2.3.2
if (!Object.setPrototypeOf) {
  Object.setPrototypeOf = (function(Object, magic) {
    'use strict';
    var set;
    function checkArgs(O, proto) {
      if (typeof O !== 'object' || O === null) {
        throw new TypeError('can not set prototype on a non-object');
      }
      if (typeof proto !== 'object' && proto !== null) {
        throw new TypeError('can only set prototype to an object or null');
      }
    }
    function setPrototypeOf(O, proto) {
      checkArgs(O, proto);
      set.call(O, proto);
      return O;
    }
    try {
      // this works already in Firefox and Safari
      set = Object.getOwnPropertyDescriptor(Object.prototype, magic).set;
      set.call({}, null);
    } catch (o_O) {
      if (Object.prototype !== {}[magic]) {
        // IE < 11 cannot be shimmed
        return;
      }
      // probably Chrome or some old Mobile stock browser
      set = function(proto) {
        this[magic] = proto;
      };
      // please note that this will **not** work
      // in those browsers that do not inherit
      // __proto__ by mistake from Object.prototype
      // in these cases we should probably throw an error
      // or at least be informed about the issue
      setPrototypeOf.polyfill = setPrototypeOf(
        setPrototypeOf({}, null),
        Object.prototype
      ) instanceof Object;
      // setPrototypeOf.polyfill === true means it works as meant
      // setPrototypeOf.polyfill === false means it's not 100% reliable
      // setPrototypeOf.polyfill === undefined
      // or
      // setPrototypeOf.polyfill ==  null means it's not a polyfill
      // which means it works as expected
      // we can even delete Object.prototype.__proto__;
    }
    return setPrototypeOf;
  }(Object, '__proto__'));
}

or not....

if (Object.prototype !== {}[magic]) {
   // IE < 11 cannot be shimmed
  return;
}
Member

obiot commented Jun 17, 2014

and another polyfill here :
http://webreflection.blogspot.nl/2013/05/objectsetprototypeofo-proto-is-in-es6.html

/*jslint devel: true, indent: 2 */
// 15.2.3.2
if (!Object.setPrototypeOf) {
  Object.setPrototypeOf = (function(Object, magic) {
    'use strict';
    var set;
    function checkArgs(O, proto) {
      if (typeof O !== 'object' || O === null) {
        throw new TypeError('can not set prototype on a non-object');
      }
      if (typeof proto !== 'object' && proto !== null) {
        throw new TypeError('can only set prototype to an object or null');
      }
    }
    function setPrototypeOf(O, proto) {
      checkArgs(O, proto);
      set.call(O, proto);
      return O;
    }
    try {
      // this works already in Firefox and Safari
      set = Object.getOwnPropertyDescriptor(Object.prototype, magic).set;
      set.call({}, null);
    } catch (o_O) {
      if (Object.prototype !== {}[magic]) {
        // IE < 11 cannot be shimmed
        return;
      }
      // probably Chrome or some old Mobile stock browser
      set = function(proto) {
        this[magic] = proto;
      };
      // please note that this will **not** work
      // in those browsers that do not inherit
      // __proto__ by mistake from Object.prototype
      // in these cases we should probably throw an error
      // or at least be informed about the issue
      setPrototypeOf.polyfill = setPrototypeOf(
        setPrototypeOf({}, null),
        Object.prototype
      ) instanceof Object;
      // setPrototypeOf.polyfill === true means it works as meant
      // setPrototypeOf.polyfill === false means it's not 100% reliable
      // setPrototypeOf.polyfill === undefined
      // or
      // setPrototypeOf.polyfill ==  null means it's not a polyfill
      // which means it works as expected
      // we can even delete Object.prototype.__proto__;
    }
    return setPrototypeOf;
  }(Object, '__proto__'));
}

or not....

if (Object.prototype !== {}[magic]) {
   // IE < 11 cannot be shimmed
  return;
}
@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Jun 17, 2014

Member

@parasyte
to fix the 1.0.x branch, we could also modify the deecopy function to not use setPrototypeOf ?
(I don't exactly remember why we had to add it)

Member

obiot commented Jun 17, 2014

@parasyte
to fix the 1.0.x branch, we could also modify the deecopy function to not use setPrototypeOf ?
(I don't exactly remember why we had to add it)

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Jun 17, 2014

Member

one additional note :
__proto__ that is used by our small polyfill is supported in IE11 :
http://msdn.microsoft.com/en-us/library/ie/dn342818(v=vs.94).aspx

"Supported in the Internet Explorer 11 standards document mode. Also supported in Store apps (Windows 8.1 and Windows Phone 8.1). See Version Information. Not supported in the following document modes: Quirks, Internet Explorer 6 standards, Internet Explorer 7 standards, Internet Explorer 8 standards, Internet Explorer 9 standards, Internet Explorer 10 standards. Not supported in Windows 8."

so since it's supported on IE11 and W8 based store apps, does it really worth it to spend additional time on this ?

Member

obiot commented Jun 17, 2014

one additional note :
__proto__ that is used by our small polyfill is supported in IE11 :
http://msdn.microsoft.com/en-us/library/ie/dn342818(v=vs.94).aspx

"Supported in the Internet Explorer 11 standards document mode. Also supported in Store apps (Windows 8.1 and Windows Phone 8.1). See Version Information. Not supported in the following document modes: Quirks, Internet Explorer 6 standards, Internet Explorer 7 standards, Internet Explorer 8 standards, Internet Explorer 9 standards, Internet Explorer 10 standards. Not supported in Windows 8."

so since it's supported on IE11 and W8 based store apps, does it really worth it to spend additional time on this ?

@pierrebeitz

This comment has been minimized.

Show comment
Hide comment
@pierrebeitz

pierrebeitz Jun 17, 2014

Hi!
I somehow managed to delete the comment i just wrote, so again:
IE11 and IE for surface work fine.

The first snippet you posted looks the same as the solution i came up with and should work. The second snippet does not, since it relies on proto.

I think we should fix this, because it took me several hours to find the source of the problem (debugging is not exactly fun in IE) and melonJS claims to support all major browsers. I do believe that everyone should have an to an up-to-date-browser, but is this just not the case. WinXP-users for example have no possibility to use IE11 and even if it is not officialy supported anymore we should accept, that still a lot of people use XP and clients still want support for them.

pierrebeitz commented Jun 17, 2014

Hi!
I somehow managed to delete the comment i just wrote, so again:
IE11 and IE for surface work fine.

The first snippet you posted looks the same as the solution i came up with and should work. The second snippet does not, since it relies on proto.

I think we should fix this, because it took me several hours to find the source of the problem (debugging is not exactly fun in IE) and melonJS claims to support all major browsers. I do believe that everyone should have an to an up-to-date-browser, but is this just not the case. WinXP-users for example have no possibility to use IE11 and even if it is not officialy supported anymore we should accept, that still a lot of people use XP and clients still want support for them.

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Jun 17, 2014

Member

I see... could I ask then if maybe you could test the first workaround on IE9/IE10 ? my issue here is that I only have OSX machines and only 1 windows one that runs IE11 :P

If then fully working, i will extend it to also work with IE10 (but not IE11 or higher) and we can add it to the 1.0.x branch.

Member

obiot commented Jun 17, 2014

I see... could I ask then if maybe you could test the first workaround on IE9/IE10 ? my issue here is that I only have OSX machines and only 1 windows one that runs IE11 :P

If then fully working, i will extend it to also work with IE10 (but not IE11 or higher) and we can add it to the 1.0.x branch.

@pierrebeitz

This comment has been minimized.

Show comment
Hide comment
@pierrebeitz

pierrebeitz Jun 17, 2014

Yes, i will. It can take one or two days though.
You can emulate old IEs in IE11 in the developer tools. I also just have a virtual machine (from http://mordern.ie) for testing ;)

pierrebeitz commented Jun 17, 2014

Yes, i will. It can take one or two days though.
You can emulate old IEs in IE11 in the developer tools. I also just have a virtual machine (from http://mordern.ie) for testing ;)

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Jun 29, 2014

Member

@pierrebeitz I just added the here above workaround, see the other ticket linked here above, thanks !

Member

obiot commented Jun 29, 2014

@pierrebeitz I just added the here above workaround, see the other ticket linked here above, thanks !

@obiot

This comment has been minimized.

Show comment
Hide comment
@obiot

obiot Jul 1, 2014

Member

@parasyte @agmcleod since this ticket has been linked over the time with so many things that are either for most of them closed or not relevant anymore, i'm not sure of what to do with this ticket anymore ;P

Member

obiot commented Jul 1, 2014

@parasyte @agmcleod since this ticket has been linked over the time with so many things that are either for most of them closed or not relevant anymore, i'm not sure of what to do with this ticket anymore ;P

@agmcleod

This comment has been minimized.

Show comment
Hide comment
@agmcleod

agmcleod Jul 1, 2014

Member

I think other deep copy issue is kinda covering this. So i will close. Most of my initial concern has been met with the state of 1.1 & inheritance

Member

agmcleod commented Jul 1, 2014

I think other deep copy issue is kinda covering this. So i will close. Most of my initial concern has been met with the state of 1.1 & inheritance

@agmcleod agmcleod closed this Jul 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment