Hinata fixes

[alexander-novo]

Attempting to fix #8685 (this never actually got fixed) and #10112

Just some tests for now.

[alexander-novo]

[[Hinata, Dawn-Crowned]]

Wanted to write down a list of interesting cards to write tests for:

• [[Volcanic Offering]] - This one is interesting because it has a target adjuster that lets opponents choose, and they can choose the same permanents that you do. Target adjusters don't work at all with Hinata right now, so this card is always unaffected by Hinata's effect when determining whether or not the spell may be cast.
• [[Magma Opus]] - I think Hinata might only be looking at the targets of the first effect here. Not too sure. Can only reduce by up to -4.
• [[Mystic Confluence]] Modal where the same mode may be chosen multiple times.
• [[Prismari Command]] Modal where a specific number of different modes have to be chosen which could have the same target.
• [[Wail of the Forgotten]] Modal where the number of modes you can choose is dependent on game state???
• [[Sublime Epiphany]] Modal where you can "choose one or more" and some of the targets can overlap (what if the only nonland permanent on the battlefield is also the only creature you control)

[github-actions]

Hinata, Dawn-Crowned - (Gatherer) (Scryfall) (EDHREC)

{1}{U}{R}{W}
Legendary Creature — Kirin Spirit
4/4
Flying, trample
Spells you cast cost {1} less to cast for each target.
Spells your opponents cast cost {1} more to cast for each target.

Volcanic Offering - (Gatherer) (Scryfall) (EDHREC)

{4}{R}
Instant
Destroy target nonbasic land you don't control and target nonbasic land of an opponent's choice you don't control.
Volcanic Offering deals 7 damage to target creature you don't control and 7 damage to target creature of an opponent's choice you don't control.

Magma Opus - (Gatherer) (Scryfall) (EDHREC)

{6}{U}{R}
Instant
Magma Opus deals 4 damage divided as you choose among any number of targets. Tap two target permanents. Create a 4/4 blue and red Elemental creature token. Draw two cards.
{U/R}{U/R}, Discard this card: Create a Treasure token.

Mystic Confluence - (Gatherer) (Scryfall) (EDHREC)

{3}{U}{U}
Instant
Choose three. You may choose the same mode more than once.
• Counter target spell unless its controller pays {3}.
• Return target creature to its owner's hand.
• Draw a card.

Prismari Command - (Gatherer) (Scryfall) (EDHREC)

{1}{U}{R}
Instant
Choose two —
• Prismari Command deals 2 damage to any target.
• Target player draws two cards, then discards two cards.
• Target player creates a Treasure token.
• Destroy target artifact.

Wail of the Forgotten - (Gatherer) (Scryfall) (EDHREC)

{U}{B}
Sorcery
Descend 8 — Choose one. If there are eight or more permanent cards in your graveyard as you cast this spell, choose one or more instead.
• Return target nonland permanent to its owner's hand.
• Target opponent discards a card.
• Look at the top three cards of your library. Put one of them into your hand and the rest into your graveyard.

Sublime Epiphany - (Gatherer) (Scryfall) (EDHREC)

{4}{U}{U}
Instant
Choose one or more —
• Counter target spell.
• Counter target activated or triggered ability.
• Return target nonland permanent to its owner's hand.
• Create a token that's a copy of target creature you control.
• Target player draws a card.

[alexander-novo]

Another weird test case: something makes [[Collective Effort]] cost {1} more to cast and you only have 2 white mana available to cast it. It would be possible to cast this with Hinata if you have an untapped creature.

[github-actions]

Collective Effort - (Gatherer) (Scryfall) (EDHREC)

{1}{W}{W}
Sorcery
Escalate—Tap an untapped creature you control. (Pay this cost for each mode chosen beyond the first.)
Choose one or more —
• Destroy target creature with power 4 or greater.
• Destroy target enchantment.
• Put a +1/+1 counter on each creature target player controls.

[alexander-novo]

I'm also pretty sure [[Battlefield Thaumaturge]] is incorrect as well.

[github-actions]

Battlefield Thaumaturge - (Gatherer) (Scryfall) (EDHREC)

{1}{U}
Creature — Human Wizard
2/1
Each instant and sorcery spell you cast costs {1} less to cast for each creature it targets.
Heroic — Whenever you cast a spell that targets this creature, this creature gains hexproof until end of turn.

