Motion tweaking #923

starwed · 2015-06-15T19:03:34Z

Several small issues with Motion/Control components

Fix bugs with NewDirection (and angular version)
- Fixes a rounding issue when calculating the sign of negative fractions
- Triggers NewDirection in the frame where the direction changes, instead of the next frame
- Renames NewRotation to NewAngularDirection and fixes the same issues
- Adds/fixes related tests
Sync motion with clock rather than number of frames
- Instead of adjusting the speed based on the frame rate, try to saty in sync with the game clock by using frame.dt. Note that speeds are in pixels per second.
- This is a breaking change, because it changes how we interpret speeds. (Pixels per second instead of pixels per frame.)
- Since it's a breaking change, I needed to fix several tests. I found the existing tests very hard to follow, so I added a lot of clarification, and removed some that seemed redundant.
Change Jumpway -> Jumper
Reword motion docs

- Fixes a rounding issue when calculating the sign of negative fractions - Triggers NewDirection in the frame where the direction changes, instead of the next frame - Renames NewRotation to NewAngularDirection and fixes the same issues - Adds/fixes related tests

starwed · 2015-06-15T19:07:28Z

As mentioned, this does have a breaking change: it changes speeds from pixels per frame to pixels per second.

I think this is a more natural definition in most cases -- I often had to choose very small speeds or accelerations with the old units. Any arguments to keep it as is, though?

starwed · 2015-06-15T19:17:54Z

