Problems with Target.choose() and Player.choose() : clicking "Done" after choosing less than max amount of cards doesn't behave as expected.

[padfoothelix]

While coding [WHO] The Five Doctors (see #13691) I found two problems with choosing cards. They happen when you choose less cards than the max number of targets and then click "Done" to finish choosing. I believe they are caused by coding inconsistencies in methods Target/TargetCardInLibrary.choose() and HumanPlayer.choose(). Here is what I found out :

1. This first problem concerns methods TargetCardInLibrary.choose() and Target.choose() (and probably Target.chooseTarget() and similar methods from the Targets class). Here is the problem : when you select less cards than the max number via these methods and then click "Done", nothing happens. You can continue selecting cards. Choosing only stops when you click "Done" after having selected the same number of cards twice in a row (they don't have to be the same cards).

For example :

• if you select card A, then click "Done" twice, choosing stops
• if you select card A, then click "Done", then add card B and remove card A, then click "Done", it stops.
• if you select card A, then click "Done", then add card B, then click "Done", it doesn't stop (because the number of selected cards change).

This should affect all calls to Player.searchLibrary(), which calls TargetCardInLibrary.choose(). It can be checked using e.g. [[Burnished Hart]]. It doesn't exactly break the game flow, since you can still select whatever you want, but it is clearly not the expected behaviour.

This is the relevant code from Target.choose() (file TargetImpl.java) :

// stop by cancel/done
            if (!targetController.choose(outcome, this, source, game)) {
                chosen = isChosen(game);
                return chosen;
                break;
            }

            chosen = isChosen(game);

            // stop by nothing to use (actual for human and done button)
            if (prevTargetsCount == this.getTargets().size()) {
                break;
}

The first condition ("stop by cancel/done") is actually never reached if you choose at least one card. The second is what allows the player to stop choosing by using "Done", but it doesn't work the first time it is used, and it doesn't seem that this is what it is for.

The thing is, unless I am missing something, the do{}while(true); loops in Target/TargetCardInLibrary.choose() are actually useless. They seem to be here to let the player choose several targets in succession, but the HumanPlayer.choose/chooseTarget() method called inside already does that. Therefore there seems to be a rather easy fix in just removing the loop and the "nothing to use" condition. I have tried that, and it does seem to resolve the problem.

2. The second problem is simpler and concerns method HumanPlayer.choose(outcome, cards, target, ability, game). Contrary to the way HumanPlayer.choose(outcome, card, ability, game) and other similar methods are coded, it returns false after "Done" is clicked. This is the relevant code :

 if (responseId != null) {
     ...
     } else {
     // done or cancel button pressed
     if (target.isChosen(game)) {
         // try to finish
         return false;
     } else {
          if (!required) {
          ...

This means that if less cards than max are chosen, the choose method will simply not return the correct answer (this was mentioned in #13691).

3. As mentioned in #11134, and event after the recent work in #13638, there are a lot of redundant methods that let a player choose cards. The chooseTarget() variants are, if I understand correctly, now outdated because that distinction is managed with isNotTarget(), but there is also the variants with the additional Cards argument, which seems to me strictly better than the one without. All of these are often slightly differently coded…

[github-actions[bot]]

Burnished Hart - (Gatherer) (Scryfall) (EDHREC)

{3} Artifact Creature — Elk 2/2 {3}, Sacrifice this creature: Search your library for up to two basic land cards, put them onto the battlefield tapped, then shuffle.

[JayDi85]

It's a known issue for human/ai players (diff between target.choose and player.choose) -- it's still under development with #13638

Not all player.choose methods migrated to loop choice logic yet (human/ai/test) -- so use of target.choose is workaround, cause it's same for any type of player and use loop logic inside.

[JayDi85]

Final solution to all that targets rework: any choose dialog from any player type must use loop logic, e.g. must do full targets selection and return result.