Add optional win condition feature

[sookmax]

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)

Optional win banner

optional_win_1080p.mov

Optional win banner (secret)

optional_win_secret_1080p.mov

Optional win banner (secret + reward)

optional_win_secret_reward_1080p.mov

[cpojer]

This will not actually apply the action properly! You need to move this case up to where case 'GameEnd': is to make sure it calls applyGameOverActionResponse.

[sookmax]

Ah I see. Just to make sure we're on the same page though, this is for the consumers of applyActionResponse(), right? Would it be possible for that consumer to call applyGameOverActionResponse() instead? Or maybe it's impossible to do so in some cases?

[cpojer]

Ah, this is funky since the function name/module name no longer makes sense now. Maybe we should rename this to getMatchingTeam since it gives us the team matching the ActionResponse?

[sookmax]

This is just my opinion, but I see processRewards() is the sole consumer of this getWinningTeam() function in the entire codebase, and it checks if the return value is 'draw' (i.e., winningTeam !== 'draw').

It just occurred to me that it might be a little awkward to have matchingTeam !== 'draw'? In the processRewards() perspective, I think it makes more sense to call it winningTeam indeed, since the winner takes the rewards?

Maybe we could wait until there are other use cases that calling getWinningTeam() wouldn't make sense at all?

Or maybe we could bring getWinningTeam() into processRewards.tsx as a local function and worry about other use cases later?

[cpojer]

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]

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

[cpojer]

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]

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

[sookmax]

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]

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 59c42ca

[cpojer]

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 59c42ca

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]

fyi your work on this issue spawned another one that will really add depth to the game: #35

[sookmax]

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: #35

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

[sookmax]

🤦‍♂️ 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]

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

[sookmax]

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]

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 🙇‍♂️