-
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
Alternative solution to problem of unplayable cards from target adjustment #12842
base: master
Are you sure you want to change the base?
Conversation
Mage/src/main/java/mage/target/targetadjustment/ConditionalTargetAdjuster.java
Show resolved
Hide resolved
I think this is a fine direction for the short-term solution. |
@@ -12,6 +12,7 @@ | |||
public interface TargetAdjuster extends Serializable { | |||
|
|||
// Warning: This is not Copyable, do not use changeable data inside (only use static objects like Filter) | |||
// Note: in playability check for cards, targets are not adjusted. |
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.
Original commit 9c52dfa reverted from master due wrong usage. Current PR don’t use that code, but it have docs about skip adjusting.
- Is it fixed a bugs in current master code or just a refactored for better class usage?
- Is it require additional fixes due not working adjusters in playable check?
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 fixes the same major bug as 9c52dfa, where cards that should be playable are not, such as trying to use [[Long River's Pull]] with a Gift, where the base spell's target is more restrictive than the adjusted version.
- There are some cases where a spell that isn't currently playable is highlighted as if it were, such as [[Bloodchief's Thirst]] if you don't have enough mana to kick it and the only creatures on the battlefield have a higher MV, but I don't think there's currently any reasonable way to do that.
[[Bloodchief's Thirst]] |
Bloodchief's Thirst - (Gatherer) (Scryfall) (EDHREC)
|
#12753 was my original attempt at fixing it, but actually applying target adjusters proved overly complex and not worthwhile. We could instead make it official that target adjusters are not applied for the pre-activation target check, and adjust the
ConditionalTargetAdjuster
accordingly.