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

Rework drawing cards, associated replacement effects, and removing cards from library; implement [WHO] River Song #12700

Merged
merged 12 commits into from
Aug 24, 2024

Conversation

xenohedron
Copy link
Contributor

@xenohedron xenohedron commented Aug 22, 2024

  1. Remove obsolete MageAction framework, move code directly into PlayerImpl::drawCards
  2. Clarify event for drawing two or more cards
  3. Clarify methods for drawing cards from library
  4. Add support for replacement effects to draw from bottom rather than top of library
  5. Implement [WHO] River Song
  6. Fix Teferi's Ageless Insight incorrect interaction with "for each card drawn this way" #12616 by storing number of cards drawn through the replacement event

@JayDi85 JayDi85 changed the title Implement [WHO] River Song Implement [WHO] River Song, rework draw and remove from library Aug 22, 2024
@JayDi85
Copy link
Member

JayDi85 commented Aug 22, 2024

Be careful, remove and draw are different actions. [[Grenzo, Dungeon Warden]]

IMG_0702

BTW found source of the nulls in cards list — must be fixed by null check:
IMG_0701

Copy link

Grenzo, Dungeon Warden - (Gatherer) (Scryfall) (EDHREC)

{X}{B}{R}
Legendary Creature — Goblin Rogue
2/2
Grenzo, Dungeon Warden enters with X +1/+1 counters on it.
{2}: Put the bottom card of your library into your graveyard. If it's a creature card with power less than or equal to Grenzo's power, put it onto the battlefield.

@@ -54,7 +54,7 @@ public boolean apply(Game game, Ability source) {
return true;
}
for (int i = 0; i < count; i++) {
Card card = opponent.getLibrary().removeFromTop(game);
Card card = opponent.getLibrary().getFromTop(game);
Copy link
Member

Choose a reason for hiding this comment

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

And don’t forget to add cards removing in code like that here. Maybe it’s better to keep removeFromXXX methods.

@xenohedron
Copy link
Contributor Author

Be careful, remove and draw are different actions. [[Grenzo, Dungeon Warden]]

Yeah, if it's not a draw, then it shouldn't be flagging empty library for game loss. But some devs in the past occasionally choose wrong method. Cellar Door is the one I fixed here.

BTW found source of the nulls in cards list — must be fixed by null check

I don't see the problem? CardsImpl won't add null

Maybe it’s better to keep removeFromXXX methods.

Yeah, after further consideration, I'm inclined to agree. I don't like that the code requires it, but for now, best to make minimal changes on that front.

@xenohedron xenohedron marked this pull request as draft August 23, 2024 02:55
@xenohedron xenohedron changed the title Implement [WHO] River Song, rework draw and remove from library Rework drawing cards, associated replacement effects, and removing cards from library; implement [WHO] River Song Aug 23, 2024
@xenohedron xenohedron marked this pull request as ready for review August 24, 2024 03:48
@xenohedron xenohedron merged commit 9fcbfde into magefree:master Aug 24, 2024
2 checks passed
@xenohedron xenohedron deleted the mageaction branch August 24, 2024 05:02
@xenohedron xenohedron mentioned this pull request Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teferi's Ageless Insight incorrect interaction with "for each card drawn this way"
2 participants