shivansh-bhatnagar18 / multiplayer-uno

https://multiplayer-uno.vercel.app
21 stars 43 forks source link

gameEvents: created `ANNOUNCE_UNO` event #136

Closed sethdivyansh closed 1 week ago

sethdivyansh commented 2 weeks ago

Fixes: #132

Description

In short:

The plan this PR followed to handle ANNOUNCE_UNO event:

Checklist

Screenshots (if applicable)

[If your changes include any visual updates, provide screenshots here.]

vercel[bot] commented 2 weeks ago

@sethdivyansh is attempting to deploy a commit to the Shivansh Bhatanagar's projects Team on Vercel.

A member of the Team first needs to authorize it.

sethdivyansh commented 1 week ago

@kuv2707 According to the game rules, the vulnerable duration lasts until the next player has played their turn. Should I keep it this way, or should I set the vulnerable duration based on the number of players?

image
kuv2707 commented 1 week ago

In that case, we'll go with the rules. It'll be easier to implement too

sethdivyansh commented 1 week ago

@kuv2707 I have made the changes and followed your structure with some modifications. Please see the updated structure in the PR description.

kuv2707 commented 1 week ago

Please address this to fix the CI test.

sethdivyansh commented 1 week ago

Seems like we are allowing announce_uno when either the player has 2 cards and one of them is throwable, or he has only one card. This means we allow the player to announce uno even when its next player's turn now(which wasn't originally what I had thought). I am a little worried it would lead to race conditions, but the idea seems exciting right now (and would add to the thrill), so we can integrate this for now and see if it works well.

Seems good enough to merge, once the minor remarks are addressed

Yes, you are right. I have made it such that players can announce UNO even if it's next player's turn. As players can be caught not saying "UNO" before the next player has taken their turn.

kuv2707 commented 1 week ago

This seems good for now, we can optimize this code later if need arises.

kuv2707 commented 1 week ago

Merged, thanks @sethdivyansh