Refactor MultiWay

[mucaho]

Overview

This is the continuation of #708 in order to incorporate the new Motion component into the library.
This PR focuses on changing Multiway to work with Motion.

Details of this PR

• Use Motion component in MultiWay to move entities.
• re-enable movement after control has been disabled and enabled again, during which a key was held the whole time
• allow changing speed on the fly (while keys are pressed)
• allow changing direction ony the fly (while keys are pressed) - think of a "disorient player" effect that inverts all his movement keys suddenly :)
• incorporate detection of multiple keys pressed for same direction as in Have multiway keep track of the number of keys pressed for each direction #794 / Pressing direction keys simultaneously causes fourway to move multiple times #793
  The current solution prevents keys for the same direction to move the player twice as fast. However, for more complex scenarios, additional modifications will need to be done (see linked PR/issue)
• add tests for detection of multiple keys pressed for same direction
• move NewDirection event to Motion component, make it trigger before EnterFrame motion tick
  also add NewRevolution to AngularMotion component analoguely
• add tests for NewDirection and NewRevolution events
• update documentation

Changes from user point-of-view

• NewDirection event changed significantly:
  • direction means something different now: direction = {x: Math.sign(vx), y: Math.sign(vy)}
  • the event arguments are thus normalized to -1, 0 or 1
  • it is now triggered only once per frame, if the direction is different from last frame
  • it is triggered immediately before the motion tick, which enables the user to change/limit the direction before it is applied
  • should it behave like before (absolute values instead of sign)? However, multiple components may alter the velocity now, so absolute values change constantly in contrast to before (think gravity and twoway)

Issues

• if Multiway is added, while a key is pressed, the component will not function properly (unfortunately as described in Logic separation of components #578 we can not assume access to the keyboard state on the server)
  I may have a fix for this, but need to test resolved, see comment
• despite the changes to Multiway, the speed of movement should be approximately close when using the same speed argument before and after this change
  __convertPixelsToMeters should multiply by 50, instead of 100, but then Gravity and Twoway don't work well

Future Changes regarding Motion

• move Multiway, Fourway, Twoway, Gravity into new src file, also move the tests accordingly (move control tests from 2D to controls)
• make sure to let user set vx and similar in pixels, internally convert to correct scale
• cascade Motion changes to children
  • currently when player jumps from a platform in motion, the player looses all momentum it had
  • it will be tricky to implement:
    • _cascade "MotionChange" events, just like "Move" & "Rotate" events to _children
    • figure out how both can co-exist, because if both "MotionChange" and "Move" event is propagated to child, child will move twice as much
    • trigger "Move" event after "MotionChange" was applied in next "EnterFrame"
• optionally add some kind of inertia to movement controls, something along the lines of this tut

var gradualIncrease = function(frameData)  {
    switch ( moveState ) {
        case MS_LEFT:  
            vel.x = max( vel.x - 0.1f, -5.0f );
            if (vel.x == -0.5f)
                this.unbind("EnterFrame",  gradualIncrease) 
            break;
        case MS_STOP:  
            vel.x *=  0.98; 
            if (vel.x == 0.0f)
                this.unbind("EnterFrame",  gradualIncrease) 
            break;
        case MS_RIGHT:
            vel.x = min( vel.x + 0.1f,  5.0f ); 
            if (vel.x == 0.5f)
                this.unbind("EnterFrame",  gradualIncrease) 
            break;
    }
};
this.bind("EnterFrame", gradualIncrease);

[mucaho]

@starwed would you kindly take a look at this, if you have time/motivation? would like to hear your opinion

[starwed]

Sure, I have some work to do this afternoon, but should be able to look at it either later today or tomorrow.

[mucaho]

Alrighty, I have adapted the code a bit. The Crafty.kedown issue should be resolved for now.

A possible future issue is that Multiway uses Crafty.keydown internally to detect already pressed keys and applies movement in retrospect. Initially I wanted to remove the dependency on Crafty.keydown in order to make it work smoothly on a headless server (see #603 ). However, I have postponed this feature for another PR, because there are some things to consider:

• Why not refactor the current keyboardDispatch so that the key state in Crafty.keydown can also be set over the net? The problem is that 2 players playing co-operatively, each represented by their own entity & Multiway on the server, would trigger each others movement as both players could bind to UP_ARROW for moving upwards (as Crafty.keydown is a global, rather than a per-entity based key state)
• Another solution would be to record the key state for each Multiway seperately. Each time the Multiway component registers a key press, it updates it's state. That introduces some code complexity, but works. However, Multiway can not register any key presses before it was added, thus making it impossible for it to retrospectively apply movement.
• The cleanest solution (imo) is to have a dedicated, per-entity component which records all Mouse, Touch and Keyboard states. However, implementing this out-of-scope of this PR. User should take care to add it upon initial entity construction. A later added Multiway could then access the entity's input state and apply movement retrospectively.

[starwed]

Regarding netplay: It's probably not worth worrying about netplay with Multiway specifically. As soon as you want more control over how an entity moves (whether it's tweaking the physics, having contextual controls, or syncing with a server) you're going to have to write your own code for that stuff. Something like Multiway can't possibly meet every use case -- its main reason for existence is to help someone just starting with Crafty.

I'd agree that a component or system that makes it easy to connect input to internal game state would be super usefull! Like you say, we can perhaps convert to a better model in a future PR.

[starwed]

PR in general: I don't see any specific problems jumping out, but it's a big enough set of changes I'll want to make up a small demo and play around with the upgraded components.

[starwed]

I don't think this will matter too much, but the way we've recommended handling collisions is to listen to the "Moved" event, test for collisions, and then move back to the old position if necessary.

The catch here is that this will cause a second "Moved" event to trigger on the entity before the first has returned, and both of them use the same _movedEvent object. As long as the collision check is the only thing listening to Moved, no big deal, but it might cause some unexpected behavior if someone has multiple event handlers for "Moved".

I think this would be pretty rare, so I'm ok solving this particular issue down the road. But maybe add a comment about it?

[mucaho]

Thanks for taking a look at it. We should discuss 3 more things before proceeding.

Move Event

The catch here is that this will cause a second "Moved" event to trigger on the entity before the first has returned

How could a second move event be triggered before the first one has returned?
This was the code for triggering the Move event before.

if (this._movement.x !== 0) {
    this.x += this._movement.x;
    this.trigger('Moved', {
        x: this.x - this._movement.x,
        y: this.y
    });
}
if (this._movement.y !== 0) {
    this.y += this._movement.y;
    this.trigger('Moved', {
        x: this.x,
        y: this.y - this._movement.y
    });
}

In order to preserve the old semantics I refactored it to this. However, the event is triggered in the 1st block with the new y which isn't even assigned to this.y yet. Good catch!

var movedEvent = this.__movedEvent;
if (this._dx !== 0) {
    this.x = newX;
    movedEvent.x = oldX;
    movedEvent.y = newY;
    this.trigger('Moved', movedEvent);
}
if (this._dy !== 0) {
    this.y = newY;
    movedEvent.x = newX;
    movedEvent.y = oldY;
    this.trigger('Moved', movedEvent);
}

Originally I had this. This makes the most sense to me, even if the Move event doesn't trigger twice anymore.

var movedEvent = this.__movedEvent;
if (this._dx !== 0 || this._dy !== 0) {
    this.x = newX;
    this.y = newY;
    movedEvent.x = oldX;
    movedEvent.y = oldY;
    this.trigger('Moved', movedEvent);
}

There's even another alternative which circumvents setting x or y if it wasn't changed (since _attr doesn't check if oldValue == newValue, probably because of performance reasons). Although I think this is uglier than the first/old approach.

var movedEvent = this.__movedEvent;
var chandedX = this._dx !== 0,
    changedY = this._dy !== 0;
if (changedX || changedY) {
    if (changedX)
        this.x = newX;
    if (changedY)
        this.y = newY;
    movedEvent.x = oldX;
    movedEvent.y = oldY;
    this.trigger('Moved', movedEvent);
}

Which approach would you recommend?

MoveSpeed Issue

If you use the same speed arguments, from before the Motion patch, in Twoway, Multiway or Gravity you will get very different results than before the introduction of Motion.
After long testing of different values across old and new Crafty versions, I think I have finally figured how to make the speed arguments 99% backward compatible. The speed backward-compatibility and bug fixes to gravityConst & Twoway speed will be added in new PR.

Overly cautious event objects

Instead of creating a new object when triggering Moved and NewDirection events, I changed it to use the same object from last frame instead.

Specifically the NewDirection event looks like this

var oldDirection = this.__oldDirection;
var _vx = this._vx, dvx = _vx >> 31 | -_vx >>> 31, // Math.sign(this._vx)
    _vy = this._vy, dvy = _vy >> 31 | -_vy >>> 31; // Math.sign(this._vy)
if (oldDirection.x !== dvx || oldDirection.y !== dvy) {
    var directionEvent = this.__directionEvent;
    directionEvent.x = oldDirection.x = dvx;
    directionEvent.y = oldDirection.y = dvy;
    this.trigger('NewDirection', directionEvent);
}

This could be simplified to the following. However, if user changes properties of the event object, the NewDirection event triggering will result in invalid state in the following frames.

var directionEvent = this.__directionEvent;
var _vx = this._vx, dvx = _vx >> 31 | -_vx >>> 31, // Math.sign(this._vx)
    _vy = this._vy, dvy = _vy >> 31 | -_vy >>> 31; // Math.sign(this._vy)
if (directionEvent.x !== dvx || directionEvent.y !== dvy) {
    directionEvent.x = dvx;
    directionEvent.y = dvy;
    this.trigger('NewDirection', directionEvent);
}

What is better here? Favor safety over computational performance every frame?

[starwed]

I mocked up a simple test, and Multiway seems to work fine to me! I didn't notice any problems with our front-page pong demo either.

How could a second move event be triggered before the first one has returned?

Ah true, I was thinking of the separate "Move" event which occurs when you assign new coordinates. So long as code responding to the "Moved" event doesn't trigger "Moved" itself, it's ok.

_attr doesn't check if oldValue == newValue, probably because of performance reasons

It totally does! :)

The speed backward-compatibility and bug fixes to gravityConst & Twoway speed will be added in new PR.

If this is a big pain, it's ok to break backwards compatibility here. It looks to me like the next released version of Crafty will have several breaking changes already, so now it a good time to make painful (but needed) breaks with the past.

Was there anything more you wanted to do before this is merged?

[mucaho]

Was there anything more you wanted to do before this is merged?

Thanks for the pointers, I have updated Moved event to trigger like this

var movedEvent = this.__movedEvent;
if (this._dx !== 0) {
    this.x = newX;
    movedEvent.axis = 'x';
    movedEvent.oldValue = oldX;
    this.trigger('Moved', movedEvent);
}
if (this._dy !== 0) {
    this.y = newY;
    movedEvent.axis = 'y';
    movedEvent.oldValue = oldY;
    this.trigger('Moved', movedEvent);
}

It makes more sense to me. Is it ok to break backward compatibility here (NewDirection also breaks backward compatibility)?

[mucaho]

No objections thus far to the Moved event change, so merging. Can still be changed if needed in testing phase.