[LTR] Add Goldberry, River-Daughter

[alexander-novo]

I've added the card. I've played a few games with her and she looks fine, but I'm gonna add some tests for some unhappy paths.

Also I borrowed the counter choosing interface from Resourceful Defense, which has you go through a dialog for each type of counter on Goldberry and choose the amount for each individual one. But this is a bit clunky and only enforces the 1+ counter rule on the card by forcing you to move the last counter if you haven't moved anything else. So I'd like to vamp up the player.getMultiAmmount UI a bit to support this card.

[alexander-novo]

[[Goldberry, River-Daughter]]

[[Resourceful Defense]]

[github-actions]

Goldberry, River-Daughter - (Gatherer) (Scryfall) (EDHREC)

{1}{U}
Legendary Creature — Nymph
1/3
{T}: Move a counter of each kind not on Goldberry, River-Daughter from another target permanent you control onto Goldberry.
{U}, {T}: Move one or more counters from Goldberry onto another target permanent you control. If you do, draw a card.

Resourceful Defense - (Gatherer) (Scryfall) (EDHREC)

{2}{W}
Enchantment
Whenever a permanent you control leaves the battlefield, if it had counters on it, put those counters on target permanent you control.
{4}{W}: Move any number of counters from target permanent you control to another target permanent you control.

[alexander-novo]

I made a few changes to make the player.getMultiAmount API a bit beefier. Now it supports min, max on each individual option in addition to a global min, max. I still need to add some tests for this new capability (the previous tests still work but don't test the new functionality), but I think I'm ready for a review on what I've got.

[alexander-novo]

Right now the dialog defaults to all 0s, but I've half a mind to change it so that it instead calls MultiAmountType.prepareDefaltValues and uses those values. This would change Goldberry's effect, for instance, to always have the first counter type automatically start with a value of 1.

[xenohedron]

Curious, does this improvement help with either

• DistributeCountersEffect allows you to put 0 counters on a creature #9379 or
• There has to be a separate refactor for effects that distribute among targets #9457 ?

[alexander-novo]

No both of those seem to be targeting issues. This is more for choosing amounts among multiple different types of counters.

[xenohedron]

This looks great to me. I'll ask @JayDi85 to take a look at it as well.

[xenohedron]

should return true? though probably doesn't matter

[alexander-novo]

I'm going to be honest, I don't know what the return value for effects is supposed to signify. It's always seemed nonsensical to me.

[Grath]

I'm pretty sure the return value signifies whether the effect actually happened.

[alexander-novo]

It looks like the return value is only used for effects which are part of mana abilities and special actions. In which case it isn't really clear to me when effects like this one should or should not return false (does it even matter?).

[xenohedron]

Since this is an inner custom effect that nothing else will call, it does not matter. But the convention is as Grath describes, to return true if something happens and false if nothing happens. It can matter for common effects that have their apply() method called within the apply() method of another effect (either a subclass calling super, or just creating a new effect to use its logic) and trying to take action based on whether or not that effect succeeded. Of course, whenever attempting this it is important to double check that the logic will work as intended. Because it so often doesn't matter, there are lots of effects that don't adhere properly to the convention with no ill effects other than confusing developers.