Animation.loop should be true by default for sprites

[alanag13]

Perhaps this could be up for debate, but I feel that in the majority of cases it is desirable for a sprite to loop indefinitely.

[vritant24]

Hi, I'd like to try this out.
Could I get some help with finding where I would start?

[kamranayub]

Yes (we need to make sure we do this for jump in's, sorry).

Change this LOC to true:

https://github.com/excaliburjs/Excalibur/blob/master/src/engine/Drawing/Animation.ts#L43

Then verify this in a test. We don't yet have an AnimationSpec so you can create a new one (hooray!):

https://github.com/excaliburjs/Excalibur/tree/master/src/spec

Just need to verify by default animation.loop is true then when constructing a new animation.

Finally, update the Animations.md readme in the src/engine/docs folder with the new default info, as well as add a CHANGELOG entry.

That should be it. Thanks!

[vritant24]

Alright thanks for the steps. I've made the change to Animation.ts and added an AnimationSpec.ts where I wrote the the test.
How am I supposed to run the tests? When I run npm tests it gives me the error Cannot find name 'ex'. for all the specs

Edit - fixed this issue with npm run all

[vritant24]

I have created the test, and it works. I cannot complete npm run all because another test fails.
Should I go ahead and make the pull request?

[kamranayub]

Can you send us the names and failure messages for the tests that are failing? I believe none should be but there may be another cause.

…

On Wed, Jun 28, 2017 at 9:17 AM Vritant Bhardwaj ***@***.***> wrote: I have created the test, and it works. I cannot complete npm run all because another test fails. Should I go ahead and make the pull request? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#583 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiaawRrsF22w3mkRUlbwe7caeAhuyKWks5sImAMgaJpZM4IJExN> .

[vritant24]

The following test fails. I'm running this on Mac OSX, and i also tried it out in a branch that didn't have the changes I made. It still failed.

A label
X can measure text width
     Expected 332.0625 to be 335. (1)

[kamranayub]

@jedeen was this test disabled in your recent PR? I thought we disabled all label tests for now...

[eonarheim]

@vritant24 Ah, this must have been missed in f88fa2a Mac OSX will render font width differently than other platforms, hence the reason we've disabled those tests because of the heartache they cause.

Feel free to x the test xit('can measure text width') so that this font test doesn't run and submit a PR for the Animation fix.

[vritant24]

Alright awesome. That fixed it.

[jedeen]

@eonarheim Yeah, I only disabled tests that failed locally on the new update of Windows 10.