gameEvent: Implement Throw Card Event #50

criticic · 2024-06-02T06:28:53Z

Description

adds the throw card handler

Fixes #44

Motivation and Context

[Explain the motivation behind these changes and provide any relevant context.]

How to Test

[Describe the steps to test the changes made in this pull request.]

Related Issues

[If applicable, mention any related issues or pull requests.]

Checklist

I have tested these changes locally.
I have reviewed the code and ensured it follows the project's coding guidelines.
I have updated the documentation, if necessary.
I have assigned reviewers to this pull request.

Screenshots (if applicable)

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

kuv2707

This seems promising, but is blocked by other issues currently.

kuv2707 · 2024-06-03T08:04:30Z

ARCHITECTURE.md

 	"data": {
-		"cardID": "red-5"
+		"card": {


We'd want to use the CardID here
We'd have to be able to retrieve all cards by id.
Can you do that in a commit before the actual changes?

I'll do it in a moment, rest of the things, i feel i have done

Is there much to do in this PR, apart from the documentation change, I guess the api handler will have a get card from card id function

You should address all the feedback so far first.

backend/uno-game-engine/engine.ts

kuv2707 · 2024-06-03T08:09:18Z

backend/uno-game-engine/events/throwCard.ts

+    return player.cards.find((c) => c.id === cardId);
+}
+
+export function throwCard(game: GameEngine, event: GameEvent): EventResult {


Some minor changes need to be made to the logic when the drawCard function is completed. I'll review it once that is done.

kuv2707

Looks great!
Posted few comments.

backend/uno-game-engine/events/throwCard.ts

vercel · 2024-06-07T08:46:38Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
multiplayer-uno	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 12, 2024 2:17pm

kuv2707

Posted some comments.
Mainly, we'd need to work on:

tests for the logic
keeping special and wild separate.

kuv2707 · 2024-06-07T10:32:03Z

backend/src/types.d.ts

@@ -1,7 +1,7 @@
 // We declare those types which are used throughout the application here.
 // For types that are used only in one file, we can declare them in that file itself.

-type CardType = 'number' | 'special' | 'wild';
+type CardType = 'number' | 'special';


Any reason for removing the wild type?
It made logical sense to separate special from wild, since some of the game rules (like DRAW4CHALLENGE) depend on counting the number of wild cards the user has. I think we'd want to keep them separate from special cards.

the reasoning was that wild cards are just colorless special card, in the sense that they do soemthing to the game state, instead of just getting added to the pile.

i not aware of the draw4challenge rule, whenever i have played it before, we never had it, Though i guess uno is popular for everyone having their own rules, if its a official/popular rule, I'll add it back ig

I wasn't aware of it either. But the rules we are following (link in ARCHITECTURE.md) mention it and it seems fun to implement.
Even if that weren't the case, it is a good idea to model the types after the actual game objects/properties.

backend/src/uno-game-engine/engine.ts

kuv2707 · 2024-06-07T10:38:43Z

backend/src/uno-game-engine/events/throwCard.ts

We'd need to add tests for all the logic here. This seems to be a crucial part of the game logic with many imaginable corner cases and bugs, so we'd need to look out for them in the tests.

kuv2707 · 2024-06-07T16:57:24Z

@criticic Please prioritize this PR over any new work as this will help further work on the engine.

kuv2707 · 2024-06-11T05:44:03Z

@criticic Are you working on it or should I close it?

criticic · 2024-06-11T05:45:49Z

I’ll send a updated PR by tonight On 11 Jun 2024, at 11:14 AM, Kislay ***@***.***> wrote: @criticic<https://github.com/criticic> Are you working on it or should I close it? — Reply to this email directly, view it on GitHub<#50 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJVJDCRQRLZ4Z66WSIF7XTLZG2FDTAVCNFSM6AAAAABIUZLLWKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJZHA2DAMRSGY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Adds the throw card event handler. Fixes shivansh-bhatnagar18#44 Signed-off-by: Sagnik Mandal <[email protected]>

kuv2707 · 2024-06-12T18:21:18Z

ARCHITECTURE.md

-	}
+	"type": "THROW_CARD",
+	"playerId": "1",
+	"cardId": "card-number-red-5",


We'd want to go with the data object here.

kuv2707 · 2024-06-12T18:24:50Z

backend/src/uno-game-engine/events/throwCard.ts

+        return { type: 'ERROR', message: 'Player not found' };
+    }
+
+    const card = findCard(player, event.data.card.id);


It seems unnecessary to send the whole card object from the client when we can use id. I had earlier mentioned that we must setup a function to retrieve cards by id. In accordance with that, we will only be sending the card id from the frontend.

kuv2707 · 2024-06-12T18:27:39Z

backend/tests/engine.test.ts

These provide foundational tests for the game engine, which is good to have.

kuv2707

The logic needs some minor reworking. For the record, the THROW_CARD event object will have cardId as the only attribute in its data.
Also:

We should shift the getting-card-by-id function to deck.ts.
As mentioned earlier, it is very important to have tests for the throw-card logic because of subtle corner cases.

I think we can defer the tests for now, given that we are a little behind the schedule, but please be motivated to follow up on this with the tests. (you can open a PR for that even without any issue).

Despite some minor issues, I think we can merge it as it is good enough for the time being. I'll make some of the amendments in subsequent commits.

Some general suggestions:

Respond to all feedback, even with an emoji reaction. This lets the maintainer know that the particular feedback has been addressed.
Tag the maintainer if you want another review.

kuv2707 · 2024-06-12T18:41:04Z

Merged, thanks @criticic

criticic · 2024-06-12T18:42:15Z

Just pushed a commit, did that end up being merged? On 13 Jun 2024, at 12:11 AM, Kislay ***@***.***> wrote: Merged #50<#50> into master. — Reply to this email directly, view it on GitHub<#50 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJVJDCVMFYZI2IOG5E6WYTTZHCI4NAVCNFSM6AAAAABIUZLLWKVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTGEZTKOJYHE4DONI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

criticic · 2024-06-12T18:43:38Z

hey @kuv2707 please see the latest branch: https://github.com/criticic/csoc-multiplayer-uno/tree/feat/event-throw-card

criticic marked this pull request as ready for review June 3, 2024 07:52

criticic force-pushed the feat/event-throw-card branch from 96835ef to 5656cd6 Compare June 3, 2024 07:56

criticic changed the title ~~throw card DRAFT~~ gameEvent: Implement Throw Card Event Jun 3, 2024

criticic force-pushed the feat/event-throw-card branch from 5656cd6 to 273d3d5 Compare June 3, 2024 07:58

kuv2707 force-pushed the master branch from dfd80ee to fe179ab Compare June 3, 2024 07:58

kuv2707 reviewed Jun 3, 2024

View reviewed changes

kuv2707 added under review There have been some rounds of review on the code changes in the PR. area: Backend(Game Engine) Issues related to the UNO game engine in the backend. labels Jun 3, 2024

kuv2707 force-pushed the master branch from fe179ab to 199d751 Compare June 3, 2024 08:22

criticic force-pushed the feat/event-throw-card branch from 273d3d5 to 334ad0a Compare June 3, 2024 13:57

kuv2707 reviewed Jun 7, 2024

View reviewed changes

backend/uno-game-engine/events/throwCard.ts Outdated Show resolved Hide resolved

backend/uno-game-engine/events/throwCard.ts Outdated Show resolved Hide resolved

criticic force-pushed the feat/event-throw-card branch from 334ad0a to 6993e75 Compare June 7, 2024 08:46

vercel bot deployed to Preview June 7, 2024 08:46 View deployment

kuv2707 requested changes Jun 7, 2024

View reviewed changes

kuv2707 added the priority: high label Jun 7, 2024

kuv2707 force-pushed the master branch 3 times, most recently from 4dd6ce4 to 7bc3d56 Compare June 8, 2024 19:10

criticic force-pushed the feat/event-throw-card branch from 6993e75 to b05f7a3 Compare June 11, 2024 15:49

vercel bot deployed to Preview June 11, 2024 15:49 View deployment

criticic force-pushed the feat/event-throw-card branch from b05f7a3 to 8ad0eca Compare June 11, 2024 15:52

vercel bot deployed to Preview June 11, 2024 15:52 View deployment

criticic force-pushed the feat/event-throw-card branch from 8ad0eca to 937e3a0 Compare June 11, 2024 15:52

vercel bot deployed to Preview June 11, 2024 15:53 View deployment

gameEvent: Implement Throw Card Event

435d475

Adds the throw card event handler. Fixes shivansh-bhatnagar18#44 Signed-off-by: Sagnik Mandal <[email protected]>

criticic force-pushed the feat/event-throw-card branch from 937e3a0 to 435d475 Compare June 12, 2024 14:16

vercel bot deployed to Preview June 12, 2024 14:17 View deployment

kuv2707 reviewed Jun 12, 2024

View reviewed changes

backend/tests/engine.test.ts

Copy link

Collaborator

kuv2707 Jun 12, 2024

Choose a reason for hiding this comment

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

These provide foundational tests for the game engine, which is good to have.

kuv2707 approved these changes Jun 12, 2024

View reviewed changes

kuv2707 merged commit 0b74c25 into shivansh-bhatnagar18:master Jun 12, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gameEvent: Implement Throw Card Event #50

gameEvent: Implement Throw Card Event #50

criticic commented Jun 2, 2024

kuv2707 left a comment

kuv2707 Jun 3, 2024

criticic Jun 7, 2024

criticic Jun 11, 2024

kuv2707 Jun 12, 2024 •

edited

Loading

kuv2707 Jun 3, 2024

kuv2707 left a comment

vercel bot commented Jun 7, 2024 •

edited

Loading

kuv2707 left a comment

kuv2707 Jun 7, 2024

criticic Jun 11, 2024

kuv2707 Jun 11, 2024

kuv2707 Jun 7, 2024

kuv2707 commented Jun 7, 2024

kuv2707 commented Jun 11, 2024

criticic commented Jun 11, 2024 via email

kuv2707 Jun 12, 2024

kuv2707 Jun 12, 2024 •

edited

Loading

kuv2707 Jun 12, 2024

kuv2707 left a comment

kuv2707 commented Jun 12, 2024

criticic commented Jun 12, 2024 via email

criticic commented Jun 12, 2024

gameEvent: Implement Throw Card Event #50

gameEvent: Implement Throw Card Event #50

Conversation

criticic commented Jun 2, 2024

Description

Motivation and Context

How to Test

Related Issues

Checklist

Screenshots (if applicable)

kuv2707 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuv2707 Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuv2707 left a comment

Choose a reason for hiding this comment

vercel bot commented Jun 7, 2024 • edited Loading

kuv2707 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuv2707 commented Jun 7, 2024

kuv2707 commented Jun 11, 2024

criticic commented Jun 11, 2024 via email

Choose a reason for hiding this comment

kuv2707 Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuv2707 left a comment

Choose a reason for hiding this comment

kuv2707 commented Jun 12, 2024

criticic commented Jun 12, 2024 via email

criticic commented Jun 12, 2024

kuv2707 Jun 12, 2024 •

edited

Loading

vercel bot commented Jun 7, 2024 •

edited

Loading

kuv2707 Jun 12, 2024 •

edited

Loading