-
Notifications
You must be signed in to change notification settings - Fork 834
[WHO] Implement The Five Doctors #13691
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
base: master
Are you sure you want to change the base?
Conversation
} | ||
} | ||
|
||
class TheFiveDoctorsSearchLibraryGraveyardEffect extends OneShotEffect { |
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.
- Use multizones search logic from refactor PutCardFromOneOfTwoZonesOntoBattlefieldEffect #11213 (ask about library, fill cards list, ask to select, move and shuffle)
- If you found buggy GUI then report about it — do not try to fix it by target/notTarget or other workarounds. choose and chooseTarget had an important diff — one for non-target choices (color protection do not work) and another for target choices (color protection works) due mtg rules
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.
For info: up to target/choice selection logic in GUI - human can select and unselect objects until their press a done button or their select max amount. There aren’t double dones. Maybe some dialogs calls multiple times.
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.
-
I hadn't seen that code, thanks for pointing it out. I'm not sure if it can be used for [[The Five Doctors]] though, because if I'm reading this correctly, it doesn't exactly let the player search the library, it just selects cards from it with the
getCards
. When a player searches his library, he/she should be able to see all the cards in it, correct ? What's more, would triggered abilities that trigger whenever a player searches trigger correctly ? -
The change from
choose
tochooseTarget
method is not to work around a bug, it's just that the common method that useschoose
is not meant to select more than one card. -
Thanks for the precision on the double done issue. I believe this might be a bug in a common class ; I'll try and understand what's going on and I'll report about it if it's necessary.
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.
Hmm, search is important, yes. Then try to use another approach instead custom classes -- use two conditional standard effects and custom text like that:
There are:
SearchLibraryGraveyardPutInHandEffect
withInvertCondition
+KickedCondition
SearchLibraryGraveyardPutOntoBattlefieldEffect
withKickedCondition
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.
I used code from those methods, but I can't use them directly, because they are coded to find just one card with a specific name. I need up to five cards with a given type. That's why I use a custom class. I didn't find a common class that does the trick.
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.
well, then keep custom class
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.
I have done a little more research and found this about the graveyard search :
- it is possible to use the player.choose(outcome, targetcard, source, game)
- it is possible to use the player.chooseTarget(outcome, cards, targetcard, source, game)
- it is not possible to use the player.choose(outcome, cards, targetcard, source, game) - I believe it is because it returns false if you don't choose all possible targets.
I'm not sure if it's better to use option 1 or 2. Option 2 opens up the graveyard automatically, which is more user-friendly, I suppose. I'm not sure if the target/notTarget is relevant, since we search out of the battlefield anyway, and the targetcard is defined with the boolean notTarget set as true.
Let me know if you think one option is better.
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.
I can’t reproduce your player.choose(cards) bug in gui’s tests dialogs — it’s allow to selects any cards.
I need more details: dialog call code, target setting and cards list for choosing
P.S. target.choose is better way to choose targets - it works same way for any player type
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.
On this line (114) :
if (controller.chooseTarget(outcome, controller.getGraveyard(), targetGrave, source, game)) {
In you replace chooseTarget with choose, then you can select cards from the graveyard, but then when you click Done, nothing happens (cards are not put in hand or battlefield), unless you have selected the maximum amount of cards.
I don't think this is a bug, I think it's just the way the method is supposed to work. If I understand correctly, it returns false if you interrupt the choosing process.
PS : I used player.choose because it was the method used in SearchLibraryGraveyardPutInHandEffect
, but I can try and use target.choose if that's better.
} | ||
if (controller.chooseUse(outcome, "Search your library for up to " + CardUtil.numberToText(cardsToSearch) + " " + filter.getMessage() + '?', source, game)) { | ||
TargetCardInLibrary targetLib = new TargetCardInLibrary(0, cardsToSearch, filter); | ||
targetLib.clearChosen(); |
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.
No need in clear for new target
TargetCardInLibrary targetLib = new TargetCardInLibrary(0, cardsToSearch, filter); | ||
targetLib.clearChosen(); | ||
if (controller.searchLibrary(targetLib, source, game)) { | ||
if (!targetLib.getTargets().isEmpty()) { |
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.
No need for empty check before “for”
Implement [[The Five Doctors]].
Part of #10653.
Card was tested and seems to work as intended.
Note that the
searchLibrary
method means that you have to click "done" twice when you're done searching cards from your library if you want to stop before the max number of targets. I'm not sure why that is.I used code from the common
SearchLibraryGraveyardPutInHandEffect
, but I had to change the graveyard search logic fromcontroller.choose
tocontroller.chooseTarget
, otherwise you can't choose more than one card.As always, thanks for your work !