[alexander-novo]

Some notes:

• Currently, the opponent cost increase and controller cost decrease are calculated the same way, but when the game is checking for activatable abilities, it needs to be more conservative in its approach and assume that the caster could attempt to cast a spell for its minimum possible cost. For an opponent, this means something different than for the controller of Hinata - an opponent could attempt to cast a spell with a minimum number of targets, but Hinata's controller could attempt to cast a spell with a maximum number of targets. These end up being vastly different calculations.
• Hinata's effect currently only checks the first mode of a spell, but obviously other modes can have different targets. As mentioned above, an opponent could select as few modes as possible (and of the ones available, the ones with the fewest targets) and the controller could select as many modes as possible (and of the ones available, the ones with the most targets).
• Hinata's effect currently only pays attention to each "target"'s ("target" here meaning the word "target" in an effect or an instance of Target in the Effect - not an object being targeted, as referred to in Hinata's ability) maximum number of targets allowed to be selected - but of course an opponent could actually choose the minimum number of targets allowed and a controller may not actually be able to select the maximum number of targets for a particular "target" since there may not be that many targets to actually select. There is currently a min calculation which caps the effect to the number of possible targets, but as discussed in the next point this is too coarse of a calculation and needs to be done on an individual "target" level.
• Hinata's effect currently takes the maximum number of targets over all "target"s in an effect but of course the different targets select from each "target" would all decrease/increase the cost. As well, it is not a simple matter of addition between each "target" since the allowable targets between each "target" could overlap - an opponent could purposefully choose to overlap targets to reduce the effect (how does this work with cards that need "another target" e.g. [[Cosmic Hunger]]?), and a controller may not be able to choose non-overlapping targets (i.e. like Magma Opus when the only permanents to tap are also the only targets to choose for damage)

[github-actions]

Cosmic Hunger - (Gatherer) (Scryfall) (EDHREC)

{1}{G}
Instant
Target creature you control deals damage equal to its power to another target creature, planeswalker, or battle.

[JayDi85]

Some info/ideas (do not debug real code) for second effect (opponent's cost increase):

1. Cost modification calls 1 time per ability, so problem must be in HinataDawnCrownedEffectUtility.getTargetCount -- it's return too much amount, so cost increases too much due Hinata, Dawn-Crowned taxes opponent spells too much. #8685
2. CardUtil.getAllSelectedTargets - it's use real selected targets, so real activation code is fine. The problem must be in playable calculation, e.g. with getAllPossibleTargets.
3. CardUtil.getAllPossibleTargets - it's impossible to find max possible targets for multi modal spells (if it allow to activate same mode multiple times). Also it's useless to search max targets amount to playable calculation (e.g. max cost). It's need only minimum possible targets.

Looks like that's a problem code:

It must look for required and min targets amount like that -- maybe it can help:

if (game.inCheckPlayableState()) {
  return abilityToModify.getTargets().stream()
                    .mapToInt(a -> !a.isRequired() ? 0 : a.getMinNumberOfTargets())
                    .min()
                    .orElse(0);
} else {
  return CardUtil.getAllSelectedTargets(abilityToModify, game).size();
}

[alexander-novo]

Yup, these were all the same conclusions I came to. Right now, I'm focusing on writing tests to hit all of the edge cases I can think of.

The code snippet you give is too simple, though. It doesn’t acknowledge the existence of different modes and also as mentioned above a simple min() over all Targets isn’t sophisticated enough.

[JayDi85]

BTW it’s ok to reduce whole generic cost in hard use case like Integer.MAX (example: possible targets count in single mode and whole generic cost on multiple modes) — real cast will use real targets list anyway. The main task here - pass playable check.

P.S. Snippet can be improved by code
abilityToModify.getModes().values()
.stream()
.map(Mode::getTargets)

[alexander-novo]

I’d personally prefer to end up with something that’s correct in most cases (and overly conservative in others) because I personally dislike when a card highlights as playable when it isn’t actually.

[Grath]

Last I knew, the playable check is used to not only determine whether a card is highlighted as playable but also whether or not you can attempt to play it at all so if we can't be 100% correct, we must be liberal in corner cases.

[alexander-novo]

@Grath yeah that's what led me to fixing this because as it is right now the card is nigh-unplayable.