Add a default shape to an entity body if not overridden by the entity constructor.

[agmcleod]

No description provided.

[agmcleod]

Need to update the wiki entry here: https://github.com/melonjs/melonjs/wiki/Shapes to show two things:

1. A screen shot of a rect/ellipse in Tiled, as a visual for how that works. There's one there now, but could probably expand on it a bit.

2. A code example of the same thing that doesn't involve tiled at all

This code example can be added to the comment docs as well.

[ldd]

if you look at the code of Entity, you initialize the body based on settings.getTMXShapes or otherwise it is []

possible ideas:

• add settings.getInitialShape and check it (something along the lines of
  this.body = new me.Body(this, (
  typeof (settings.getTMXShapes) === "function" ?
  settings.getTMXShapes() : settings.getInitialShapes()
  ));
• instead of [] assign a shape and check the entities' orientation...
• remind people to add a shape after initialization, which seems weird.

[parasyte]

I agree with @ldd. Moved this to the next major milestone. Documentation is not the right solution. We need a better API to provide a default shape as a rule. The exception is that users may instead add a list of their own shapes.

I suggest we rename getTMXShapes and expose it as the supported method to install custom shapes in the me.Entity constructor, with a fallback to [ new me.Rect(0, 0, this.width, this.height) ] That way the default rectangle shape can be overridden

[agmcleod]

Yeah agreed, i think that is an excellent idea!

[obiot]

Not sure I agee with this, especially with the part where we expose the getTMXShape function so that end user can override it to add their own shape, that sounds awefully "custom" to me !

In my opinion having to manually add a shape to a body makes as much sense as when we manually add a renderable to an entity, and as far as i can see this is also how it works in physic engine, no?

However, as a compromise, we could add a basic rect shape at the size of the entity renderable frame, but this would be done in the getBounds function (as part of the initial check for bounds) so that this would work both for tiled and manual coding.

[agmcleod]

I think the idea would be to pass a shape through as an option, over it falling back to an empty array.

[parasyte]

The tradeoff here is convenience vs customizability. A body without a shape is not useful, in the same way that an entity without a body is not useful. An entity that cannot perform physics or hit detection is just a sprite, and we have a lightweight class for that.

The problem we need to solve here is that adding an entity is not enough to get collision detection, you also need to add a shape. The simplest, most basic case is that adding an entity should also add a default shape. However, it should not always add a default shape.

The best API I can think of is one that opts-in to not have a default shape. And why opt-in when you can just give it the shape you really want in place of a default dummy shape? Well, we already have that functionality through getTMXShapes! My suggestion is to reuse it for that purpose. I don't like the other alternatives provided, so far.

[ldd]

umm what about

var initialShape;
if (settings.initialShape && (typeof (settings.initialShape) === "function")){
    initialShape = settings.initialShape();
}
else if (settings.getTMXShapes && (typeof (settings.getTMXShapes) === "function")){
    initialShape = settings.getTMXShapes();
}
else{
        initialShape = me.TMXObject.__methods__.getTMXShapes.apply({
                             width: settings.width, height: settings.height, rotation: 0});
 //ninja-edit: controversial? PS: rotation can be something besides 0, but I simplified that here
}
   this.body = new me.Body(this, initialShape);

etc

[ldd]

pros of my idea:

• the user never overrides or sees me.TMXObject's code
• the user must know what she is doing when passing an initial shape that is NOT default
• if the user passes an initial shape, this is taken into account regardless of whether settings.TMXObject is defined or not.
• if the user does something like settings.initialShape = "rectangle", the default shape is still used

cons of my idea:

• if you don't want an initial Shape, you must know exactly what to put in settings.initialShape and this is not necessarily trivial (I'd argue that this is good, because it means that the user has understood that she will not have a shape in her Entity and she has read the docs =D)
• More precisely, settings.initialShape = function(){return [];};
• I have no idea if me.TMXObject.__methods__ is a hack or not XD
• not backwards-compatible: user probably added a shape in her code, so this might break things (nothing that a note in an upgrade guide couldn't fix)

[obiot]

To be honest i think that this is over complicating things and we should keep all this as simple as possible.

For "collidable" object, an entity must define a body that contains at least one shape, and at this point i would even just modiffy the code to just throw an exception at rutime if no shape is defined for an object.

To disable collsion collisionType can be used, and for non collidable object a simple renderable object is the way to go.

[parasyte]

Throwing an exception just pushes the onus of the default shape boilerplate code to the user. melonJS should always try to provide sane defaults, to the best of its ability.

[obiot]

yes indeed, and exactly like renderable. I'm only worried that we will end up with something super complicated, where we could just say that end-user have to add a collision shape, if we do not want to throw an exception, we could add a warning message in the console ("no collision shape defined for object XXXX, collision will be disable"), that seems to be fine enough for me.

[ldd]

Like @parasyte commented, I think exposing TMXShapes makes sense. (maybe?)

Let me at least mention my specific case:
NONE of my entities are defined in Tiled, and all need the same (initial) Shape.
Right now, I simply created a baseEntity that extends me.Entity.
In its constructor, I add TMXShapes to settings with a default value (the one i mentioned above) and then call _super(me.Entity, 'init',...)

That's all that baseEntity does.
(as you can tell, this is not ideal.)

[parasyte]

@ldd is this for your isometric project? Perhaps #601 is deeply related... If a user supplies a getTMXShapes function, then she must also supply the shapes with isometric projection applied. That's really bad for user experience.

[obiot]

err... this one , I dunno what to do :)