A quick note about tests: lots of tests are certainly way better than none! But I wanted to give a few recommendations. (I'm not trying to pick on the tests for Motion, they just happened to be what I happened to interface with on this patch! And the thorough testing definitely let me feel good about making the changes! :) )

If tests don't have a description string, it becomes a bit difficult to discover which specific test is failing.
Ideally, each test() block should test one set of things in reasonable isolation, unless it's supposed to be an integration test. (Those are good too!)
There's no need to test every aspect of a behavior. (For instance, a lot of the tests for multiway checked that the velocity was set correctly and that the entities moved the correct distance -- the latter is actually testing "Motion", not "Multiway".)

mucaho · 2015-06-16T03:31:28Z

tests/2d.js

-    equal(newRevolutionEvents, 3);
-    equal(movedEvents, 6);
-    equal(rotatedEvents, 2);
-    equal(motionEvents, 17);


this checks shouldn't be removed, as we can not guarantee that all event callbacks fired as we wanted

mucaho · 2015-06-16T03:46:34Z

I have looked over the PR, good job!
Specifying speed in pixels per second is the most groundbreaking change I have witnessed so far. It's the right thing to do, but this will probably break all existing games! That's why I initially proposed we stick to dt * 0,05.
Nice job identifying the problem with the Math.sign solution from SO.

The only thing I would like to nitpick is the order of the NewDirection event. The motivation behind it was that NewDirection should be fired just before the velocity/acceleration was applied. If user has custom EnterFrame callback that changes velocity/acceleration, he can not be sure whether it is applied before/after Motion changes the velocity/acceleration. By binding to the NewDirection event he can make sure what combined velocity/accel will be applied and still change it to suit his needs (e.g. he can prevent moving left).
Initially I had the NewDirection event fire after Motion changed it (as it's natural), but maybe there is a corner case later where the paragraph above will be relevant. We can leave it as proposed in this PR for now.

starwed · 2015-06-17T19:07:04Z

Initially I had the NewDirection event fire after Motion changed it (as it's natural), but maybe there is a corner case later where the paragraph above will be relevant. We can leave it as proposed in this PR for now.

Another possibility would just be to trigger this when the velocity is actually changed. (So in the setter for the velocity properties.) That way it wouldn't matter how the velocity was changed.

starwed · 2015-06-17T19:10:33Z

Specifying speed in pixels per second is the most groundbreaking change I have witnessed so far. It's the right thing to do, but this will probably break all existing games! That's why I initially proposed we stick to dt * 0,05.

I think there are enough breaking changes since the last version that pretty much any game will require a bit of work to update, so I'm not too worried about this. I find it makes it easier to reason about how speeds and accelerations, so it should be worth it.

mucaho · 2015-06-17T20:10:22Z

I find it makes it easier to reason about how speeds and accelerations, so it should be worth it.

It also plays nicely with tweening.

Ok then, I suggest you readd some test checks, as well as update the default values for movement and their respective documentation (_speed in Multiway and _jumpSpeed in Jumper).
After that we can merge this :)

mucaho · 2015-06-18T01:30:07Z

Another possibility would just be to trigger this when the velocity is actually changed. (So in the setter for the velocity properties.) That way it wouldn't matter how the velocity was changed.

Yes, that would be great! Note that it will introduce some overhead probably.

Add some in-line documentaiton, and tweak the wording of the API docs for the Motion component

starwed · 2015-06-22T20:50:37Z

Ok, I re-added those tests. (A better fix would be to break up that long litany of tests into several isolated ones, and then check that each event was called individually, but I think this is fine for now.)

I also adjusted the default speeds/accelerations to match the new time-scale.

mucaho · 2015-06-24T03:36:14Z

src/controls/controls.js

@@ -450,13 +451,13 @@ Crafty.c("Jumpway", {
     *
     * @example
     * ~~~
-     * this.jumpway(6, ['UP_ARROW', 'W']);
-     * this.jumpway(['UP_ARROW', 'W']);
+     * this.jumper(6, ['UP_ARROW', 'W']);


update speed here, e.g. 300

mucaho · 2015-06-24T03:46:34Z

Great, I noticed some minor things that should be updated in documentation.

Another possibility would just be to trigger this when the velocity is actually changed. (So in the setter for the velocity properties.) That way it wouldn't matter how the velocity was changed.

Should this be in current or seperate PR?

starwed · 2015-06-24T03:58:01Z

Good catch with the examples.

Should this be in current or seperate PR?

If we do that, it'll be a separate PR. Since there's an event any time the velocity changes, I'm not sure how important it is to have a separate one that occurs for a particular type of change.

Instead of adjusting the speed based on the frame rate, try to saty in sync with the game clock by using frame.dt. Note that speeds are in pixels per second. This is a breaking change, because it changes how we interpret speeds. (Pixels per second instead of pixels per frame.)

"Jumpway" is probably a bit confusing, even though it kind of makes sense in the context of Twoway and Fourway.

starwed · 2015-06-29T20:41:39Z

Ok, all the examples should be updated now.

Motion tweaking

letmaik · 2015-07-09T19:42:02Z

I just noticed this since I use the nightlies:
http://neothemachine.github.io/hungrymonkey/

I think to make the transition easier it would good to provide some rough formula to get from pixels per frame to pixels per second, assuming some common framerates like 60fps.

mucaho · 2015-07-12T12:45:42Z

To go from old [px / frame] to new [px / s] you multiply it by the target frame rate, which defaults to 50, e.g.:

old jumpSpeed = 6 [px / frame] -> new jumpSpeed = 300 [px / s]
old gravityConst = 0.2 [px / frame²] -> new gravityConst = 0.2 * 50² = 500 [px / s²]

Good idea, we should probably add this to the changelog

see craftyjs/Crafty#923 (comment)

letmaik · 2015-07-12T13:04:46Z

Yep, that worked. Thanks

starwed mentioned this pull request Jun 15, 2015

Speed of Motion (and related) components depend on the current frame rate #920

Closed

mucaho reviewed Jun 16, 2015
View reviewed changes

Reword motion docs

68c160e

Add some in-line documentaiton, and tweak the wording of the API docs for the Motion component

starwed force-pushed the motion-tweaking branch 2 times, most recently from 3f52c06 to da56d20 Compare June 22, 2015 20:49

mucaho reviewed Jun 24, 2015
View reviewed changes

starwed added 2 commits June 29, 2015 16:39

Change Jumpway -> Jumper

d72d8b6

"Jumpway" is probably a bit confusing, even though it kind of makes sense in the context of Twoway and Fourway.

starwed force-pushed the motion-tweaking branch from da56d20 to d72d8b6 Compare June 29, 2015 20:41

mucaho pushed a commit that referenced this pull request Jun 30, 2015

Merge pull request #923 from starwed/motion-tweaking

3a93ddc

Motion tweaking

mucaho merged commit 3a93ddc into craftyjs:develop Jun 30, 2015

letmaik mentioned this pull request Jul 9, 2015

Adapt to new speed definition letmaik/hungrymonkey#2

Closed

letmaik added a commit to letmaik/hungrymonkey that referenced this pull request Jul 12, 2015

fixes #2

f59f5cc

see craftyjs/Crafty#923 (comment)

mucaho mentioned this pull request Jul 24, 2015

Has Fourway slowed down recently? #945

Closed

starwed mentioned this pull request Apr 1, 2017

devDemos are painfully slow craftyjs/demos#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Motion tweaking #923

Motion tweaking #923

starwed commented Jun 15, 2015

starwed commented Jun 15, 2015

starwed commented Jun 15, 2015

mucaho Jun 16, 2015

mucaho commented Jun 16, 2015

starwed commented Jun 17, 2015

starwed commented Jun 17, 2015

mucaho commented Jun 17, 2015

mucaho commented Jun 18, 2015

starwed commented Jun 22, 2015

mucaho Jun 24, 2015

mucaho commented Jun 24, 2015

starwed commented Jun 24, 2015

starwed commented Jun 29, 2015

letmaik commented Jul 9, 2015

mucaho commented Jul 12, 2015

letmaik commented Jul 12, 2015

Motion tweaking #923

Motion tweaking #923

Conversation

starwed commented Jun 15, 2015

starwed commented Jun 15, 2015

starwed commented Jun 15, 2015

mucaho Jun 16, 2015

Choose a reason for hiding this comment

mucaho commented Jun 16, 2015

starwed commented Jun 17, 2015

starwed commented Jun 17, 2015

mucaho commented Jun 17, 2015

mucaho commented Jun 18, 2015

starwed commented Jun 22, 2015

mucaho Jun 24, 2015

Choose a reason for hiding this comment

mucaho commented Jun 24, 2015

starwed commented Jun 24, 2015

starwed commented Jun 29, 2015

letmaik commented Jul 9, 2015

mucaho commented Jul 12, 2015

letmaik commented Jul 12, 2015