Add optional win condition amount feature #47
Conversation
hera/editor/lib/WinConditionCard.tsx
Outdated
| const amount = parseInteger(value); | ||
| setAmount(amount); | ||
| if (amount) { | ||
| const newCondition = { | ||
| ...condition, | ||
| amount, | ||
| }; | ||
| if (validate(newCondition)) { | ||
| setAmount(amount); | ||
| onChange(newCondition); |
There was a problem hiding this comment.
This change unfortunately doesn't work since it disallows invalid values while typing, like for example pressing backspace to set the amount to "" and then typing "100". Why did you make this change?
There was a problem hiding this comment.
This has to do with the fact that when you're adjusting the amount for OptionalObjectiveAmount, the input value changes regardless whether we have a valid number of optional objectives specified. So I wanted to make sure the user could only increment the amount up to the number of currently added optional objectives, so when the user notices the input value doesn't increment anymore, they could add more optional objectives.
For example, let's say if the user adds OptionalObjectiveAmount in the map editor, and specifies the value of amount to be 1. He then goes on to add a condition that he means to be optional, but he forgets to check optional checkbox and then hit the Playtest. The UI will give the feedback There is a problem with this map. The win conditions are not valid. [invalid-win-conditions] and the user thinks the problem was him not ticking off the optional checkbox and he goes on and do just that. But when he clicks Playtest again with everything looking valid, he'll instead get another There is a problem with this map. The win conditions are not valid. [invalid-win-conditions]. And the reason is condition.amount has not been updated when the user incremented the amount from 0 to 1, right after he added the OptionalObjectiveAmount in the beginning, because back then, there wasn't any optional condition added yet (since the user added it after he had added OptionalObjectiveAmount)
I thought it'd be safe to do it since all other 'amount' objectives only checks whether the amount specified by the user is between MIN_AMOUNT and MAX_AMOUNT. Sorry, I didn't really think about the use case where the user types in the value directly to the input as you mentioned.
Do you want me to revert this change?
There was a problem hiding this comment.
Yeah I think we should revert it for now.
| const denyingPlayer = pickWinningPlayer( | ||
| previousMap, | ||
| activeMap, | ||
| lastActionResponse, | ||
| deniedOptionalCondition, | ||
| ); | ||
| let deniedPlayers = ( | ||
| deniedOptionalCondition.players && | ||
| deniedOptionalCondition.players.length > 0 | ||
| ? deniedOptionalCondition.players | ||
| : map.active | ||
| )?.filter((playerId) => playerId !== denyingPlayer); | ||
|
|
||
| if ( | ||
| deniedPlayers.some( | ||
| (player) => !deniedOptionalCondition.failed?.has(player), | ||
| ) | ||
| ) { | ||
| deniedPlayers = deniedPlayers.filter( | ||
| (player) => !deniedOptionalCondition.failed?.has(player), | ||
| ); |
There was a problem hiding this comment.
Is this the part that you are unsure about?
There was a problem hiding this comment.
Yeah I'm not entirely sure whether defining deniedPlayers based on condition.players or map.active is the right move. I think it'd be best to identify exactly who was denied by a destructive action, but I guess that information is hard to get in the current structure?
|
Sorry for the late response, it took me a moment to get back to this PR. I left a few comments – am I right in assuming the biggest challenge is making sure which players to check for denied conditions? What else do you think is a rough edge? |
Yeah pretty much. By rough edges I think I meant more of my general uncertainties around the implementation of |
|
Got it! It looks very similar to how I would have implemented it, so I'll try to take a look through all the possibilities of where things could go wrong with this condition before merging. Would you mind updating the tests? |
sookmax
force-pushed
the
optional-condition-amount
branch
from
f91097c
to
2bdeb16
Compare
I guess the charge values have been updated? This is done in 2bdeb16 |
Co-authored-by: CoconutTank <[email protected]> GitOrigin-RevId: bfa41934cf96bda26820f8274d97ebeea9ab2fcc
This PR is my attempt at #35.
The implementation kinda got bloated to cover the case where optional objective amount fails due to one of the target objectives is no longer achievable as discussed extensively in #35 with @cpojer.
I feel like there are still a lot of rough edges to be ironed out in the PR, but thought it'd be better to share the work early than to keep pulling my hair out, thinking about some potential edge cases by myself.