Collision hitbox - wrong coordinates - Crafty.e().clone

[mucaho]

The issue was originally posted on the forum.

I tracked the bug down, here is the scenario:

• Create an entity with Collision component and place the entity somewhere (x != 0, y != 0)
• Clone the entity; here is what happens:
  • All of the original enitity's components get added to the cloned entity
  • All of the copy-by-value properties (primitive types like numbers) get copied to the cloned entity (except special ones like the entitiy id)
• The collision hitbox of the cloned entity is wrongly offset by (-originalEntity.x, -originalEntity.y)

The problem occurs because the x, y of the original entity get copied to the cloned entity after the Collision component was added to the cloned entity.
Internally, the Collision component attaches it's hitbox to the entity, so that it moves alongside this parent entity. The problem is the hitbox's initial position is wrong and any later x/y changes through attach method are relative. That gives us a constant offset between the cloned entity and its hitbox (-originalEntity.x, -originalEntity.y)

[mucaho]

The easiest way to fix this is to check for a special case inside the clone method: call .collision() at the end of the method, if the Collision component was copied.
Is this the way to go or is there a more elegant way?

[mucaho]

Also the cloned entity does not get drawn until it's marked dirty, so clone should trigger("Invalidate")

[starwed]

The problem occurs because the x, y of the original entity get copied to the cloned entity after the Collision component was added to the cloned entity.

I'm guessing the problem here has to do with our use of getters/setters. That is, normally when you set the position of an object you do so with e.x=10, and the setter for .x triggers all the stuff that needs to happen when the position changes. (Such as moving the collision hitbox.)

Presumably when we clone, that doesn't happen? Instead the _x property gets read by the clone function and set directly. We might want to special case those 2D properties that rely on setters?

Now, there's actually an underlying common case here where an entity needs to be assembled from data. The general solution would be for components to run code when this happens; thus being able to handle this stuff without special cases in clone itself. I suspect this is something that'll be tricky to get right, so we should punt on that for now. (In addition to cloning, you might want this for saving/loading world state, and freezing/recycling entities.)

Also the cloned entity does not get drawn until it's marked dirty, so clone should trigger("Invalidate")

I think the init functions of the relevant components are supposed to trigger "Invalidate" when they're added -- are you sure clone needs to trigger it separately?

[mucaho]

are you sure clone needs to trigger("invalidate") it separately

Yes, I am positive.

I'm guessing the problem here has to do with our use of getters/setters

Take a look again at the last paragraph in my first post. Let me explain again:

• In the general case the clone() method works. The setters and getters are poperly set up and the "Change" event is fired correctly
• The problem is specific to the Collision component: it's how the Collision.init is implemented
• The init method attaches the hitbox to the entity before the entity has the correct position
  • Any attached child entity gets moved by the same amount the parent entity gets moved. So if you move the parent entity (Δx, Δy) the child will also move (Δx, Δy)
  • If you attach the child entity (the clone's hitbox in our case) to the parent entity (the clone itself in our case) before the parent entity gets moved from (x_before = 0, y_before + Δy = 0) to the correct coordinates (x_after, y_after)
    then future changes to the position of the parent entity will not move the child entity to the correct position (x_after + Δx, y_after + Δy) but rather to the position
    (x_before + Δx, y_before + Δy)
  • So to recap: Collision does not listen to absolute (x,y) changes, but delegates the work to the attach method which listens to relative (Δx, Δy) changes. If before cloning the original entity is on the position (0,0) you will not notice the bug. If, however, the original entity was on other position != (0, 0), then the hitbox will be falsely offset by that amount

[starwed]

Yes, I am positive.

Ah, odd -- I'd consider this particular problem more an error in "DOM" than in clone, then. Canvas elements get drawn properly, because the component takes care of things in init. (It works fine if you change DOM to Canvas in your example.) "Color" should also trigger an invalidate event on being added.

[starwed]

The init method attaches the hitbox to the entity before the entity has the correct position

Right, but that wouldn't matter if changing the position triggered the events it normally does. It starts at (0,0) just like the original entity, and Collision works fine in that case.

The setters/getters might exist, but they're not used by clone when it runs through setting properties.

e: Or if it does, it probably runs through the private versions first.

[starwed]

OK, according to MDN there's an enumerable option for defineProperty:

true if and only if this property shows up during enumeration of the properties on the corresponding object.
Defaults to false.

We don't set the option, so I think it'll be false for us, and therefore the setter/getters like .x won't show up in the clone loop. (I haven't played with this directly yet, so I still might be totally missing something.)

[mucaho]

Look at this JSFiddle - be sure to have the console open.
The comps and props print loop is copy pasted from the clone method. All properties get set at the clone (including _x and x), but no events get triggered (WTF?!?)
Uncomment last line and you will see an event popping.

[starwed]

@mucaho Fiddle link is broken, so total speculation: if _x is set before x then the event won't fire. Crafty would see that the property already has the target value, and so return early.

[mucaho]

Fixed link.

if _x is set before x then the event won't fire

Oh man nice catch. So what do we do about it?

[starwed]

Oh man nice catch. So what do we do about it?

We should blacklist the _x type properties somehow. We could always explicitly check for them in clone, I guess.

Maybe there's some defineProperty trickery we could use? I'm kind of unclear of what it means for a property to be enumerable, but maybe that option would let us blacklist them in a more consistent way?

[starwed]

Yeah, doing something like this seems to work?

Object.defineProperty(original, "_x", { enumerable: false });

We could just do that for all the properties we define setter/getters for... if it works cross-browser.

[mucaho]

We don't set the option, so I think the enumerability will be false for us

It should work that way yes. But the problem is we fallback to __defineGetter__ and __defineSetter__ so we can not rely on deterministically hiding the properties using the enumerable flag across every browser.