Skip to content
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

improve mulligan choice for AI #67

Closed
wants to merge 2 commits into from
Closed

Conversation

firemind
Copy link
Contributor

This improves the AI's mulligan decisions in 3 major ways:

  1. It bases max. and min. lands on the sum of all mana costs for each deck. This is to ensure that a modern burn deck is completely fine with 2 lands whereas a deck with a mana curve heavier on three- and four-drops might want at least 3. Certainly needs some fine-tuning but at least it now handles average modern decks better that have a lower mana curve than what is assumed for the randomly-generated decks.
  2. It checks how many spells in hand can actually be cast with the lands in hand. This ensures that a deck full of 1 drops is completely fine with just one land as long as that land enables it to cast it's 1 drops. For now this is only a positive check, if it doesn't match it falls back on just counting the lands. I'd like to also add a negative clause (like: always mulligan a hand with 3 islands and 4 green spells since you can't cast anything), but the problem is that it doesn't handle fetch lands and the like too well since I haven't figured out a clean way to consider all to combinations of duel lands it could fetch. But this is what part three is for:
  3. Now if the land count clause matches but the "can I cast anything" clause does not it will return an actual choice which will cause the AI to evaluate both paths (taking a mulligan or keeping).

@buildhive
Copy link

magarena » magarena #600 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@lodici
Copy link
Member

lodici commented Feb 20, 2015

Looks like pull request was made against an older version of MagicMulliganChoice - it has recently been updated to remove all references to the UI. Getting latest master and replacing getArtificialChoiceResults() with your updates should resolve.

@firemind
Copy link
Contributor Author

I merged it with the latest version from master.

@melvinzhang
Copy link
Contributor

@firemind I noticed you added ACTUAL_CHOICE_LIST and returned it in some situation. It isn't meaningful to require the AI to search through future possibilities by returning two options as the AI is unable to predict the next hand that will be drawn. The mulligan choice should return either YES_CHOICE_LIST or NO_CHOICE_LIST.

@buildhive
Copy link

magarena » magarena #607 SUCCESS
This pull request looks good
(what's this?)

@firemind
Copy link
Contributor Author

@melvinzhang Thanks for clarifying that. This would mean though that the full weight of making a mulligan decision will lie on whatever heuristic the MagicMulliganChoice uses. Unfortunately I doubt this will ever be able to make good decisions for all (or even most) decks because in my opinion the only way to see how good a hand is is to see where it will get you in the first couple of turns (or how well it disrupts the opponent in that phase of the game).

To fix this I see two possible solutions. Either enable the AI to evaluate the current 7 card hand against the "average" 6 card hand. Or have sophisticated heuristics that use profiles for a deck to know what a good hand or a bad hand looks like.

Since neither of those are quick solutions I propose replacing the ACTUAL_CHOICE with NO_CHOICE for now since not taking a mulligan will be right a higher percentage of the time than taking one.

I just wanted to note that there is still a lot of room for improvement for the AI in this crucial part of the game.

@melvinzhang
Copy link
Contributor

Replace ACTUAL_CHOICE with NO_CHOICE, merged + clean up in 80763b8 @mike please double check that the logic is unchanged, I tried to simplify some of the condition checking.

@firemind firemind deleted the mulligan branch June 25, 2016 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants