Add support for 702.190b with Sneak'ed creatures entering tapped and attacking#14960
Add support for 702.190b with Sneak'ed creatures entering tapped and attacking#14960muz wants to merge 6 commits into
Conversation
…attacking the same target
|
@muz something useful in cost adjusters and target adjusters? 
|
I'll be the first to admit it's not the most elegant solution for sure. Do you want to elaborate specifically on which bits have you concerned? The code syntactically could be tidied up a bit, but if getting this PR has gotten the discussion going and things moving in the right direction to address user reported bugs and missing features from a new set, then it's still an improvement (not saying it needs merging yet, but getting timely attention on it is good). I went through a number of iterations locally before landing on this initial pass that at least has the test case passing and seems to be OK-ish with some rudimentary testing. Sneak is definitely a weird one in the sense that it does cast the spell, so interaction and the stack needs interacting with unlike Ninjitsu, but it also needs to retain some knowledge of the state of the items being used to pay the cost (namely attack target) so the incoming permanent from the spell itself enters both tapped, and attacking the correct item (if possible, otherwise it defaults to just being tapped per rule 506.3c) |
| // Sync targets and effects from the card's spell ability at target-selection | ||
| // time. This allows SneakAbility to be constructed before addEffect/addTarget | ||
| // are called on the card's spell ability (construction order doesn't matter). | ||
| this.setTargetAdjuster((ability, game) -> { |
There was a problem hiding this comment.
This is an interesting solution direction, which would take care of #14975 if it works.
However, I worry it might not work right with playable checks, because target adjusters aren't called for that. I guess since it'll never have targets during playable check, the problem is simply that it might appear playable when no legal targets.
There was a problem hiding this comment.
Playable check calls target and cost adjusters (if it miss somewhere then must be fixed). If you need different logic (e.g. allow more targets before real select) then custom target class with possible targets and playable state check can help.
There was a problem hiding this comment.
Well, I don't know for sure, but this comment suggests it doesn't. So would need to be tested for this use case. (But I think it's better to have a utility method that adds the effects to both SpellAbility rather than trying to grope them from a TargetAdjuster.)
| public boolean activate(Game game, Set<MageIdentifier> allowedIdentifiers, boolean noMana) { | ||
| if (!super.activate(game, allowedIdentifiers, noMana) | ||
| public ActivationStatus canActivate(UUID playerId, Game game) { | ||
| if (game.getStep() == null |
There was a problem hiding this comment.
interesting null check, I believe that's null only at the very start of the game
|
|
||
| @Override | ||
| public void adjustTargets(Game game) { | ||
| // Skip the second run (inside AbilityImpl.activate's mode loop) if |
There was a problem hiding this comment.
I'm not following this. If you still think this is the best approach, can you walk me through what's happening in more detail?
| // can find the synced targets on the ability. The adjustTargets override above | ||
| // prevents the second run (inside super) from overwriting those selections. | ||
| adjustTargets(game); | ||
| return super.activate(game, allowedIdentifiers, noMana); |
There was a problem hiding this comment.
These workarounds are not filling me with confidence. Especially if discrepancies between test behavior and in game behavior. I suspect the target adjuster approach is not actually what we want to do.
| } | ||
| // Enter tapped | ||
| permanent.setTapped(true); | ||
| // Enter attacking — build a CombatGroup directly, since addAttackerToCombat |
There was a problem hiding this comment.
Why do you need to build a CombatGroup manually? I don't believe any other implementation needs to do that. I would expect addAttackingCreature to work. (Maybe current usages of addAttackerToCombat really ought to be using addAttackingCreature?)
| if (!super.pay(ability, game, source, controllerId, noMana, costToPay)) { | ||
| return false; | ||
| } | ||
| // Register the ETB effect now that defenderId is known (set in addReturnTarget during super.pay()) |
There was a problem hiding this comment.
should this comment be one line lower? it seems independent of the costs tag?
|
OverloadAbility was recently reworked (#14015) to add effects/targets to both the card base SpellAbility and the OverloadAbility. I think for instants/sorceries that have alternate casting implemented as a SpellAbility, we probably want a general purpose solution to solve the problem of effects/targets getting to both. This has caused bugs for DisturbAbility (7da8557) and SpectacleAbility (#10164) in the past. |
3 participants
Linked to #14006
Fixes #14895