last time we however discussed that settings should provide a shapes[] property with all the defined shapes for an object, this will however not help with the "add a default shape iif no shape is defined" question :)

[parasyte]

this.body = new me.Body(this, settings.shapes instanceof Array ? settings.shapes : [ new me.Rect(0, 0, this.width, this.height) ]);

This solves it all. We get our overrides; settings.shapes is an array that replaces the settings.getTMXShapes callback, and can be an empty array. In the case it is not an array, a default Rect is created.

[ldd]

+1000. (it doesn't help with isometric shapes, but...)

settings.shapes = [...];
is better than
settings.getTMXShapes = function(){return [...]};

bonus: noDefaultShape is no longer needed

[agmcleod]

Yeah i agree. I like this proposal as well :)

On Thu Feb 12 2015 at 9:30:30 AM ldd notifications@github.com wrote:

+1000. (it doesn't help with isometric shapes, but...)

settings.shapes = [...];
is better than
settings.getTMXShapes = function(){return [...]};

—
Reply to this email directly or view it on GitHub
#614 (comment).

[obiot]

why is noDefaultShape no longer needed ? this one is used to prevent from creating a default shape when using Tiled, if not there how do you prevent a default shape to be created ?

[obiot]

my only issue with this is that now (after) we would have a set of instantiated me.Shape object into settings, as opposed to now (before) where we only have the shape definition (but no object instances).

which means that when passing settings.shapes[] to entities, they will all share the same shapes references, which can be good too, but if one entity start modifying the defined shapes, it will cause issues with other entities of the same type

[ldd]

instead of noDefaultShape, in Tiled or programmatically, you set shapes = []
i.e:
//nothing = assume default from Tiled that used to be created by TMXgetShapes()
shapes = [] // no shape
shapes = [new me.Rect...] // some defined shape

but I guess technically speaking noDefaultShape can still be used, if desired.

[parasyte]

In Tiled, it's shapes = json:[]

[obiot]

no, that does not work as programmatically add or not a default shape defeats the initial purpose of the noDefaultShape flag (which is to ignore the default shape given by Tiled).

https://github.com/melonjs/melonJS/blob/master/src/entity/entity.js#L120

so I would rather do :

if (settings.noDefaultshape === true) {
   this.body = new me.Body(this, []);
} else {
   this.body = new me.Body(this, settings.shapes instanceof Array ? settings.shapes : [ new me.Rect(0, 0, this.width, this.height) ]);
}

[obiot]

also, as a bonus question, what would be the default value of this.width and this.height here if the object is programmatically created ?

[parasyte]

In what way is [] different from []? (I pose this question in the form of making a point that there's no difference between the following inputs to the code you provided.)

settings.noDefaultShape = true;

... or ...

settings.shapes = [];

Bonus answer:

melonJS/src/entity/entity.js

Lines 127 to 130 in 3572679

	// resize the entity if required
	if (this.width === 0 && this.height === 0) {
	this.resize(this.body.width, this.body.height);
	}

[parasyte]

By the way, me.Body.addShapesFromJSON is the new way to specify shapes as a data structure, right? Seems like a practical solution to your issue regarding object references. (It isn't like the rest of melonJS is immune to resource sharing violations. The issue doesn't raise anything new.)

[obiot]

yes, they are the same, but from Tiled itself it's easier to set a custom property, rather then settings the array to []. But yes, the internal (melonJS) code should set array to [] if the flag is defined.

as a reminder this flag is iniitally only to discard any shape creation when using Tiled.

[obiot]

hi guys, so I implemented what I think is the result of the above discussion. Please review and comments (separate branch for now until we reach a consensus)

[obiot]

do we merge this branch ?

[parasyte]

@obiot Yeah, the last patch is way better! 😆 I'm ok with merging this branch now.