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

Graeme/playerbumpin #249

Merged
merged 3 commits into from
Jul 31, 2018
Merged

Graeme/playerbumpin #249

merged 3 commits into from
Jul 31, 2018

Conversation

Grafne23
Copy link
Collaborator

@Grafne23 Grafne23 commented Jul 25, 2018

🎟️ Ticket(s): Closes #210


👷 Changes

Changes player to player collision to not directly modify either players position. Also adds a period to award points to a player if another player is killed within a number of ticks after a collision with the still alive player.

🔦 Testing Instructions

Play a bit and see if you bump a player into a hole you are given points. And if you bump a player and then much later they drive themselves into a hole you are not awarded points.

@Grafne23 Grafne23 added the ready for review This issue or pull request is ready to be reviewed label Jul 25, 2018
@coveralls
Copy link

coveralls commented Jul 25, 2018

Coverage Status

Coverage increased (+6.07%) to 70.986% when pulling 6cf6b86 on graeme/playerbumpin into 5d217dc on master.

@@ -168,12 +168,12 @@ func (a *Arena) playerCollisions() {
memo := make(map[*models.Player]*models.Player)
for _, player := range a.Players {
for _, playerHit := range a.Players {
if player == playerHit || memo[playerHit] == player {
if player == playerHit || memo[player] == playerHit {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔥

continue
}
if areCirclesColliding(player.Position, models.PlayerRadius, playerHit.Position, models.PlayerRadius) {
memo[playerHit] = player
player.HitPlayer(playerHit, a.Height, a.Width)
player.HitPlayer(playerHit)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Angle float64 `json:"angle"`
Controls KeysPressed `json:"-"`
Points int `json:"points"`
PlayerHitMe *Player `json:"-"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you rename this to LastPlayerHit? It'll keep the naming of this property consistent with our Junk type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That variable, like the junk one, keeps track of the player that hit that player. Not the last player that that player hit. How confusing is that ;).
I renamed the junk one to match this one, is that okay? I want it to be clear to everyone lol

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I follow 😖Can you elaborate?

From my understanding right now, if two players collide, their LastPlayerHit is always each other. There isn't a concept of who hit who right? So there probably isn't a need to differentiate between "Player B that hit Player A and the last player that Player A hit" <- am I understanding your comment correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a very good point. Yeah I just had it in my head that it mattered from when I started but switched at some point to it not mattering. Okay, LastPlayerHit is best then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh I see. Yeah LastPlayerHit is good enough :)

initalVelocity := p.Velocity
// HitPlayer calculates collision, update Player's velocity based on calculation of hitting another player
func (p *Player) HitPlayer(ph *Player) {
if p.pDebounce == 0 {
Copy link
Collaborator

@brian-nguyen brian-nguyen Jul 26, 2018

Choose a reason for hiding this comment

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

To avoid the extra nest, let's just return early here 🤓🐦

@@ -205,3 +205,83 @@ func TestKeyHandler(t *testing.T) {
})
}
}

func TestPlayerBumpPlayer(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔥 🔥 🔥

Copy link
Collaborator

@brian-nguyen brian-nguyen left a comment

Choose a reason for hiding this comment

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

Great job. Thanks the adding the tests for this new functionality.

Just a few comments about naming and returning early.

SeeratSek
SeeratSek previously approved these changes Jul 26, 2018
brian-nguyen
brian-nguyen previously approved these changes Jul 28, 2018
@brian-nguyen
Copy link
Collaborator

Great! If you're up for it, you can try squashing those last 4 or 5 commits, otherwise feel free to merge whenever you are ready.

@Grafne23
Copy link
Collaborator Author

Squashed those commits, much cleaner I agree. Needs your review again though now Brian.

Copy link
Collaborator

@brian-nguyen brian-nguyen left a comment

Choose a reason for hiding this comment

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

👍

@brian-nguyen brian-nguyen merged commit e3727e1 into master Jul 31, 2018
@brian-nguyen brian-nguyen deleted the graeme/playerbumpin branch July 31, 2018 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review This issue or pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants