Add optional win condition feature

sookmax commented 1 month ago

Hopefully closes #17

Well I was not positive that I would be able to put together a PR for this feature, but thanks to @cpojer's comments in #17, here is my attempt 😁

One thing in particular I want to mention here though, is that I decided to handle OptionalWin in applyGameOverActionResponse() instead of applyActionResponse() since it feels like OptionalWinActionResponse is very similar to GameEndActionResponse (they have exact same fields except for the type, and they require the usage of pickWinningPlayer() and processRewards()).

Default win condition (no 'optional' checkbox)

'Optional' checkbox (unchecked)

'Optional' checkbox (checked)

cpojer commented 1 month ago

Wow, this is a huge PR. Thank you for implementing it, this is really great work.

I left a few inline comments, but I think we are pretty close on it. Here is some overall feedback:

I know it kinda gets confusing here because these are called "win conditions", and we are adding "optional win conditions", but they don't actually win the game. Let's rename "OptionalWin" to "OptionalCondition" everywhere. Note that you need to manually edit apollo/ActionMap.json and run pnpm codegen.
When you press the (i) button on the right hand side of a map, we should show whether conditions are optional. Let's split required and optional ones and show the optional ones below the required ones with text like "Complete optional conditions for extra rewards.". See GameDialog.tsx.

sookmax commented 1 month ago

Hey thanks for the thorough review! Let me work on your feedbacks and get back to you 🫡

cpojer commented 1 month ago

One thing I missed earlier, we aren't verifying that the game can actually continue. It's currently implied, but not tested. Would you mind adding a test with an optional condition and a regular win condition and verifying that it's possible to first achieve the optional condition, then the win condition and the game ends?

sookmax commented 1 month ago

Yeah that makes a lot of sense. I'll see to that as well!

sookmax commented 1 month ago

I know it kinda gets confusing here because these are called "win conditions", and we are adding "optional win conditions", but they don't actually win the game. Let's rename "OptionalWin" to "OptionalCondition" everywhere. Note that you need to manually edit apollo/ActionMap.json and run pnpm codegen.

Hey one problem here. So I was changing OptionalWin to OptionalCondition and faced an issue where generated EncodedActions.tsx contains the wrong encoded type for OptionalCondition (44 instead of 43, thus giving me a type error when I run pnpm tsc)

So I was looking at generate-actions.tsx to find out what went wrong, and it seems when getStableTypeID(type, name) processes the arguments (type = 'action' & name = 'OptionalCondition'), it extracts shortName as 'Optional' (with name.replace(/Action(Response)?|Condition$/, '');), resulting in an increment of actionCounter and a false entry in actionMap ('Optional' => [ 44, [] ]). And I think this is the root cause of an erroneous EncodedActions being generated, although not 100% sure.

If what I'm suspecting is correct, then would you like to change the regex in name.replace(/Action(Response)?|Condition$/, ''); or come up with another name for OptionalCondition?

sookmax commented 1 month ago

When you press the (i) button on the right hand side of a map, we should show whether conditions are optional. Let's split required and optional ones and show the optional ones below the required ones with text like "Complete optional conditions for extra rewards.". See GameDialog.tsx.

This is addressed in 59c42cac2dec8232da2315c313ea0a1b430f0986

cpojer commented 1 month ago

Getting close!

Hey one problem here. So I was changing OptionalWin to OptionalCondition and faced an issue where generated EncodedActions.tsx contains the wrong encoded type for OptionalCondition (44 instead of 43, thus giving me a type error when I run pnpm tsc)

🤦‍♂️ oh man, nice find. There are "Conditions" in Effects which will later be used for scripting in the editor, and they are encoded in the same way as actions. I think the best fix here is to change getShortName to take a type arg, and then apply a different regex depending on whether the type is action (name.replace(/Action(Response)?$/, '')or condition (name.replace(/Condition$/, '')). Sorry for the inconvenience.

This is addressed in https://github.com/nkzw-tech/athena-crisis/commit/59c42cac2dec8232da2315c313ea0a1b430f0986

Sorry, maybe I wasn't clear. I'd prefer to split required and optional conditions into two sections. The top one with required conditions should be unchanged, then it should say "Complete optional conditions for extra rewards." and list all the optional ones.

cpojer commented 1 month ago

fyi your work on this issue spawned another one that will really add depth to the game: https://github.com/nkzw-tech/athena-crisis/issues/35

sookmax commented 1 month ago

Hey thanks for additional feedbacks! I truly appreciate them 😁. Let me work on them and get back to you.

By the way, they must have gone unnoticed while I was updating too many things at once, but I've actually left a couple of questions on inline review threads (ones that I haven't resolved above), so could you also take a look at them? 🥹

fyi your work on this issue spawned another one that will really add depth to the game: https://github.com/nkzw-tech/athena-crisis/issues/35

haha this is great! I'll take a look at that after this PR is polished and merged!

sookmax commented 1 month ago

🤦‍♂️ oh man, nice find. There are "Conditions" in Effects which will later be used for scripting in the editor, and they are encoded in the same way as actions. I think the best fix here is to change getShortName to take a type arg, and then apply a different regex depending on whether the type is action (name.replace(/Action(Response)?$/, '')or condition (name.replace(/Condition$/, '')). Sorry for the inconvenience.

done in 59d09ac

One thing I missed earlier, we aren't verifying that the game can actually continue. It's currently implied, but not tested. Would you mind adding a test with an optional condition and a regular win condition and verifying that it's possible to first achieve the optional condition, then the win condition and the game ends?

done in 96c073d

I only used WinCriteria.DefeatAmount for both optional and required win conditions. If there's any other conditions (or scenarios) that we should test, let me know!

Sorry, maybe I wasn't clear. I'd prefer to split required and optional conditions into two sections. The top one with required conditions should be unchanged, then it should say "Complete optional conditions for extra rewards." and list all the optional ones.

done in d8de90f

I think I'm the one who misunderstood the request haha sorry. Here's a new screenshot:

cpojer commented 1 month ago

Thanks for adding this feature. This is awesome work, and the funds were sent to you!

sookmax commented 1 month ago

Hey thanks for merging my PR! I had a lot of fun implementing the feature and collaborating with you 😄.

I've noticed, however, some UI issues that I wanted to discuss with you.

1. Objective counter keeps increasing after an optional objective is done.

Not sure if clipping the value and keep showing 1/1 to the user or removing the displayed data after an optional objective is fulfilled is better. What do you think?

2. First line `justify-content: space-between` and second line `justify-content: flex-start` when PlayerCard is expanded.

I think it would look better when both of the lines are justify-content: flex-start, but I want to know what you think!

3. Checkbox 'x' mark position

Maybe it's intentional, but to my eye the X mark is a little bit up too high?

cpojer commented 1 month ago

1. Objective counter keeps increasing after an optional objective is done.

Not sure if clipping the value and keep showing 1/1 to the user or removing the displayed data after an optional objective is fulfilled is better. What do you think?

Actually, I think we should not show any objectives in the PlayerCard for players that have completed the objective.

2. First line justify-content: space-between and second line justify-content: flex-start when PlayerCard is expanded.

I think it would look better when both of the lines are justify-content: flex-start, but I want to know what you think!

Actually, I think it would be best to keep the funds numbers on the left, and all the other info on the right.

3. Checkbox 'x' mark position

Maybe it's intentional, but to my eye the X mark is a little bit up too high?

Yeah that's a bug in the documentation site because the documentation site's CSS is conflicting. I'll fix this one, but feel free to send a PR for issue 1 and 2 🙇‍♂️

nkzw-tech / athena-crisis