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

Improved AI Shockland play #6778

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marthinwurer
Copy link
Contributor

  • AI now knows that taplands can enter untapped
  • Added a hacky fix to the shockland payment logic so that it will pay if there's just a card of that mana cost.
  • Added tests for AI land playing logic

return true;
}
// // Check if the AI intends to play the card and if it can pay for it with the mana it has
Copy link
Contributor

@tool4ever tool4ever Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

way too hacky

this logic was working fine in general:
image

if you want to enforce different decisions with other API those should be enhanced instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After sleeping on it, I agree that it's a bit too hacky. I think the API that's being called in the original code represents the correct line of thought, but the actual implementation of that API doesn't match up with what I'd think it would. I don't think that the internal canPlaySA stuff should be caring about whether or not the AI wants to play it, just whether it has the ability to. Basically 'can' vs 'should'. I don't know what all is relying on the current can implying should logic there, though. I can try setting up some unit tests to make sure I don't break anything too much in the process. I probably won't be able to work on this much until the weekend.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, those AI ability methods tend to play a bit fast and loose with the responsibilities of "can I play it", "should I play it", and "actually play it" (they handle assigning targets as well for instance). Took me a while to puzzle out which ones were called and when on the couple occasions I've dabbled in them. Maybe I'm not seeing the bigger picture but it'd be nice to have more documentation someday for how they're meant to be used.

@Hanmac
Copy link
Contributor

Hanmac commented Jan 16, 2025

Why the SimultionTest -> AiTest changes?

@@ -542,6 +542,8 @@ private Card chooseBestLandToPlay(CardCollection landList) {
}

// try to skip lands that enter the battlefield tapped if we might want to play something this turn
// TODO figure out a better life level for shocking in lands
boolean check_shocks = player.getLife() > 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good catch! A side effect of coding past midnight 😅

@marthinwurer
Copy link
Contributor Author

Why the SimultionTest -> AiTest changes?

I wanted to reuse the setup of those tests for my land drop tests and figured that I should move them up and into a non-simulation-specific and more generic parent class.

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.

4 participants