Add card 'Downpour'

[gseitz]

No description provided.

[gseitz]

Maybe a convenience function for asking for n targets of the same target spec returning a value of TargetList [a] would be handy. Another option would be to add an Int argument to the AskTarget question and let the clients ask the question n times.

[MedeaMelana]

Hi Gerolf, Downpour says "up to 3 target creatures", right now your implementation wants exactly 3 targets. To build this card I think you need to create a new function in Target.hs, something like askMaybeTarget :: PlayerRef -> TargetSpec a -> Magic (Maybe (TargetList a)). You probably need a new Question too then, something like AskMaybeTarget. Also isn't foldMap (pure <$>) more commonly written as sequenceA?

[gseitz]

Do we really need the Maybe in the type, since a TargetList a could also be an empty list. Or how about extending the AskTarget question with a field like data Optionality = Mandatory | Optional? This way the client would know to provide an extra No Choice option to the user.

As a general questions, do you prefer more specialised AskXyz data constructors or fewer but more general questions?

And yes, sequenceA is obviously better.

[MedeaMelana]

I think we do need the Maybe because an empty TargetList a must evaluate to an a while a Maybe (TargetList a) can evaluate to a Maybe a. Imagine you have an empty TargetList PlayerRef that has no targets (because it is empty) but still always evaluates to a PlayerRef, that's not possible.

Optionality would be a good idea if we could indeed use empty TargetLists but I don't think we can.

As for general/specific questions, I think the rule of thumb should be something like this: reasonable specific questions for things that concern the rules/engine (like targets), more general questions for random questions that cards might ask. For example, many cards ask the player to choose a color, this will probably end up as a general question.

[gseitz]

As you suggested, I added the question AskMaybeTarget, for which the CLI offers a No Target choice.
Additionally, if the answer to the first or second question was No Target (ie. Nothing), the card doesn't ask for more targets. This is probably not super adhering to rules, but more a convenience when playing.
If we want to move this behaviour to the client, I think we'll have to modify AskMaybeTarget to take an upper bound, and then the client can ask questions and stop early depending on the provided answers. Even though I have implemented the shortcut in the card, my gut feeling says it should be the client's decision, not the card's. So maybe change AskMaybeTarget to also take an upper bound?

[MedeaMelana]

Actually it's exactly adhering to the rules, this looks great :-) AskMaybeTarget asking just 1 question like now is fine. I agree we should have a convenience function but I think it should be in a helper module in project Magic somewhere, probably in Target since it's to do with targetting. Then that function could do something that Downpour currently doesn't (but should): you can't pick the same target twice if the card mentions the word "target" just once (see rule 114.3). So if there is only 1 creature on the battefield, Downpour has at most 1 target. I think the burden for this logic should be in the engine, not in the client, so that's why the current question is fine IMO.

[gseitz]

Thanks for the clarification on the rules. I just pushed the latest changes, which in addition to askMaybeTarget also includes the function askTargetsUpTo :: Int -> PlayerRef -> TargetSpec a -> Magic (TargetList [a]). This function piggy-backs on the previously introduced AskMaybeTarget and pretty much all the logic that was implemented in the effect of Downpour, is now in this generally useful function. What do you think?

Edit: askTargetsUpTo also takes care of not letting the user choose a target more than once.

[gseitz]

Hm, now that I look at it again, I'm not sure TargetList [a] is an appropriate type, because the Snoc constructor actually captures a value of type EntityRef pointing to an actual card, right? So going from 1 EntityRef to more than 1 a (ie. [a]) doesn't seem right.

Disclaimer: I'm not sure I fully grokked TargetList a, so my reasoning might be way off.

[MedeaMelana]

This is perfect, using [a] actually makes sense if you want to capture "up to n" targets.