-
Notifications
You must be signed in to change notification settings - Fork 762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[don't merge] Fix target adjusters sometimes not allowing abilities/cards to be used #12753
base: master
Are you sure you want to change the base?
Conversation
…ivated/cast when they should be
if (ability.getTargetAdjuster() != null){ | ||
// We can't easily determine how the adjuster will adjust the target | ||
// So just always treat it as legal to cast | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, should've explained myself further.
With Long River's Pull, the decision to give the gift has not yet been made in the canPlay check. Given that, even if we do call the adjuster, it wouldn't adjust the target correctly since it would default to "no gift". Sometimes the adjuster makes casting more restrictive, sometimes it makes casting less restrictive, so there's no default I can always use.
The only way I could think of to solve the issue would be to have any target adjuster that can make casting less restrictive to have a new function that return all of the possible Targets it could make, and then check if any of them succeed. That would be a lot more code, and would require adding that method to some pre-existing target adjusters that aren't generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must use getNetMana logic: checking game.inCheckPlayableState()
and assign max possible target in playable state and real target in real game state.
Example solution: with ConditionalTargetAdjuster
-- add withPlayableStateTarget(Target replacementTargetOnPlayableState)
or like that. So adjustTargets
can use it on game.inCheckPlayableState()
.
Alternative solution: extends ConditionalTargetAdjuster and override adjustTargets
with code like "if playable state then add playable target else call super.adjust".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"max possible target" is not necessarily well-defined as a single Target. It might be for all existing cards (haven't checked), but for example:
"Destroy target creature. If ~ was kicked, instead destroy two target noncreature permanents,"
AFAIK there's no possible way to correctly handle this with one Target. I suppose you could use just "Target permanent"? Require the user of the target adjuster to define what the playable state target is? That's probably good enough for most situations, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in many use cases getNetMana return {any}
instead complex calculations. It's ok for UX. Show wrongly playable card is much better (AI and user has a choice) than show wrongly non playable card (AI and user can't play it at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is equivalent to always returning {any} for all target adjusters. I'll try the more complex solution later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s more equal to old ALLOW_USERS_TO_PUT_NON_PLAYABLE_SPELLS_ON_STACK_WORKAROUND )))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only skips the "is there a legal target" check, all other checks remain in place. Many previous target adjusters effectively did this, with no target originally and adding either form of the target in the adjuster.
@JayDi85 changes are complete and tested, though I didn't test the full interactions with Splice (related to the relevant target code). |
There were enough target adjusters where things broke with this that I'm defaulting to a blank |
Code goes to wrong direction. I'll look for better solution (if it exists). |
If I understand correctly, the bug is only currently present with ConditionalTargetAdjuster? For now, let's just use the original 5 line fix, but instead of It's not acceptable for abilities that should be playable to not be playable. It's okay if a few abilities display as playable when they can't actually be played. More extensive rework can always be done in future. P.S. if you're going to rework target adjuster logic, make it a parameter by mode, not by ability, see #12397 |
The bug (that adjusters don't apply to the "is this ability playable" check) is present in everything but it's only important with
That seems reasonable to me, should I just commit that directly? |
Right, that's what I meant to say, it's just ConditionalTargetAdjuster that is obviously impacted by the bug. Thanks for clarifying.
Yes, though I think best to make a new issue summarizing the source of the problem and what ought to be done for a proper fix, and including a TODO that references it. |
@xenohedron @ssk97 -- it's not a fix (9c52dfa). It's just hide a real problem and wrong logic of target adjusters (not conditional only). |
It doesn't fix the deeper incorrect logic, true. However, it does fix the issue of cards not being castable when they should be, which is a major problem. Aside from Technically, by a strict reading of the MTG rules, the pre-activation target check doesn't even exist - but it's very convenient and is in Arena, so I think it's good to have. |
If you're planning on releasing a new version in 1 day (as mentioned in the scryfall issue) I'd strongly recommend that you also have a fix for the "card should be playable but isn't" issue - whether it be via the short fix you reverted, the rework in this PR, or your own solution. |
Reverting 9c52dfa makes no sense to me whatsoever. The workaround is minimal and does not add technical debt. As discussed, it fixes incorrect in-game behavior, which is important. I'm all for a proper solution to the root cause in the future, but in the meantime, not allowing the workaround is a disservice to the xmage community. |
Currently, the spell or ability always checks the default target to check if it can be used. However, this fails if the target adjuster can adjust the targeting to be more lenient than
This isn't a perfect solution, as it allows cards to begin to be cast with no legal targets, but I can't think of a way to solve that. This fixes issues like [[Long River's Pull]]'s gift ability being unusable, as the card can only start being cast if there is a noncreature spell on the stack already.