Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gravity update to bird.js #45

Merged
merged 3 commits into from
Apr 9, 2018
Merged

Gravity update to bird.js #45

merged 3 commits into from
Apr 9, 2018

Conversation

buensons
Copy link
Contributor

@buensons buensons commented Apr 1, 2018

Hello everyone!

First off, I would like to say that I'm new to GitHub and this is my first pull request, so if I did something wrong, I'm sorry.

Recently, I was inspired by Coding Train's Flappy Bird video to make my own version of the game in Java. As I created my game, I would like to add one minor change to this JavaScript version, which I think improves it a little bit, as it adds acceleration of gravity to the game. Before, the 'bird' was going down with a constant speed, and after my changes it accelerates while going downwards.

Changes:

  1. I removed the 'this.lift' variable and replaced it with this.delay which is used to limit the speed of going up, by setting a delay.

  2. Changed up() function to implement 'delay'

  3. Removed 'this.velocity *= 0.9', because it's function was to limit the speed, and as I said I did it by implementing delay. It was also causing the lack of acceleration.

Hopefully you like my change, and my contribution is somewhat useful. Once again, it's my first time contributing to a project on GitHub, so I'm sorry for any mistakes.

Cheers!

Implement gravity
bird.js Outdated
@@ -12,7 +12,7 @@ class Bird {
this.x = 64;

this.gravity = 0.6;
this.lift = -15;
this.delay = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a boolean flag variable make more sense here?

Copy link
Contributor Author

@buensons buensons Apr 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. A boolean variable would work, and it's going ot be easier to read. Is it better now?

bird.js Outdated
@@ -26,12 +26,17 @@ class Bird {
}

up() {
this.velocity += this.lift;
if (this.delay)
Copy link

@ZwergB ZwergB Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this whole this.delay variable kind of irrelevant? Sorry if I miss something but it seems like you could delete the entire if-statement and just leave line 33.

bird.js Outdated
this.delay = false;

if (!this.delay) {
this.velocity = -7;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a global variable for the -7 would be a good thing to do. It's easier to modify static values when they are global.

@buensons
Copy link
Contributor Author

buensons commented Apr 3, 2018

Yes, you're right. I can see now that this.delay is completely useless here. I used it in a bit different way in Java to fix one problem, but I can see that it doesn't make sense here, and just the line 33 will give the same results.

@shiffman
Copy link
Member

shiffman commented Apr 9, 2018

Thank you for this contribution! I am going to merge it and test it out. I might revert if I prefer the previous behavior but very thankful for this suggestion. More soon!

@shiffman shiffman merged commit 8e8a0a3 into CodingTrain:master Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants